Hi Junio,
On Wed, 31 Jul 2019, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx>
> 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
