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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Fri, 8 Nov 2019, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > This is hardly the first conversion of a Git command that is implemented
> > as a script to a built-in. So far, the most successful strategy for such
> > conversions has been to add a built-in helper and call that for more and
> > more functionality from the script, as more and more parts are
> > converted.
> >
> > With the interactive add, we choose a different strategy....
>
> This is hardly the first conversion that we took the "build the
> whole program piece by piece and flip the whole thing on with
> usebuiltin" conversion successfully.  Pratik's rebase-in-c series
> comes to mind.
>
> Personally, I do not think the first two paragraphs of the proposed
> log message do not belong here.  Cover letter is a different story
> and it may make sense to explain why the approach was taken there,
> but here, I'd prefer to see it more succinctly tell what approach is
> taken and go directly to describe what this step in that approach
> does to the readers, which is more important.

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?

> > diff --git a/builtin/add.c b/builtin/add.c
> > index dd18e5c9b6..4f625691b5 100644
> > --- a/builtin/add.c
> > +++ b/builtin/add.c
> > @@ -20,6 +20,7 @@
> >  #include "bulk-checkin.h"
> >  #include "argv-array.h"
> >  #include "submodule.h"
> > +#include "add-interactive.h"
> >
> >  static const char * const builtin_add_usage[] = {
> >  	N_("git add [<options>] [--] <pathspec>..."),
> > @@ -185,6 +186,14 @@ int run_add_interactive(const char *revision, const char *patch_mode,
> >  {
> >  	int status, i;
> >  	struct argv_array argv = ARGV_ARRAY_INIT;
> > +	int use_builtin_add_i =
> > +		git_env_bool("GIT_TEST_ADD_I_USE_BUILTIN", -1);
>
> Have blank here at the boundary between decl and stmt ...
>
> > +	if (use_builtin_add_i < 0)
> > +		git_config_get_bool("add.interactive.usebuiltin",
> > +				    &use_builtin_add_i);
> > +
>
> ... and lose it here (optional).

Done.
>
> > +	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);
	}

Thanks,
Dscho