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