On Mon, Jul 25 2022, Elijah Newren wrote:

On Fri, Jul 22, 2022 at 10:53 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

quoted

On Fri, Jul 22 2022, Elijah Newren wrote:

[...]

quoted

quoted

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

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:

quoted

* 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/ (local)

  * 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...