On Fri, Jul 22 2022, Elijah Newren wrote:

On Fri, Jul 22, 2022 at 3:46 AM Ævar Arnfjörð Bjarmason
[...]

quoted

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

quoted

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.

quoted

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*

quoted

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

 >>  * I wonder if bending over backwards to emit the exact message we

quoted

   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.