Re: [PATCH v4 2/7] merge-resolve: abort if index does not match HEAD

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

On Mon, Jul 25 2022, Elijah Newren wrote:

> On Fri, Jul 22, 2022 at 10:53 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>> On Fri, Jul 22 2022, Elijah Newren wrote:
>>
> [...]
>> > So, ignoring the return code from diff-index is correct behavior here.
>> >
>> > Were you thinking this was a test script or something?
>>
>> We can leave this for now.
>>
>> But no. Whatever the merge driver is documenting as its normal return
>> values we really should be ferrying up abort() and segfault, per the
>> "why do we miss..." in:
>> https://lore.kernel.org/git/patch-v2-11.14-8cc6ab390db-20220720T211221Z-avarab@gmail.com/
>>
>> I.e. this is one of the cases in the test suite where we haven't closed
>> that gap, and could hide segfaults as a "normal" exit 2.
>>
>> So I think your v5 is fine as-is, but in general I'd be really
>> interested if you want to double-down on this view for the merge drivers
>> for some reason, because my current plan for addressing these blindspots
>> outlined in the above wouldn't work then...
>
> Quoting from there:
>
>> * We have in-tree shellscripts like "git-merge-one-file.sh" invoking
>>   git commands, they'll usually return their own exit codes on "git"
>>   failure, rather then ferrying up segfault or abort() exit code.
>>
>>   E.g. these invocations in git-merge-one-file.sh leak, but aren't
>>   reflected in the "git merge" exit code:
>>
>>src1=$(git unpack-file $2)
>>src2=$(git unpack-file $3)
>>
>>   That case would be easily "fixed" by adding a line like this after
>>   each assignment:
>>
>>test $? -ne 0 && exit $?
>>
>>   But we'd then in e.g. "t6407-merge-binary.sh" run into
>>   write_tree_trivial() in "builtin/merge.c" calling die() instead of
>>   ferrying up the relevant exit code.
>
> Sidenote, but I don't think t6407-merge-binary.sh calls into
> write_tree_trivial().  Doesn't in my testing, anyway.

I haven't gone back & looked at that, but I vaguely recall that if you
"get past" that one it was returning 128 somewhere, either directly or
indirectly.

In any case, for the purposes of that summary it's a common pattern
elsewhere...

> Are you really planning on auditing every line of git-bisect.sh,
> git-merge*.sh, git-sh-setup.sh, git-submodule.sh, git-web--browse.sh,
> and others, and munging every area that invokes git to check the exit
> status?

Not really, but just for that change explaining why it's required to
have this log-on-the-side to munge the exit code.

Although I think you might have not kept up with just how close we are
to "git rm"-ing most of our non-trivial amounts of
shellscript. git-{submodule,bisect}.sh is going away, hopefully in this
release.

*If* we go for that approach I'd think it would be relatively easy to
 add a helper to git-sh-setup.sh to wrap the various "git" callse.

>   Yuck.  A few points:
>   * Will this really buy you anything?  Don't we have other regression
> tests of all these commands (e.g. "git unpack-file") which ought to
> show the same memory leaks?  This seems like high lift, low value to
> me, and just fixing direct invocations in the regression tests is
> where the value comes.

It's a long-tail problem, and we don't need to fix it all now. I'm just
commenting on it here because there's an *addition* of a hidden exit
code, and more importantly I wanted to clear up if you thought ferrying
up abort() or segfaults wouldn't be desired in those cases.

>  (If direct coverage is lacking in the
> regression tests, shouldn't the solution be to add coverage?)


But how do we find out what we're covering? Yes "make coverage", but
that's just going to give you all C lines we "visit", but you don't know
if those lines were visited by parts of our test suite where we're
checking the exit code.

>   * Won't this be a huge review and support burden to maintain the
> extra checking?

I think the end result mostly makes things easier to deal with &
maintaine, e.g. consistently using &&-chaining, or test_cmp instead of
"test "$(...)" etc.

>   * Some of these scripts, such as git-merge-resolve.sh and
> git-merge-octopus.sh are used as examples of e.g. merge drivers, and
> invasive checks whose sole purpose is memory leak checking seems to
> run counter to the purpose of being a simple example for users

I'm not doing any of this now, but I'd think we could do something like
this (i.e. new helpers in git-sh-setup.sh):
	
	diff --git a/git-merge-resolve.sh b/git-merge-resolve.sh
	index 343fe7bccd0..f23344dfec8 100755
	--- a/git-merge-resolve.sh
	+++ b/git-merge-resolve.sh
	@@ -37,15 +37,16 @@ then
	 	exit 2
	 fi
	 
	-git update-index -q --refresh
	-git read-tree -u -m --aggressive $bases $head $remotes || exit 2
	+run_git git update-index -q --refresh
	+run_git --exit-on-failure 2 git read-tree -u -m --aggressive $bases $head $remotes
	 echo "Trying simple merge."
	-if result_tree=$(git write-tree 2>/dev/null)
	+result_tree=$(git write-tree 2>/dev/null)
	+if check_last_git_cmd $?
	 then
	 	exit 0
	 else
	 	echo "Simple merge failed, trying Automatic merge."
	-	if git merge-index -o git-merge-one-file -a
	+	if run_git git merge-index -o git-merge-one-file -a
	 	then
	 		exit 0
	 	else

>   * Wouldn't using errexit and pipefail be an easier way to accomplish
> checking the exit status (avoiding the problems from the last few
> bullets)?  You'd still have to audit the code and write e.g.
> shutupgrep wrappers (since grep reports whether it found certain
> patterns in the input, rather than whether it was able to perform the
> search on the input, and we often only care about the latter), but it
> at least would automatically check future git invocations.

Somewhat, but unless we're going to depend on "bash" I think we can at
most use those during development to locate various issues, e.g. as I
did in this series:
https://lore.kernel.org/git/20210123130046.21975-1-avarab@gmail.com/

>   * Are we running the risk of overloading special return codes (e.g.
> 125 in git-bisect)

No, we already deal with that as a potential problem in test_must_fail
in the test suite, i.e. we just catch abort(), segfaults & the like. In
bourn shells those are 134.

> I do still think that "2" is the correct return code for the
> shell-script merge strategies here, though I think it's feasible in
> their cases to change the documentation to relax the return code
> requirements in such a way to allow those scripts to utilize errexit
> and pipefail.

I think for any such interface it makes sense to exit with 0, 1 and 2 or
whatever during normal circumstances.

But if the program you just called segfaulted I think it makes sense to
treat that as an exception. I.e. it's not just that the merge failed,
but we should really abort() in the calling program too..

Anyway, this is all quite academic at this point, but since you asked...