"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Omar Polo <op@omarpolo.com>
Subject:
Re: got patch; got patch -R
To:
Stefan Sperling <stsp@stsp.name>
Cc:
Christian Weisgerber <naddy@mips.inka.de>, gameoftrees@openbsd.org
Date:
Thu, 21 Jul 2022 11:14:57 +0200

Download raw body.

Thread
Stefan Sperling <stsp@stsp.name> wrote:
> On Wed, Jul 20, 2022 at 11:51:56PM +0200, Omar Polo wrote:
> > A quick solution may be to disable the diff3 merge in -R mode:
> 
> > However, I think we can do better.
> 
> Yes, swapping the diff3 tmpfile/oldfile file arguments (and labels)
> in patch.c will do a reverse-merge for -R mode.
> 
> The old blob will still be used as the merge base.

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)

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.)

ok?


P.S.: yeah, there's an error in the order of the label passed to
got_merge_diff3; it should be (newlabel, anclabel, oldlabel) -- or
alternatively swap the order of tmpfile and oldfile -- will fix this in
a follow-up diff.

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,68 @@ 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
+
+	jot 10 | sed s/4/four/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/4/four/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) > $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "failed to revert the patch" >&2
+		test_done $testroot $ret
+		return 1
+	fi
+
+	echo 'G  numbers' > $testroot/stdout.expected
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done $testroot $ret
+		return 1
+	fi
+
+	jot 10 | sed s/6/six/g > $testroot/wt/numbers.expected
+	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 +1967,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