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

[Junio C Hamano <gitster@pobox.com>]

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.

Unless it is in GSoC or something that wants to avoid a total
failure of nothing to show at the end of the period, in which case a
piecemeal "we did not finish, but at least we have a handful of
subcommands rewritten to show for the consolation prize" might be a
way to have "something" that resembles "working".  But if we value
the quality of the final product over having to have something to
show in a set term (like you as a paid programmer working as a
professional), building piece by piece in the final framework
(i.e. "in C as a builtin") and flipping the "useBuiltin" to turn on
the whole thing at the end would be the preferrable way to do this
kind of thing, I would think.  Also, it is less wasteful, not having
to worry about the inter-language glue code.  Even if the original
is in shell, not in Perl, we have quite a lot of glue code to throw
values back and forth across the language boundary to imitate what
used to be mere assignments to global shell variables between shell
functions and their callers, and always somebody screws up quoting
there ;-)

>> > +	if (use_builtin_add_i == 1 && !patch_mode)
>> > +		return !!run_add_i(the_repository, pathspec);
>> >
>>
>> Strictly speaking, we can bypass the probing of environment and
>> config when upon the entry of the function, where patch_mode is
>> already known.  I do not know offhand if rearranging the code to
>> take advantage of that fact would result in a flow that is also
>> easier to follow, but I suspect it would.
>
> Okay. I changed it to:
>
> 	if (!patch_mode) {
> 		if (use_builtin_add_i < 0)
> 			git_config_get_bool("add.interactive.usebuiltin",
> 					    &use_builtin_add_i);
> 		if (use_builtin_add_i == 1)
> 			return !!run_add_i(the_repository, pathspec);
> 	}

Doesn't look so bad as I feared.

Thanks.