Subject: Re: [PATCH v8 08/14] merge-resolve: rewrite in C

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Thu, 18 Aug 2022 16:24:38 +0200

On Wed, Aug 17 2022, Junio C Hamano wrote:

> Elijah Newren <[email-masked]> writes:
>
>> There's also another concern you tried to address in your other email;
>> let me quote from that email here:
>>
>>> If you want to have an easy example of a custom merge strategy, then let's
>>> have that easy example. `git-merge-resolve.sh` ain't that example.
>>>
>>> It would be a different matter if you had commented about
>>> `git-merge-ours.sh`:
>>> https://github.com/git/git/blob/v2.17.0/contrib/examples/git-merge-ours.sh
>>> That _was_ a simple and easy example.
>>
>> ...and it was _utterly useless_ as an example.  It only checked that
>> the user hadn't modified the index since HEAD.  It doesn't demonstrate
>> anything about how to merge differing entries, since that merge
>> strategy specifically ignores changes made on the other side.  Since
>> merging differing entries is the whole point of writing a strategy, I
>> see no educational value in that particular script.
>>
>> `git-merge-resolve.sh` may be an imperfect example, but it's certainly
>> far superior to that.
>> ...
>> If someone makes a better example (which I agree could be done,
>> especially if it added lots of comments about what was required and
>> why), and ensures we keep useful test coverage (maybe using Junio's
>> c-resolve suggestion in another email), then my concerns about
>> reimplementing git-merge-resolve.sh in C go away.
>>
>> If that happens, then I still think it's a useless exercise to do the
>> reimplementation -- unless someone can provide evidence of `-s
>> resolve` being in use -- but it's not a harmful exercise and wouldn't
>> concern me.
>>
>> If the better example and mechanism to retain good test coverage
>> aren't provided, then I worry that reimplementing is a bunch of work
>> for an at best theoretical benefit, coupled with a double whammy
>> practical regression.
>
> Ah, you said many things I wanted to say already.  Thanks.

I may have missed something in this thread, but wouldn't an acceptable
way to please everyone here be to:

 1. Have git's behavior be that of the end of this series...
 2. Add a GIT_TEST_* mode where we'll optionally invoke these "built-in"
    merge strategies as commands, i.e. have them fall back to
    "try_merge_command()".

So something like this on top of this series (assume my SOB etc. if this
is acceptable). I only tested this locally, but it seems to do the right
thing for me:

diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index 8ebff425967..9d0f68b8147 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -30,6 +30,7 @@ linux-TEST-vars)
 	export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master
 	export GIT_TEST_WRITE_REV_INDEX=1
 	export GIT_TEST_CHECKOUT_WORKERS=2
+	export GIT_TEST_MERGE_COMMANDS=true
 	;;
 linux-clang)
 	export GIT_TEST_DEFAULT_HASH=sha1
diff --git a/sequencer.c b/sequencer.c
index 00a36205848..91d651f9b12 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2309,6 +2309,7 @@ static int do_pick_commit(struct repository *r,
 	} else {
 		struct commit_list *common = NULL;
 		struct commit_list *remotes = NULL;
+		const int test_commands = git_env_bool("GIT_TEST_MERGE_COMMANDS", 0);
 
 		res = write_message(msgbuf.buf, msgbuf.len,
 				    git_path_merge_msg(r), 0);
@@ -2316,10 +2317,10 @@ static int do_pick_commit(struct repository *r,
 		commit_list_insert(base, &common);
 		commit_list_insert(next, &remotes);
 
-		if (!strcmp(opts->strategy, "resolve")) {
+		if (!test_commands && !strcmp(opts->strategy, "resolve")) {
 			repo_read_index(r);
 			res |= merge_strategies_resolve(r, common, oid_to_hex(&head), remotes);
-		} else if (!strcmp(opts->strategy, "octopus")) {
+		} else if (!test_commands && !strcmp(opts->strategy, "octopus")) {
 			repo_read_index(r);
 			res |= merge_strategies_octopus(r, common, oid_to_hex(&head), remotes);
 		} else {