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

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

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

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

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