On Thu, Aug 18, 2022 at 7:55 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

On Thu, Aug 18 2022, Elijah Newren wrote:

quoted

On Thu, Aug 18, 2022 at 7:42 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

quoted

On Wed, Aug 17 2022, Junio C Hamano wrote:

quoted

Elijah Newren [off-list ref] writes:

quoted

There's also another concern you tried to address in your other email;
let me quote from that email here:

quoted

If you want to have an easy example of a custom merge strategy, then let's
have that easy example. `git-merge-resolve.sh` ain't that example.

It would be a different matter if you had commented about
`git-merge-ours.sh`:
https://github.com/git/git/blob/v2.17.0/contrib/examples/git-merge-ours.sh
That _was_ a simple and easy example.

...and it was _utterly useless_ as an example.  It only checked that
the user hadn't modified the index since HEAD.  It doesn't demonstrate
anything about how to merge differing entries, since that merge
strategy specifically ignores changes made on the other side.  Since
merging differing entries is the whole point of writing a strategy, I
see no educational value in that particular script.

`git-merge-resolve.sh` may be an imperfect example, but it's certainly
far superior to that.
...
If someone makes a better example (which I agree could be done,
especially if it added lots of comments about what was required and
why), and ensures we keep useful test coverage (maybe using Junio's
c-resolve suggestion in another email), then my concerns about
reimplementing git-merge-resolve.sh in C go away.

If that happens, then I still think it's a useless exercise to do the
reimplementation -- unless someone can provide evidence of `-s
resolve` being in use -- but it's not a harmful exercise and wouldn't
concern me.

If the better example and mechanism to retain good test coverage
aren't provided, then I worry that reimplementing is a bunch of work
for an at best theoretical benefit, coupled with a double whammy
practical regression.

Ah, you said many things I wanted to say already.  Thanks.

I may have missed something in this thread, but wouldn't an acceptable
way to please everyone here be to:

 1. Have git's behavior be that of the end of this series...
 2. Add a GIT_TEST_* mode where we'll optionally invoke these "built-in"
    merge strategies as commands, i.e. have them fall back to
    "try_merge_command()".

In the portion of the email you quoted and responded to, most of the
text was talking about how git-merge-resolve.sh serves an important
educational purpose, yet you've only tried to address the testing
issue.  I think both are important.

*Nod*, I meant (but should have said) "on the topic of the test
 coverage"...

Ah, yes, that would have helped.  :-)

quoted

The easiest way to fix the
educational shortcoming of this series is to reverse the deleting of
git-merge-resolve.sh, and restore the building and distribution of
git-merge-resolve from that script.  Unfortunately, that generates a
collision between both the script and the builtin being used to build
the same file (namely, git-merge-resolve)...

I'd think if we were shipping it as an example we could give it a
different name, or not install it as an executable, but in the "shared"
part (along with the README etc.).

Seems reasonable; I'm slightly partial to the name
"git-merge-strategy-demo" (though "--strategy strategy-demo" might
look weird), or perhaps just "git-merge-demo" (though that makes
people wonder what kind of demo).

Or keep it in-tree in contrib, but we did try that sort of thing before
with 49eb8d39c78 (Remove contrib/examples/*, 2018-03-25) :)

I think the best way forward is to just note in the documentation some
examples of how to write a merge driver, either by linking to an older
version of the script, or quoting it inline.

Nitpick: "merge strategy", not "merge driver".

A merge driver is something defined in .gitattributes and only ever
functions on three versions of one file, never having bigger knowledge
of the wider tree.  A merge driver is thus a special purpose three-way
content merge of a single file (replacing the normal xdiff merge
stuff) tailored to a specific file type.

A merge strategy, in contrast, is given multiple commits to merge and
thus has a view of the whole tree.  A merge strategy needs to decide
whether and how to handle directory/file conflicts, differing modes,
submodule updates, recursive ancestor consolidation, file renames
(including weird cases like colliding renames or renamed differently),
directory renames, etc.  A merge strategy may well call various merge
drivers (assuming some are defined in .gitattributes) for different
paths within the tree, and/or fall back to calling (directly or
indirectly) the code in xdiff to handle the three-way merge of
individual files.

quoted

which is yet another
reason that the easiest solution available here is to just not rewrite
this script in C at all.

I think there's bigger benefits to moving more things to C & built-ins,
so I'd prefer to see some version of this where what we do by default is
to call this C code (or similar), and not as a sub-process.

Yes, in general I agree there are big benefits to moving towards C &
built-ins.  I'm unconvinced any of them apply in the specific case of
merge-resolve, as noted at length earlier in this thread.

If someone wants to do it anyway, they should just make sure that (1)
testing of external merge strategies doesn't regress and remains well
tested, and (2) there is a good story for educating users about how to
write external merge strategies, or at least as good as what we have
now.  If I feel either is being ignored or regressing, I'll likely
express my concerns again.

quoted

There are certainly other possible solutions to the educational issue,
and might not even be too hard, but we'd need someone to implement one
before I'd agree we found an "acceptable way to please everyone".  :-)

*nod*

quoted

quoted

So something like this on top of this series (assume my SOB etc. if this
is acceptable). I only tested this locally, but it seems to do the right
thing for me:

<snip patch>

How did you test?  I'm a bit confused...unless I'm misreading
something, it appears to me that ci/lib.sh sets SKIP_DASHED_BUILT_INS
unconditionally which would probably cause your proposal to break.

Admittedly not very thoroughly, but I'm fairly sure it does the right
thing when it comes to this, and SKIP_DASHED_BUILT_INS doesn't enter
into it (and all my local builds use SKIP_DASHED_BUILT_INS=Y).

The try_merge_command() invokes merge-what-ever, and does a
run_command_v_opt(args.v, RUN_GIT_CMD). At that point we'll invoke a
"git merge-what-ever", i.e. we don't need a "git-merge-what-ever" binary
to exist.

This is what we do in general when git is invoking itself, and we'd need
to go out of our way to have it not work in this case (i.e. build it as
a stand-alone program, like git-http-fetch, and not as a built-in).

Oh, right, I was mixing up git-merge-one-file (which merge-resolve has
merge-index invoke, and yes including the dash right after "git") and
`git merge-resolve`.  Sorry about that.