Re: [PATCH v8 08/14] merge-resolve: rewrite in C

From: Elijah Newren <newren@gmail.com>
Date: Aug 17, 2022, 19:06 UTC
Message-ID: <CABPp-BGSFYWvA5HktLf33=w7JB95iDLDNoE0gdA3oUtb+qYoQQ@mail.gmail.com>
In-Reply-To: <848p4p89-2219-7874-ss50-2o0rp4r02902@tzk.qr>

Hi Dscho,

I share some of Junio's concerns, and feel your response is addressing a tangent but not the actual issues. Perhaps I can try to explain why from a slightly different perspective...

On Wed, Aug 17, 2022 at 2:51 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 34 quoted lines

>
> Hi Junio,
>
> On Tue, 16 Aug 2022, Junio C Hamano wrote:
>
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >
> > > I'm all in favor of adding such a good example there, but there is no
> > > reason to hold back `git merge-resolve` from being implemented in C.
> >
> > You did not address the primary point, i.e. why the particular
> > change is a bad one.  Sure, you lost a scripted porcelain or two
> > that are not used much, but in exchange for what?  That is _the_
> > issue and you skirt around it.
>
> In exchange for what I mentioned already in
> https://lore.kernel.org/git/qs23r0n8-9r24-6095-3n9n-9131s69974p1@tzk.qr/,
> i.e. in the part you deleted from the quoted mail:
>
>         We reduce Git's reliance on POSIX shell scripting, we reduce the
>         number of programming languages contributors need to be familiar
>         with, we open up to code coverage/static analysis tools that
>         handle C but not shell scripts, just to name a few.
>
> To reiterate why reducing the reliance on POSIX shell scripting is a good
> thing:
>
> - we pay a steep price in the form of performance issues (you will recall
>   that merely rewriting the `rebase -i` engine in C and nothing else
>   improved the overall run time of p3404 5x on Windows, 4x on macOS and
>   still 3.5x on Linux, see
>   https://lore.kernel.org/git/cover.1483370556.git.johannes.schindelin@gmx.de/)
>
>   Yes, Linux sees such an incredible performance boost. Surprising, right?

Sure, scripts are slow *if* run. Junio asked explicitly about that "if" part, which you seem to be overlooking, and thus you are answering a different question.

Is anyone, anywhere, ever running `-s resolve`? Junio is doubting it, and given that even some Git developers who were unaware of its existence[1], I have to wonder too.

[1] https://public-inbox.org/git/kl6l7d58k535.fsf@chooglen-macbookpro.roam.corp.google.com/, look for "today I learned..."

> - on Windows, even aside from the performance problems (which I deem
>   reason enough on their own to aim for Git being implemented purely in
>   C), users run into issues where anti-malware simply blocks shell
>   scripts, sometimes even quarantines entire parts of Git for Windows.

I'm not sure I'm following. If users do attempt to run `git {merge,rebase,cherry-pick,revert} --strategy resolve`, then anti-malware disables other parts of Git for Windows? Or is the mere presence of git-merge-resolve enough to trigger such problems? If the latter, then I could agree that's really problematic and worth addressing. If the former, we may be back to that all important "if". But I'm not sure it's one of those two; could you clarify a bit here?

Show 6 quoted lines

> - have you ever attempted to debug a Git invocation that involves spawning
>   a shell script that in turn spawns the failing Git command, using `gdb`?
>   I have. It ain't pretty. And you know that there are easier ways to
>   abuse and deter new contributors than to ask them to do the same. In
>   particular when large amounts of data have to be passed between those
>   processes, typically via `stdio`.

Yes, that's very painful. It's annoyed me many times. It's a problem, *if* you need to debug a script. But again, you seem to be presuming that git-merge-resolve is in use, which dodges the very question Junio was asking. Is it in use?

Show 12 quoted lines

> - show me the equivalent of CodeQL/Coverity for POSIX shell scripting? ;-)
>
> - portability issues dictate that we're not just using your grand father's
>   POSIX shell scripting, but that we limit it to a subset that is opaque
>   to developers unfamiliar with Git project.
>
> - as a consequence, our shell scripts are highly opinionated, often using
>   unintuitive idioms such as `&&` chains instead of `set -e`, which makes
>   them unsuitable as examples how to script Git for regular users.
>
> - a decreasing number of software developers is familiar with the
>   intricacies of that language, leaving us with tech debt.

Yes, these are real issues for code written in shell being actively developed and maintained, yes. (shellcheck might help as a CodeQL/Coverity-like thing for shell.)

However, that doesn't really apply here. There have literally only been two commits to git-merge-resolve.sh in the last decade, one from me that copied a few lines verbatim from git-merge-octopus.sh, and the other was a single character change 5 years ago.

> In short, there is not a single shred of doubt in my mind that avoiding
> shell scripted parts in Git is a really good goal to have for this
> project.

I think it's a good goal in general, especially for anything heavily used. I share Junio's concern about this one in particular. I'm not sure this script is even being used directly, the maintenance burden for it is essentially zero, and the script does have both educational and testing value.

Show 6 quoted lines

> > The series makes us lose all strategies that are actively tested
> > that are spawned as a subprocess, which is the way all third-party
> > strategies will be used.
>
> Then have that even-simpler-than `git-merge-resolve.sh` example be tested
> as part of the test suite. That's what the test suite is for.

That simpler thing being a resurrection of git-merge-ours.sh from a00a42ae33708caa742d9e9fbf10692cfa42f032^ ?

That would test that we shell out to another strategy. But it wouldn't really test as many of the cases in builtin/merge.c for dealing with external strategies. `-s resolve` can fail on "interesting" changes, after making changes to the working tree and index, and builtin/merge.c is expected to handle that -- using a simpler example would lose that important testing. (I kinda think it's a bug that it doesn't clean up after itself and that we made builtin/merge.c do the cleanup, but backward compatibility suggests we at least need some way to keep testing that we handle that.) We would also need to be careful about testing the "preferred" strategy when the user asks for multiple strategies, another thing covered in our testsuite (though using two builtins might be good enough for that). I'd have to look over the testsuite to check and see if there are other important properties being tested too; -s resolve has been used in a few dozen places.

Show 9 quoted lines

> > After this, we have less test coverage of the codepaths we care about,
> > which is *not* a scripted "resolve" strategy, but the code that runs
> > third-party strategies as externals.
>
> It is better to leave the responsibility of test coverage to the test
> suite, avoiding to ship the corresponding support code to users.
>
> tl;dr your concerns are easy to address, without having to incur the price
> of keeping parts of Git implemented in shell.

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

Show 7 quoted lines

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

Show 6 quoted lines

> I would also have understood a lament about the absence of any good
> example in https://git-scm.com/docs/git-merge#_merge_strategies to help
> users develop their own custom merge strategies.
>
> I'm all in favor of adding such a good example there, but there is no
> reason to hold back `git merge-resolve` from being implemented in C.

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.