Hi Junio,
On Sun, 10 Nov 2019, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@xxxxxx> 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
