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

From: Elijah Newren <newren@gmail.com>
Date: Jul 26, 2022, 01:58 UTC
Message-ID: <CABPp-BE99NVBKRn9Uh3HMcgzV-egtmgnuVJdvT1Rk5VrEKnd4w@mail.gmail.com>
In-Reply-To: <220723.86h738qsr9.gmgdl@evledraar.gmail.com>

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:
>

[...]

Show 18 quoted lines

> > 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:

Show 18 quoted lines

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

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? 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.  (If direct coverage is lacking in the
regression tests, shouldn't the solution be to add coverage?)
  * Won't this be a huge review and support burden to maintain the
extra checking?
  * 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
  * 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.
  * Are we running the risk of overloading special return codes (e.g.
125 in git-bisect)

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.

Show 28 quoted lines

>  >>  * I wonder if bending over backwards to emit the exact message we
> >>    emitted before is worth it
> >>
> >> If you just make this something like (untested):
> >>
> >>         {
> >>                 gettext "error: " &&
> >>                 gettextln "Your local..."
> >>         }
> >>
> >> You could re-use the translation from the *.c one (and the "error: " one
> >> we'll get from usage.c).
> >>
> >> That leaves "\n %s" as the difference, but we could just remove that
> >> from the _() and emit it unconditionally, no?
> >
> > ??
> >
> > Copying a few lines from git-merge-octopus.sh to get the same fix it
> > has is "bending over backwards"?  That's what I call "doing the
> > easiest thing possible" (and which _also_ has the benefit of being
> > battle tested code), and then you describe a bunch of gymnastics as an
> > alternative?  I see your suggestion as running afoul of the objection
> > you are raising, and the code I'm adding as being a solution to that
> > particular objection.  So this particular flag you are raising is
> > confusing to me.
>
> I wasn't aware of some greater context vis-as-vis octopus,

I didn't expect everyone to be, but that's why I put it in the commit message. ;-)