Re: [PATCH v5 1/9] Start to implement a built-in version of `git add --interactive`

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 11, 2019, 09:15 UTC
Message-ID: <nycvar.QRO.7.76.6.1911110949590.46@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqqtv7cjj2n.fsf@gitster-ct.c.googlers.com>

Hi Junio,

On Sun, 10 Nov 2019, Junio C Hamano wrote:

Show 33 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > I reworded the commit message:
> >
> >     Start to implement a built-in version of `git add --interactive`
> >
> >     To convert the interactive `add` to C, we start with a bare-bones
> >     version of the built-in interactive add, guarded by the new
> >     `add.interactive.useBuiltin` config variable, and then add more and more
> >     functionality to it, until it is feature complete.
> >
> >     This is in contrast to previous conversions to C, where we started with
> >     a built-in helper that spawns the script by default, but optionally
> >     executes the C code instead. The sole reason for this deviation from
> >     previous practice is that on Windows (where such a conversion has the
> >     most benefits in terms of speed and robustness) we face the very
> >     specific problem that a `system()` call in Perl seems to close `stdin`
> >     in the parent process when the spawned process consumes even one
> >     character from `stdin`. And that just does not work for us here, as it
> >     would stop the main loop as soon as any interactive command was
> >     performed by the helper. Which is almost all of the commands in `git add
> >     -i`.
> >
> >     It is almost as if Perl told us once again that it does not want us to
> >     use it on Windows.
> >
> >     At this point, the built-in version of `git add -i` only states that it
> >     cannot do anything yet ;-)
> >
> > Hopefully you like this one better?
>
> Not really.  I find the "we could do the other way but we don't, and
> I hate Perl" totally irrelevant and misleading.

Okay, I understand that you take exception at my criticism of Git's use of Perl, and I fully understand that you think I blame you for it because you added most of it.

And I agree that this sidetrack is totally irrelevant for the patch under discussion.

I do think, however, that the discussion of "we wanted to do it the other way, but when we tried, it did not work" is relevant, even if I shortened it to "we use a different approach than previous conversions, because that previous approach would not work".

Truth be told: I would have _much rather_ stayed with the previous `--helper` approach, as that would have made it possible to have a passing test suite at every step, with and without `GIT_TEST_ADD_I_USE_BUILTIN=true`.

The "let GIT_TEST_ADD_I_USE_BUILTIN=true use the built-in even for functions we _know_ are not implemented" way only gives us the full comfort of a passing test suite at the very end of all six patch series, if which the patch series we are currently discussing is merely the first.

If I was a reviewer of this patch series rather than the sender, I would be a bit uncomfortable with the fact that `GIT_TEST_ADD_I_USE_BUILTIN` cannot be added to the CI/PR builds' `linux-gcc` over-job, not until much, much later. In fact, it can only be added as the very last patch in the very last of the six patch series.

And as I write this, I realize that I never spelled that out in the commit message, and it is a rather important point for reviewers to see addressed pre-emptively, in my opinion.

Therefore I revised the commit message again:

    Start to implement a built-in version of `git add --interactive`

    Unlike previous conversions to C, where we started with a built-in
    helper, we start this conversion by adding an interception in the
    `run_add_interactive()` function when the new opt-in
    `add.interactive.useBuiltin` config knob is turned on (or the
    corresponding environment variable `GIT_TEST_ADD_I_USE_BUILTIN`), and
    calling the new internal API function `run_add_i()` that is implemented
    directly in libgit.a.

    At this point, the built-in version of `git add -i` only states that it
    cannot do anything yet. In subsequent patches/patch series, the
    `run_add_i()` function will gain more and more functionality, until it
    is feature complete. The whole arc of the conversion can be found in the
    PRs #170-175 at https://github.com/gitgitgadget/git.

    The "--helper approach" can unfortunately not be used here: on Windows
    we face the very specific problem that a `system()` call in
    Perl seems to close `stdin` in the parent process when the spawned
    process consumes even one character from `stdin`. Which prevents us from
    implementing the main loop in C and still trying to hand off to the Perl
    script.

    The very real downside of the approach we have to take here is that the
    test suite won't pass with `GIT_TEST_ADD_I_USE_BUILTIN=true` until the
    conversion is complete (the `--helper` approach would have let it pass,
    even at each of the incremental conversion steps).

Any suggestions how to improve that commit message?

Ciao, Dscho