commit f4602cbd0dc693f398f68e64406240dcba6b57c0
from: Omar Polo <op@omarpolo.com>
via: Thomas Adam <thomas@xteddy.org>
date: Sat Jul 23 13:24:37 2022 UTC

fix `got patch -R' when using diff3 merge

`got patch -R' fails spectacularly when applied on a diff that contains
the info of the original blob for the diff3 merge machinery since it
tries to apply the reverse of the patch to the old blob.

change it to run the patch (_not_ reversed) on the old blob and then
swap the arguments to got_diff3_merge which gives us the correct reverse
merge of the diff.  while here add a test case too.

reported by naddy, discussed with and ok stsp@

commit - 408db73d08e36bce6cbc5780738f3517b1297ee1
commit + f4602cbd0dc693f398f68e64406240dcba6b57c0
blob - c6dfc285c84c6ac028b7f49f4ad9244a52f39e71
blob + a3ad82ba986e1bb5e246d67e517888bbe2ca5a83
--- lib/patch.c
+++ lib/patch.c
@@ -311,7 +311,36 @@ done:
 	imsg_free(&imsg);
 	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.
@@ -698,7 +727,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;
@@ -726,6 +756,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;
 	}
 
@@ -806,6 +838,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;
@@ -907,35 +952,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,
@@ -1000,15 +1016,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
blob + 5b3df4e132acb5f7a05acfcdaca7e9d1eb7ee25a
--- regress/cmdline/patch.sh
+++ regress/cmdline/patch.sh
@@ -1874,7 +1874,81 @@ EOF
 	fi
 	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