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

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

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(-)