From: Omar Polo Subject: got patch vs unchanged files To: gameoftrees@openbsd.org Date: Fri, 18 Mar 2022 18:47:06 +0100 While testing a diff for an unrelated thing I noticed that `got patch' fails to handle the case of a patch that tries to add a file already versioned. Long story short, I'm using got_worktree_status to check the status of a file before applying the patch. got_worktree_status calls worktree_status with report_unchanged=0, so the `can_add' callback in lib/patch.c doesn't get called and check_file_status assumes that it's possible to add the file. Patch below adds another argument to got_worktree_status so the caller can decide to have also unchanged files reported to the callback. If we don't want to change got_worktree_status I have another idea to solve this issue locally in patch.c, but I thought this was the easiest way. --- Why the regression suite didn't found this earlier? There's a test for it, test_patch_illegal_status, which seems to be OK, fails as expected with 'beta: file has unexpected status', but for the wrong reason :) Since check_file_status returns NULL, patch_file is happy to create the file but then got_worktree_schedule_add fails because the file is already present in the repository. What's worse is that at the end of apply_patch I call unlink(newpath) in a tentative to roll back the creation of a file when aborting the operation which removes the file from the worktree. The patch below fixes this too by moving the unlink call up a bit so the file is deleted only when got_worktree_schedule_* fails. (i guess that we can also completely drop the unlink call, was there only to avoid leaving an extraneous file in the worktree if their scheduling failed.) --- diff 46ebad135d9dc52eb84d6985393298465fa7b3ff /home/op/w/got blob - fb5211d500574e714e2964dd6a9ab1ac8433e57d file + got/got.c --- got/got.c +++ got/got.c @@ -4690,7 +4690,7 @@ cmd_diff(int argc, char *argv[]) arg.ignore_whitespace = ignore_whitespace; arg.force_text_diff = force_text_diff; - error = got_worktree_status(worktree, &paths, repo, 0, + error = got_worktree_status(worktree, &paths, repo, 0, 0, print_diff, &arg, check_cancelled, NULL); free(id_str); goto done; @@ -5619,7 +5619,7 @@ cmd_status(int argc, char *argv[]) if (error) goto done; - error = got_worktree_status(worktree, &paths, repo, no_ignores, + error = got_worktree_status(worktree, &paths, repo, no_ignores, 0, print_status, &st, check_cancelled, NULL); done: TAILQ_FOREACH(pe, &paths, entry) @@ -10816,8 +10816,8 @@ cmd_histedit(int argc, char *argv[]) if (error) goto done; error = got_worktree_status(worktree, &paths, - repo, 0, check_local_changes, &have_changes, - check_cancelled, NULL); + repo, 0, 0, check_local_changes, + &have_changes, check_cancelled, NULL); got_pathlist_free(&paths); if (error) { if (error->code != GOT_ERR_CANCELLED) @@ -11495,7 +11495,7 @@ cmd_stage(int argc, char *argv[]) goto done; if (list_stage) - error = got_worktree_status(worktree, &paths, repo, 0, + error = got_worktree_status(worktree, &paths, repo, 0, 0, print_stage, NULL, check_cancelled, NULL); else { cpa.patch_script_file = patch_script_file; blob - 2a33834a903f96a6f0206be4636193177a5f443d file + include/got_worktree.h --- include/got_worktree.h +++ include/got_worktree.h @@ -171,7 +171,7 @@ typedef const struct got_error *(*got_worktree_status_ * a path, and a corresponding status code. */ const struct got_error *got_worktree_status(struct got_worktree *, - struct got_pathlist_head *, struct got_repository *, int no_ignores, + struct got_pathlist_head *, struct got_repository *, int, int, got_worktree_status_cb, void *, got_cancel_cb cancel_cb, void *); /* blob - 4aabb46d0d6d31262d423862cce84ad3cf0b11f6 file + lib/patch.c --- lib/patch.c +++ lib/patch.c @@ -564,20 +564,20 @@ check_file_status(struct got_patch *p, int file_rename static const struct got_error *err; if (p->old != NULL && p->new == NULL) - return got_worktree_status(worktree, old, repo, 0, + return got_worktree_status(worktree, old, repo, 0, 1, can_rm, NULL, cancel_cb, cancel_arg); else if (file_renamed) { - err = got_worktree_status(worktree, old, repo, 0, + err = got_worktree_status(worktree, old, repo, 0, 1, can_rm, NULL, cancel_cb, cancel_arg); if (err) return err; - return got_worktree_status(worktree, new, repo, 0, + return got_worktree_status(worktree, new, repo, 0, 1, can_add, NULL, cancel_cb, cancel_arg); } else if (p->old == NULL) - return got_worktree_status(worktree, new, repo, 0, + return got_worktree_status(worktree, new, repo, 0, 1, can_add, NULL, cancel_cb, cancel_arg); else - return got_worktree_status(worktree, new, repo, 0, + return got_worktree_status(worktree, new, repo, 0, 1, can_edit, NULL, cancel_cb, cancel_arg); } @@ -693,16 +693,18 @@ apply_patch(struct got_worktree *worktree, struct got_ if (err == NULL) err = got_worktree_schedule_add(worktree, newpaths, patch_add, pa, repo, 1); - } else if (p->old == NULL) + if (err) + unlink(newpath); + } else if (p->old == NULL) { err = got_worktree_schedule_add(worktree, newpaths, patch_add, pa, repo, 1); - else + if (err) + unlink(newpath); + } else err = report_progress(pa, oldpath, newpath, GOT_STATUS_MODIFY, NULL); done: - if (err != NULL && newpath != NULL && (file_renamed || p->old == NULL)) - unlink(newpath); free(parent); free(template); if (tmppath != NULL) blob - 26ce9d0c775728d7d4b6a3450c3704c51f29672a file + lib/worktree.c --- lib/worktree.c +++ lib/worktree.c @@ -3714,8 +3714,8 @@ done: const struct got_error * got_worktree_status(struct got_worktree *worktree, struct got_pathlist_head *paths, struct got_repository *repo, - int no_ignores, got_worktree_status_cb status_cb, void *status_arg, - got_cancel_cb cancel_cb, void *cancel_arg) + int no_ignores, int report_unchanged, got_worktree_status_cb status_cb, + void *status_arg, got_cancel_cb cancel_cb, void *cancel_arg) { const struct got_error *err = NULL; char *fileindex_path = NULL; @@ -3729,7 +3729,7 @@ got_worktree_status(struct got_worktree *worktree, TAILQ_FOREACH(pe, paths, entry) { err = worktree_status(worktree, pe->path, fileindex, repo, status_cb, status_arg, cancel_cb, cancel_arg, - no_ignores, 0); + no_ignores, report_unchanged); if (err) break; } blob - 65fb8dead18ae13233a8eb24e2f1f26a41d8906e file + regress/cmdline/patch.sh --- regress/cmdline/patch.sh +++ regress/cmdline/patch.sh @@ -824,7 +824,22 @@ EOF ret=$? if [ $ret -ne 0 ]; then diff -u $testroot/stderr.expected $testroot/stderr + test_done $testroot $ret + return 1 fi + + (cd $testroot/wt && got status) > $testroot/stdout + cat < $testroot/stdout.expected +~ alpha +? kappa +? patch +EOF + + cmp -s $testroot/stdout.expected $testroot/stdout + ret=$? + if [ $ret -ne 0 ]; then + diff -u $testroot/stdout.expected $testroot/stdout + fi test_done $testroot $ret }