Re: [PATCH v3 01/11] Start to implement a built-in version of `git add --interactive`

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

Hi Junio,

On Wed, 31 Jul 2019, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > +add.interactive.useBuiltin::
>
> I am not sure if three-level name is a good thing to use here.
>
> If we have end-user controllable (like branch or remote names)
> unbounded number of subcommand/submode to "add", and "interactive"
> is merely one of it, then three-level name is a perfect fit, but
> otherwise, not.

Well, my thinking was that `add.useBuiltin` would be misleading (because
the non-interactive part of `git add` is _already_ built-in, even `git
add -e` is built-in). And `addInteractive.useBuiltin`, to me, would
pretend that `add-interactive` is the name of the command.

Besides, I really hope that this would be only temporary, as I already
have a fully-built-in `git add -i` and `git add -p` in Git for Windows,
as an experimental opt-in, and so far it looks like it could replace the
scripted version relatively soon, so maybe that particular part is not
worth all that much worry ;-)

> > @@ -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);
> > +	if (use_builtin_add_i < 0)
> > +		git_config_get_bool("add.interactive.usebuiltin",
> > +				    &use_builtin_add_i);
> > +
> > +	if (use_builtin_add_i == 1 && !patch_mode)
> > +		return !!run_add_i(the_repository, pathspec);
>
> I am hoping that eventually "add -p" will also be routed to the new
> codepath.  Would it make sense to have "&& !patch_mode" here,
> especially at this step where run_add_i() won't do anything useful
> anyway yet?

The `&& !patch_mode` is here to allow for a gradual adoption of the
built-in parts. I don't want users who opted in to using the built-in
`git add -i` to be stopped from using `git add -p`, so I don't want to
print even a warning, let alone an error message, when the patch mode
needs to run under `add.interactive.useBuiltin = true`, even if that
part is still scripted-only.

Of course, eventually this will be handled. See
https://github.com/gitgitgadget/git/pull/173 for the
yet-to-be-contributed patch series.

I just don't want to send a multi-dozen patch series. I really don't
think there is any effective way to review such a long patch series,
let alone an efficient way to develop it incrementally based on feedback
on the mailing list, hance I broke things up into 6 separate patch
series (as indicated by the cover letter), and this one is the first of
them.

> > @@ -319,6 +328,7 @@ static int add_config(const char *var, const char *value, void *cb)
> >  		ignore_add_errors = git_config_bool(var, value);
> >  		return 0;
> >  	}
> > +
> >  	return git_default_config(var, value, cb);
> >  }
>
> Good addition while at it.

:-)

This was actually an oversight, sorry... But since you're in favor ;-)

Ciao,
Dscho