Re: [PATCH v3 3/7] merge: do not abort early if one strategy fails to handle the merge

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Jul 21 2022, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <newren@gmail.com>
>
> builtin/merge is setup to allow multiple strategies to be specified,
> and it will find the "best" result and use it.  This is defeated if
> some of the merge strategies abort early when they cannot handle the
> merge.  Fix the logic that calls recursive and ort to not do such an
> early abort, but instead return "2" or "unhandled" so that the next
> strategy can try to handle the merge.
>
> Coming up with a testcase for this is somewhat difficult, since
> recursive and ort both handle nearly any two-headed merge (there is
> a separate code path that checks for non-two-headed merges and
> already returns "2" for them).  So use a somewhat synthetic testcase
> of having the index not match HEAD before the merge starts, since all
> merge strategies will abort for that.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  builtin/merge.c                          |  6 ++++--
>  t/t6402-merge-rename.sh                  |  2 +-
>  t/t6424-merge-unrelated-index-changes.sh | 16 ++++++++++++++++
>  t/t6439-merge-co-error-msgs.sh           |  1 +
>  4 files changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 13884b8e836..dec7375bf2a 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -754,8 +754,10 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
>  		else
>  			clean = merge_recursive(&o, head, remoteheads->item,
>  						reversed, &result);
> -		if (clean < 0)
> -			exit(128);
> +		if (clean < 0) {
> +			rollback_lock_file(&lock);
> +			return 2;
> +		}
>  		if (write_locked_index(&the_index, &lock,
>  				       COMMIT_LOCK | SKIP_IF_UNCHANGED))
>  			die(_("unable to write %s"), get_index_file());
> diff --git a/t/t6402-merge-rename.sh b/t/t6402-merge-rename.sh
> index 3a32b1a45cf..772238e582c 100755
> --- a/t/t6402-merge-rename.sh
> +++ b/t/t6402-merge-rename.sh
> @@ -210,7 +210,7 @@ test_expect_success 'updated working tree file should prevent the merge' '
>  	echo >>M one line addition &&
>  	cat M >M.saved &&
>  	git update-index M &&
> -	test_expect_code 128 git pull --no-rebase . yellow &&
> +	test_expect_code 2 git pull --no-rebase . yellow &&
>  	test_cmp M M.saved &&
>  	rm -f M.saved
>  '
> diff --git a/t/t6424-merge-unrelated-index-changes.sh b/t/t6424-merge-unrelated-index-changes.sh
> index f35d3182b86..8b749e19083 100755
> --- a/t/t6424-merge-unrelated-index-changes.sh
> +++ b/t/t6424-merge-unrelated-index-changes.sh
> @@ -268,4 +268,20 @@ test_expect_success 'subtree' '
>  	test_path_is_missing .git/MERGE_HEAD
>  '
>  
> +test_expect_success 'resolve && recursive && ort' '
> +	git reset --hard &&
> +	git checkout B^0 &&
> +
> +	test_seq 0 10 >a &&
> +	git add a &&
> +
> +	sane_unset GIT_TEST_MERGE_ALGORITHM &&
> +	test_must_fail git merge -s resolve -s recursive -s ort C^0 >output 2>&1 &&
> +
> +	grep "Trying merge strategy resolve..." output &&
> +	grep "Trying merge strategy recursive..." output &&
> +	grep "Trying merge strategy ort..." output &&
> +	grep "No merge strategy handled the merge." output
> +'
> +
>  test_done
> diff --git a/t/t6439-merge-co-error-msgs.sh b/t/t6439-merge-co-error-msgs.sh
> index 5bfb027099a..52cf0c87690 100755
> --- a/t/t6439-merge-co-error-msgs.sh
> +++ b/t/t6439-merge-co-error-msgs.sh
> @@ -47,6 +47,7 @@ test_expect_success 'untracked files overwritten by merge (fast and non-fast for
>  		export GIT_MERGE_VERBOSITY &&
>  		test_must_fail git merge branch 2>out2
>  	) &&
> +	echo "Merge with strategy ${GIT_TEST_MERGE_ALGORITHM:-ort} failed." >>expect &&
>  	test_cmp out2 expect &&
>  	git reset --hard HEAD^
>  '

I'm re-rolling ab/leak-check, and came up with the below (at the very
end) to "fix" a report in builtin/merge.c, reading your commit message
your fix seems obviously better.

Mine's early WIP, and I e.g. didn't notice that I forgot to unlock the
&lock file, which is correct.

I *could* say "that's not my problem", i.e. we didn't unlock it before
(we rely on atexit). The truth is I just missed it, but having said that
it *is* true that we could do without it, or do it as a separate chaneg.

I'm just posting my version below to help move yours forward, i.e. to
show that someone else has carefully at least this part.

But it is worth noting from staring at the two that your version is
mixing several different behavior changes into one, which *could* be
split up (but whether you think that's worth it I leave to you).

Maybe I'm the only one initially confused by it, and that's probably
just from being mentally biased towards my own "solution". Those are (at
least):

 1. Before we didn't explicitly unlock() before exit(), but had atexit()
    do it, that could be a one-line first commit. This change is
    obviously good.

 2. A commit like mine could come next, i.e. we bug-for-bug do what we
    do do now, but just run the "post-builtin" logic when we return from
    cmd_merge().

    Doing it as an in-between would be some churn, as we'll need to get
    rid of "early_exit" again, but would allow us to incrementally move
    forward to...

 3. ...then we'd say "but it actually makes sense not to early abort",
     i.e. you want to change this so that we'll run the logic between
     try_merge_strategy() exiting with 128 now and the return from
     cmd_merge().

     This bit is my main sticking point in reviewing your change,
     i.e. your "a testcase for this is somewhat difficult" somewhat
     addresses this, but (and maybe I'm wrong) it seems to me that 

     Editing that code the post-image looks like this, with my
     commentary & most of the code removed, i.e. just focusing on the
     branches we do and don't potentially have tests for:

     		/* Before this we fall through from ret == 128 (or ret == 2...) */
		if (automerge_was_ok) { // not tested?
		if (!best_strategy) {
			// we test this...
			if (use_strategies_nr > 1)
				// And this: _("No merge strategy handled the merge.\n"));
			else
				// And this: _("Merge with strategy %s failed.\n"),
		} else if (best_strategy == wt_strategy)
			// but not this?
		else
			// Or this, where we e.g. say "Rewinding the tree to pristene..."?
	
		if (squash) {
			// this?
		} else
			// this? (probably, yes)
			write_merge_state(remoteheads);
	
		if (merge_was_ok)
			// this? (probably, yes, we just don't grep it?)
		else
			// this? maybe yes because it's covered by the
			// "failed" above too?
			ret = suggest_conflicts();
	
	done:
		if (!automerge_was_ok) {
			// this? ditto the first "not tested?"
		}

   I.e. are you confident that we want to continue now in these various
   cases, where we have squash, !automerge_was_ok etc. I think it would
   be really useful to comment on (perhaps by amending the above
   pseudocode) what test cases we're not testing / test already etc.

 4. Having done all that (or maybe this can't be split up / needs to
    come earlier) you say that we'd like to not generically call this
    exit state 128, but have it under the "exit(2)" umbrella.

Again, all just food for thought, and a way to step-by-step go through
how I came about reviewing this in detail, I hope it and the below
version I came up with before seeing yours helps.

P.s.: The last paragraph in my commit message does not point to some
hidden edge case in the code behavior here, it's just that clang/gcc are
funny about exit() and die() control flow when combined with
-fsanitize=address and higher optimization levels.

-- >8 --
Subject: [PATCH] merge: return, don't use exit()

Change some of the builtin/merge.c code added in f241ff0d0a9 (prepare
the builtins for a libified merge_recursive(), 2016-07-26) to ferry up
an "early return" state, rather than having try_merge_strategy() call
exit() itself.

This is a follow-up to dda31145d79 (Merge branch
'ab/usage-die-message' into gc/branch-recurse-submodules-fix,
2022-03-31).

The only behavior change here is that we'll now properly catch other
issues on our way out, see e.g. [1] and the interaction with /dev/full
for an example.

The immediate reason to do this change is because it's one of the
cases where clang and gcc's SANITIZE=leak behavior differs. Under
clang we don't detect that "t/t6415-merge-dir-to-symlink.sh" triggers
a leak, but gcc spots it.

1. https://lore.kernel.org/git/87im2n3gje.fsf@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/merge.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 23170f2d2a6..a8d5d04f622 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -709,10 +709,12 @@ static void write_tree_trivial(struct object_id *oid)
 
 static int try_merge_strategy(const char *strategy, struct commit_list *common,
 			      struct commit_list *remoteheads,
-			      struct commit *head)
+			      struct commit *head, int *early_exit)
 {
 	const char *head_arg = "HEAD";
 
+	*early_exit = 0;
+
 	if (refresh_and_write_cache(REFRESH_QUIET, SKIP_IF_UNCHANGED, 0) < 0)
 		return error(_("Unable to write index."));
 
@@ -754,8 +756,10 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 		else
 			clean = merge_recursive(&o, head, remoteheads->item,
 						reversed, &result);
-		if (clean < 0)
-			exit(128);
+		if (clean < 0) {
+			*early_exit = 1;
+			return 128;
+		}
 		if (write_locked_index(&the_index, &lock,
 				       COMMIT_LOCK | SKIP_IF_UNCHANGED))
 			die(_("unable to write %s"), get_index_file());
@@ -1665,6 +1669,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 
 	for (i = 0; !merge_was_ok && i < use_strategies_nr; i++) {
 		int ret, cnt;
+		int early_exit;
+
 		if (i) {
 			printf(_("Rewinding the tree to pristine...\n"));
 			restore_state(&head_commit->object.oid, &stash);
@@ -1680,7 +1686,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 
 		ret = try_merge_strategy(use_strategies[i]->name,
 					 common, remoteheads,
-					 head_commit);
+					 head_commit, &early_exit);
+		if (early_exit)
+			goto done;
+
 		/*
 		 * The backend exits with 1 when conflicts are
 		 * left to be resolved, with 2 when it does not
@@ -1732,12 +1741,18 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	} else if (best_strategy == wt_strategy)
 		; /* We already have its result in the working tree. */
 	else {
+		int new_ret, early_exit;
+
 		printf(_("Rewinding the tree to pristine...\n"));
 		restore_state(&head_commit->object.oid, &stash);
 		printf(_("Using the %s strategy to prepare resolving by hand.\n"),
 			best_strategy);
-		try_merge_strategy(best_strategy, common, remoteheads,
-				   head_commit);
+		new_ret = try_merge_strategy(best_strategy, common, remoteheads,
+					     head_commit, &early_exit);
+		if (early_exit) {
+			ret = new_ret;
+			goto done;
+		}
 	}
 
 	if (squash) {
-- 
2.36.1