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

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

On Fri, Jul 22 2022, Elijah Newren wrote:

> On Fri, Jul 22, 2022 at 3:46 AM Ævar Arnfjörð Bjarmason
> [...]
>> I don't know enough about the context here, but given our *.sh->C
>> migration elsewhere it's a bit unfortunate to see more *.sh code added
>> back.
>
> This seems like a curious objection.  "We are trying to get rid of
> shell scripts, so don't even fix bugs in any of the existing ones." ?
>
>> We have "git merge" driving this, isn't it OK to have it make this
>> check before invoking "resolve" (may be a stupid question).
>
> Ah, I can kind of see where you're coming from now, but that seems to
> me to be bending over backwards in attempting to fix a component
> written in shell without actually modifying the shell.
> builtin/merge.c is some glue code that can call multiple different
> strategies, but isn't the place for the implementation of the
> strategies themselves, and I'd hate to see us put half the
> implementation in one place and half in another.  In addition, besides
> the separation of concerns issue::
>
>    * We document that users can add their own merge strategies (a
> shell or executable named git-merge-$USERNAME and "git merge -s
> $USERNAME" will call them)
>    * git-merge-resolve and git-merge-octopus serve as examples
>    * Our examples should demonstrate correct behavior and perform
> documented, required steps.  This particular check is important:
>
>     /*
>      * At this point, we need a real merge.  No matter what strategy
>      * we use, it would operate on the index, possibly affecting the
>      * working tree, and when resolved cleanly, have the desired
>      * tree in the index -- this means that the index must be in
>      * sync with the head commit.  The strategies are responsible
>      * to ensure this.
>      */
>
> So, even if someone were to reimplement git-merge-resolve.sh in C, and
> start the deprecation process with some merge.useBuiltinResolve config
> setting (similar to rebase.useBuiltin), I'd still want this shell fix
> added to git-merge-resolve.sh in the meantime, both as an important
> bugfix, and so that people looking for merge strategy examples who
> find this script hopefully find a new enough version with this
> important check included.
>
> In general, if merge strategies do not perform this check, we have
> observed that they often will either (a) discard users' staged changes
> (best case) or (b) smash staged changes into the created commit and
> thus create some kind of evil merge (making it look like they created
> a merge normally, and then amended the merge with additional changes).
>
> We're lucky that the way resolve was implemented, other git calls
> would usually incidentally catch such issues for us without an
> explicit check.  We were also lucky that the observed behavior was
> '(a)' rather than '(b)' for resolve.  But the issue should still be
> fixed.

Makes sense I guess, yeah I was wondering if we could just assume that
"git merge" would always invoke those, and therefore could offload more
of the pre-flight checks over there.

>> For this code in particular it:
>>
>>  * Uses spaces, not tabs
>
> Yes, that's fair, but as I mentioned in the commit message, it was
> copied from git-merge-octopus.sh.  So, as you say below, "so did the
> older version".

*nod*

>>  * We lose the diff-index .. --name-only exit code (segfault), but so
>>    did the older version
>
> Um, I don't understand this objection.  I think you are referring to
> the pipe to sed, but if so...who cares?  The exit code would be lost
> anyway because we aren't running under errexit, and the next line of
> code ignores any and all previous exit codes when it runs "exit 2".
> And if you're not referring to the pipe to sed but the fact that it
> unconditionally returns an exit code of 2 on the next line, then yes
> that is the expected return code.  Whatever the diff-index segfault
> returns would be the wrong exit status and could fool the
> builtin/merge.c into doing the wrong thing.  It expects merge
> strategies to return one of three exit codes: 0, 1, or 2:
>
>     /*
>      * The backend exits with 1 when conflicts are
>      * left to be resolved, with 2 when it does not
>      * handle the given merge at all.
>      */
>
> 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...

 >>  * 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, but it just
seemed to me that you were trying to maintain the "Error" v.s. "error"
distinction, i.e. the C code you're adding uses lower case, the *.sh
upper-case.

Which I see is for consistency with some existing message we have a
translation for, so if that was the main goal (and not bug-for-bug
message compatibility) the above gettext/gettextln use would allow you
to re-use the C i18n.