From: Stefan Sperling Subject: Re: three bugs related to merge commits To: James Cook Cc: Mark Jamsek , gameoftrees@openbsd.org Date: Mon, 17 Apr 2023 19:30:05 +0200 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