From: Stefan Sperling <stsp@stsp.name>
Subject: Re: three bugs related to merge commits
To: James Cook <falsifian@falsifian.org>, Mark Jamsek <mark@jamsek.com>, gameoftrees@openbsd.org
Date: Mon, 17 Apr 2023 20:29:02 +0200

On Mon, Apr 17, 2023 at 07:30:05PM +0200, Stefan Sperling wrote:
> On Mon, Apr 17, 2023 at 04:20:26PM +0000, James Cook wrote:
> > Updated patch:
> > - Dropped the test for the third bug (test_merge_same_change).
> > - Fixed test_send_merge_commit:
> > 	- Merge origin/master into master, not vice versa.
> > 	- Check return value of got up && got merge, just in case.
> 
> Here is the same test with very minor tweaks by me, and a fix for the problem.
> Turns out the send code did indeed enforce rebase semantics. Which doesn't make
> sense when sending a merge commit, of course. Just requiring that some common
> ancestor exists between the local and remote commit should be enough, right?
> Or should suck checks be skipped entirely for some reason?

James pointed out mistakes in this diff on IRC.
It is possible to stack single-parent commits on top of merge commits
which breaks the logic I implemented in my first patch.

Here is a better version.

-----------------------------------------------
 fix sending merge commits; with a regression test by James Cook
 
diff 2b70695630ead3274d31727f6b477f3544dc9c9a afb44abd174338ad71dd507d68c4e208b71ef265
commit - 2b70695630ead3274d31727f6b477f3544dc9c9a
commit + afb44abd174338ad71dd507d68c4e208b71ef265
blob - baff5126bef7368795495150bcfabcb49a70dcd8
blob + 14d8ca7d40214b9873e11074ff64f87b425340c1
--- lib/send.c
+++ lib/send.c
@@ -162,7 +162,7 @@ check_linear_ancestry(const char *refname, struct got_
 }
 
 static const struct got_error *
-check_linear_ancestry(const char *refname, struct got_object_id *my_id,
+check_common_ancestry(const char *refname, struct got_object_id *my_id,
     struct got_object_id *their_id, struct got_repository *repo,
     got_cancel_cb cancel_cb, void *cancel_arg)
 {
@@ -178,15 +178,15 @@ check_linear_ancestry(const char *refname, struct got_
 		    "bad object type on server for %s", refname);
 
 	err = got_commit_graph_find_youngest_common_ancestor(&yca_id,
-	    my_id, their_id, 1, repo, cancel_cb, cancel_arg);
+	    my_id, their_id, 0, repo, cancel_cb, cancel_arg);
 	if (err)
 		return err;
 	if (yca_id == NULL)
 		return got_error_fmt(GOT_ERR_SEND_ANCESTRY, "%s", refname);
 
 	/*
-	 * Require a straight line of history between the two commits,
-	 * with their commit being older than my commit.
+	 * Require a straight line of history between commits,
+	 * with their commit being older than my commit(s).
 	 *
 	 * Non-linear situations such as this require a rebase:
 	 *
@@ -581,8 +581,9 @@ got_send_pack(const char *remote_name, struct got_path
 				err = got_ref_resolve(&my_id, repo, my_ref);
 				if (err)
 					goto done;
-				err = check_linear_ancestry(refname, my_id,
-				    their_id, repo, cancel_cb, cancel_arg);
+				err = check_common_ancestry(refname,
+				    my_id, their_id, repo,
+				    cancel_cb, cancel_arg);
 				free(my_id);
 				my_id = NULL;
 				if (err)
blob - db8ed49392c78fe028db90d4d5c318b3d0fa8130
blob + 039969d0a73f3e098fb5b034b25b18a4223036f4
--- regress/cmdline/send.sh
+++ regress/cmdline/send.sh
@@ -322,6 +322,45 @@ test_send_delete() {
 	test_done "$testroot" "$ret"
 }
 
+test_send_merge_commit() {
+	local testroot=`test_init send_merge_commit`
+	local testurl=ssh://127.0.0.1/$testroot
+
+	if ! got clone -q "$testurl/repo" "$testroot/repo-clone"; then
+		echo "got clone command failed unexpectedly" >&2
+		test_done "$testroot" 1
+		return 1
+	fi
+
+	echo 'upstream change' > $testroot/repo/alpha
+	git_commit $testroot/repo -m 'upstream change'
+
+	got checkout $testroot/repo-clone $testroot/wt-clone > /dev/null
+	echo 'downstream change' > $testroot/wt-clone/beta
+	(cd $testroot/wt-clone && got commit -m 'downstream change' > /dev/null)
+
+	got fetch -q -r $testroot/repo-clone
+	(cd $testroot/wt-clone && got update > /dev/null)
+	(cd $testroot/wt-clone && got merge origin/master > /dev/null)
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "got merge command failed unexpectedly" >&2
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	(cd $testroot/repo && git config receive.denyCurrentBranch ignore)
+
+	got send -q -r $testroot/repo-clone
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	test_done "$testroot" 0
+}
+
 test_send_delete() {
 	local testroot=`test_init send_delete`
 	local testurl=ssh://127.0.0.1/$testroot
@@ -1583,6 +1622,7 @@ run_test test_send_delete
 run_test test_send_basic
 run_test test_send_rebase_required
 run_test test_send_rebase_required_overwrite
+run_test test_send_merge_commit
 run_test test_send_delete
 run_test test_send_clone_and_send
 run_test test_send_tags