Download raw body.
got patch; got patch -R
Stefan Sperling <stsp@stsp.name> wrote: > On Thu, Jul 21, 2022 at 11:14:57AM +0200, Omar Polo wrote: > > I did something similar to what you suggest, but instead I'm swapping > > tmpfile (which is the "new" file) and the ancestor: this way the merge3 > > is done with the patched file against the old blob (left) and the > > current revision (right). (yeah, it gets confusing easily) > > Our diff3 is very weird about which file is the ancestor. > This has confused me before. > > Diff3 as described in the "diff3 analysis" paper does a 'diff O A' > and a diff 'O B', where O is the original file, and files A and B > were both derived from O. > > The code we inherited from the OpenRCS merge code does a diff '1 3' > and diff '2 3'. Which seems wrong, since the O being used is 1 in > the first diff, and 2 in the second diff. It would seem that perhaps > 3 is base and it somehow works in reverse, but I recall trying to > pass the base for '3' and ending up breaking things. > > This could be the reason for mis-merges we have seen while using the > Myers diff algorithm. Using Patience diff seems to be good enough to > work around such issues, but it is not a proper fix. > > I recall trying to fix it properly and not getting anywhere. > Whenever I dive into diff3.c I come back out wanting to burn it all > down and rewrite it. This code is very hard to follow, with all the > ed-script nonsense and all the non-informative naming of things. that's interesting, thanks for the insight. admittedly i haven't looked (yet? ;-) into the diff3 implementation with attention, i just glanced at the got_merge_diff3 entry point. > > To do this, i'm postponing reverse_patch in the diff3 case: we don't > > need to reverse it unless we don't find the blob and fall back to the > > search/replace. > > > > The diff is a bit longer because i had to move reverse_patch early for > > apply_patch to see it, and i've added a (simple) regress. (would a more > > complex example help there? I'm already ensuring we're using the > > merge.) > > Yes, please test your fix with some non-trivial patches. The test you > are adding is so simple that it is perhaps working inspite of arguments > not being in the right order. Ideally, you should try to trigger a > conflict and then check whether the content in left/base/right matches > what you would expect. initially i thought that assessing that we've used a merge to revert the change was enough, but you're right and a not-so-trivial case is indeed better. i've updated the test, now it generates a conflict like this: 1 2 3 4 <<<<<<< --- numbers 5 6 ||||||| +++ numbers five ======= five six >>>>>>> commit $commit_id 7 8 9 10 which seems correct to me. The diff was 4 -5 +five 6 and the latest release of the file had s/5/five and s/6/six. does it looks correct to you too? (tbf i'm quite puzzled by how the "+++ numbers" bit has only "five" and not "five \n 6") > It might be useful to have a test case which triggers a merge conflict > and verifies that the result matches the result of 'got backout' (for > patch -R) and 'got cherrypick' (for regular patch). this goes right into my todo list. I've been posticipating a sweep of the patch test suite for a while, i need to look into that soon. > Regardless, your patch is a good first step; ok stsp@ diff /home/op/w/got commit - fced5a66069199024aaf413a06bcf544b959f6a8 path + /home/op/w/got blob - 703c22b84de200f2a0ec9fc0f7bdc70dc725ab20 file + lib/patch.c --- lib/patch.c +++ lib/patch.c @@ -314,6 +314,35 @@ done: return err; } +static void +reverse_patch(struct got_patch *p) +{ + struct got_patch_hunk *h; + size_t i; + int tmp; + + STAILQ_FOREACH(h, &p->head, entries) { + tmp = h->old_from; + h->old_from = h->new_from; + h->new_from = tmp; + + tmp = h->old_lines; + h->old_lines = h->new_lines; + h->new_lines = tmp; + + tmp = h->old_nonl; + h->old_nonl = h->new_nonl; + h->new_nonl = tmp; + + for (i = 0; i < h->len; ++i) { + if (*h->lines[i] == '+') + *h->lines[i] = '-'; + else if (*h->lines[i] == '-') + *h->lines[i] = '+'; + } + } +} + /* * Copy data from orig starting at copypos until pos into tmp. * If pos is -1, copy until EOF. @@ -700,7 +729,8 @@ static const struct got_error * apply_patch(int *overlapcnt, struct got_worktree *worktree, struct got_repository *repo, struct got_fileindex *fileindex, const char *old, const char *new, struct got_patch *p, int nop, - struct patch_args *pa, got_cancel_cb cancel_cb, void *cancel_arg) + int reverse, struct patch_args *pa, + got_cancel_cb cancel_cb, void *cancel_arg) { const struct got_error *err = NULL; struct stat sb; @@ -728,6 +758,8 @@ apply_patch(int *overlapcnt, struct got_worktree *work return err; else if (err == NULL) do_merge = 1; + else if (reverse) + reverse_patch(p); err = NULL; } @@ -808,6 +840,19 @@ apply_patch(int *overlapcnt, struct got_worktree *work goto done; } + if (reverse) { + char *s; + FILE *t; + + s = anclabel; + anclabel = newlabel; + newlabel = s; + + t = afile; + afile = tmpfile; + tmpfile = t; + } + err = got_opentemp_named(&mergepath, &mergefile, template); if (err) goto done; @@ -909,35 +954,6 @@ done: return err; } -static void -reverse_patch(struct got_patch *p) -{ - struct got_patch_hunk *h; - size_t i; - int tmp; - - STAILQ_FOREACH(h, &p->head, entries) { - tmp = h->old_from; - h->old_from = h->new_from; - h->new_from = tmp; - - tmp = h->old_lines; - h->old_lines = h->new_lines; - h->new_lines = tmp; - - tmp = h->old_nonl; - h->old_nonl = h->new_nonl; - h->new_nonl = tmp; - - for (i = 0; i < h->len; ++i) { - if (*h->lines[i] == '+') - *h->lines[i] = '-'; - else if (*h->lines[i] == '-') - *h->lines[i] = '+'; - } - } -} - const struct got_error * got_patch(int fd, struct got_worktree *worktree, struct got_repository *repo, int nop, int strip, int reverse, got_patch_progress_cb progress_cb, @@ -1002,15 +1018,16 @@ got_patch(int fd, struct got_worktree *worktree, struc if (err || done) break; - if (reverse) + /* reversal application with merge base is done differently */ + if (reverse && *p.blob == '\0') reverse_patch(&p); err = got_worktree_patch_check_path(p.old, p.new, &oldpath, &newpath, worktree, repo, fileindex); if (err == NULL) err = apply_patch(&overlapcnt, worktree, repo, - fileindex, oldpath, newpath, &p, nop, &pa, - cancel_cb, cancel_arg); + fileindex, oldpath, newpath, &p, nop, reverse, + &pa, cancel_cb, cancel_arg); if (err != NULL) { failed = 1; /* recoverable errors */ blob - 7901777201377549e2dce265cf173b09d716ee4d file + regress/cmdline/patch.sh --- regress/cmdline/patch.sh +++ regress/cmdline/patch.sh @@ -1875,6 +1875,80 @@ EOF test_done $testroot $ret } +test_patch_merge_reverse() { + local testroot=`test_init patch_merge_simple` + + got checkout $testroot/repo $testroot/wt > /dev/null + ret=$? + if [ $ret -ne 0 ]; then + test_done $testroot $ret + return 1 + fi + + jot 10 > $testroot/wt/numbers + (cd $testroot/wt && got add numbers && got commit -m +numbers) \ + > /dev/null + ret=$? + if [ $ret -ne 0 ]; then + test_done $testroot $ret + return 1 + fi + + local commit_id=`git_show_head $testroot/repo` + + jot 10 | sed s/5/five/g > $testroot/wt/numbers + (cd $testroot/wt && got diff > $testroot/wt/patch \ + && got commit -m 'edit numbers') > /dev/null + ret=$? + if [ $ret -ne 0 ]; then + test_done $testroot $ret + return 1 + fi + + jot 10 | sed -e s/5/five/g -e s/6/six/g > $testroot/wt/numbers + (cd $testroot/wt && got commit -m 'edit numbers again') >/dev/null + ret=$? + if [ $ret -ne 0 ]; then + test_done $testroot $ret + return 1 + fi + + (cd $testroot/wt && got patch -R patch) >/dev/null 2>&1 + ret=$? + if [ $ret -eq 0 ]; then + echo "unexpectedly reverted the patch" >&2 + test_done $testroot 1 + return 1 + fi + + cat <<-EOF > $testroot/wt/numbers.expected + 1 + 2 + 3 + 4 + <<<<<<< --- numbers + 5 + 6 + ||||||| +++ numbers + five + ======= + five + six + >>>>>>> commit $commit_id + 7 + 8 + 9 + 10 +EOF + + cmp -s $testroot/wt/numbers $testroot/wt/numbers.expected + ret=$? + if [ $ret -ne 0 ]; then + diff -u $testroot/wt/numbers $testroot/wt/numbers.expected + fi + test_done $testroot $ret +} + test_parseargs "$@" run_test test_patch_add_file run_test test_patch_rm_file @@ -1905,3 +1979,4 @@ run_test test_patch_merge_simple run_test test_patch_merge_gitdiff run_test test_patch_merge_conflict run_test test_patch_merge_unknown_blob +run_test test_patch_merge_reverse
got patch; got patch -R