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?

-----------------------------------------------
 fix sending merge commits; with a regression test by James Cook
 
diff 2b70695630ead3274d31727f6b477f3544dc9c9a fa629a210fc04aec80a0457b2ca37266b6a9ed54
commit - 2b70695630ead3274d31727f6b477f3544dc9c9a
commit + fa629a210fc04aec80a0457b2ca37266b6a9ed54
blob - baff5126bef7368795495150bcfabcb49a70dcd8
blob + 95e25c2b5c0fa84d768744fc339de7f2426e6a81
--- lib/send.c
+++ lib/send.c
@@ -206,6 +206,33 @@ realloc_ids(struct got_object_id ***ids, size_t *nallo
 }
 
 static const struct got_error *
+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)
+{
+	const struct got_error *err = NULL;
+	struct got_object_id *yca_id;
+	int obj_type;
+
+	err = got_object_get_type(&obj_type, repo, their_id);
+	if (err)
+		return err;
+	if (obj_type != GOT_OBJ_TYPE_COMMIT)
+		return got_error_fmt(GOT_ERR_OBJ_TYPE,
+		    "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);
+	if (err)
+		return err;
+	if (yca_id == NULL)
+		return got_error_fmt(GOT_ERR_SEND_ANCESTRY, "%s", refname);
+
+	free(yca_id);
+	return err;
+}
+
+static const struct got_error *
 realloc_ids(struct got_object_id ***ids, size_t *nalloc, size_t n)
 {
 	struct got_object_id **new;
@@ -349,6 +376,7 @@ got_send_pack(const char *remote_name, struct got_path
 	uint8_t packsha1[SHA1_DIGEST_LENGTH];
 	int packfd = -1;
 	FILE *delta_cache = NULL;
+	struct got_commit_object *commit = NULL;
 
 	TAILQ_INIT(&refs);
 	TAILQ_INIT(&have_refs);
@@ -581,8 +609,20 @@ 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 = got_object_open_as_commit(&commit, repo,
+				    my_id);
+				if (err)
+					goto done;
+				if (got_object_commit_get_nparents(commit) > 1)
+					err = check_common_ancestry(refname,
+					    my_id, their_id, repo,
+					    cancel_cb, cancel_arg);
+				else 
+					err = check_linear_ancestry(refname,
+					    my_id, their_id, repo,
+					    cancel_cb, cancel_arg);
+				got_object_commit_close(commit);
+				commit = NULL;
 				free(my_id);
 				my_id = NULL;
 				if (err)
@@ -730,6 +770,8 @@ done:
 	if (npackfd != -1 && close(npackfd) == -1 && err == NULL)
 		err = got_error_from_errno("close");
 
+	if (commit)
+		got_object_commit_close(commit);
 	got_ref_list_free(&refs);
 	got_pathlist_free(&have_refs, GOT_PATHLIST_FREE_NONE);
 	got_pathlist_free(&their_refs, GOT_PATHLIST_FREE_NONE);
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