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

Commit Message

Linus Arver via GitGitGadget July 16, 2019, 2:58 p.m. UTC

From: Johannes Schindelin <[e-mail]>

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. The sole
reason for this 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.

Instead, we follow the opposite route where 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.

At this point, the built-in version of `git add -i` only states that it
cannot do anything yet ;-)

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 Documentation/config/add.txt |  5 +++++
 Makefile                     |  1 +
 add-interactive.c            |  7 +++++++
 add-interactive.h            |  8 ++++++++
 builtin/add.c                | 10 ++++++++++
 t/README                     |  4 ++++
 6 files changed, 35 insertions(+)
 create mode 100644 add-interactive.c
 create mode 100644 add-interactive.h

Comments

Junio C Hamano July 31, 2019, 5:52 p.m. UTC | #1

"Johannes Schindelin via GitGitGadget" <[e-mail]>
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.

> @@ -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?

> @@ -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.

Johannes Schindelin Aug. 26, 2019, 9:26 p.m. UTC | #2

Hi Junio,

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

> "Johannes Schindelin via GitGitGadget" <[e-mail]>
> 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

Junio C Hamano Aug. 27, 2019, 10:25 p.m. UTC | #3

Johannes Schindelin <[e-mail]> writes:

> Besides, I really hope that this would be only temporary,...

Oh, no question about it.  This should be temporary knob.

I still do worry about giving a bad example for others to copy.
People tend to copy & paste without thinking.  Either we come up
with and use a two-level name, or we add a comment to explain to
developers (not users---as this is merely a temporary thing) why
they should never follow suit using three-level names for things
like this one written in big red letters, or something, then perhaps
we won't have to worry about too much?  I dunno.

>> > +	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. ...

Ah, so "add.usebuiltin = interactive patch" can (eventually) choose
to use the C code for both while "add.usebuiltin = interactive"
would not use it for the patch mode, or something?  Or even

	add.interactive.usebuiltin = yes
	add.patch.usebuiltin = no

perhaps?

> Of course, eventually this will be handled.

Yup, again, the knob is merely temporary.

Johannes Schindelin Aug. 28, 2019, 3:06 p.m. UTC | #4

Hi Junio,

On Tue, 27 Aug 2019, Junio C Hamano wrote:

> Johannes Schindelin <[e-mail]> writes:
>
> > Besides, I really hope that this would be only temporary,...
>
> Oh, no question about it.  This should be temporary knob.
>
> I still do worry about giving a bad example for others to copy.
> People tend to copy & paste without thinking.  Either we come up
> with and use a two-level name, or we add a comment to explain to
> developers (not users---as this is merely a temporary thing) why
> they should never follow suit using three-level names for things
> like this one written in big red letters, or something, then perhaps
> we won't have to worry about too much?  I dunno.
>
> >> > +	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. ...
>
> Ah, so "add.usebuiltin = interactive patch" can (eventually) choose
> to use the C code for both while "add.usebuiltin = interactive"
> would not use it for the patch mode, or something?  Or even
>
> 	add.interactive.usebuiltin = yes

This is what I had in mind.

> 	add.patch.usebuiltin = no

And this is what I should have done ;-)

But maybe I should do

	add.useBuiltin = wheneverPossible

?

Ciao,
Dscho

>
> perhaps?
>
> > Of course, eventually this will be handled.
>
> Yup, again, the knob is merely temporary.

Junio C Hamano Aug. 28, 2019, 3:37 p.m. UTC | #5

Johannes Schindelin <[e-mail]> writes:

>> Ah, so "add.usebuiltin = interactive patch" can (eventually) choose
>> to use the C code for both while "add.usebuiltin = interactive"
>> would not use it for the patch mode, or something?  Or even
>>
>> 	add.interactive.usebuiltin = yes
>
> This is what I had in mind.
>
>> 	add.patch.usebuiltin = no
>
> And this is what I should have done ;-)
>
> But maybe I should do
>
> 	add.useBuiltin = wheneverPossible

Ah, either is fine, I think, but because this is meant to be
temporary and not advertised to end-users for purposes other than
purely an escape hatch, I would not spend too much engineering
effort (iow, do the easiest to add and then easiest to later yank
out).

Thanks.

Patch

diff --git a/Documentation/config/add.txt b/Documentation/config/add.txt
index 4d753f006e..c9f748f81c 100644
--- a/Documentation/config/add.txt
+++ b/Documentation/config/add.txt
@@ -5,3 +5,8 @@  add.ignore-errors (deprecated)::
 	option of linkgit:git-add[1].  `add.ignore-errors` is deprecated,
 	as it does not follow the usual naming convention for configuration
 	variables.
+
+add.interactive.useBuiltin::
+	[EXPERIMENTAL] Set to `true` to use the experimental built-in
+	implementation of the interactive version of linkgit:git-add[1]
+	instead of the Perl script version. Is `false` by default.
diff --git a/Makefile b/Makefile
index 98a0588416..0a47200f47 100644
--- a/Makefile
+++ b/Makefile
@@ -824,6 +824,7 @@  LIB_H := $(sort $(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null
 	-name '*.h' -print))
 
 LIB_OBJS += abspath.o
+LIB_OBJS += add-interactive.o
 LIB_OBJS += advice.o
 LIB_OBJS += alias.o
 LIB_OBJS += alloc.o
diff --git a/add-interactive.c b/add-interactive.c
new file mode 100644
index 0000000000..482e458dc6
--- /dev/null
+++ b/add-interactive.c
@@ -0,0 +1,7 @@ 
+#include "cache.h"
+#include "add-interactive.h"
+
+int run_add_i(struct repository *r, const struct pathspec *ps)
+{
+	die(_("No commands are available in the built-in `git add -i` yet!"));
+}
diff --git a/add-interactive.h b/add-interactive.h
new file mode 100644
index 0000000000..7043b8741d
--- /dev/null
+++ b/add-interactive.h
@@ -0,0 +1,8 @@ 
+#ifndef ADD_INTERACTIVE_H
+#define ADD_INTERACTIVE_H
+
+struct repository;
+struct pathspec;
+int run_add_i(struct repository *r, const struct pathspec *ps);
+
+#endif
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);
+	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);
 
 	argv_array_push(&argv, "add--interactive");
 	if (patch_mode)
@@ -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);
 }
 
diff --git a/t/README b/t/README
index 9747971d58..28999cebd3 100644
--- a/t/README
+++ b/t/README
@@ -397,6 +397,10 @@  GIT_TEST_STASH_USE_BUILTIN=<boolean>, when false, disables the
 built-in version of git-stash. See 'stash.useBuiltin' in
 git-config(1).
 
+GIT_TEST_ADD_I_USE_BUILTIN=<boolean>, when true, enables the
+builtin version of git add -i. See 'add.interactive.useBuiltin' in
+git-config(1).
+
 GIT_TEST_INDEX_THREADS=<n> enables exercising the multi-threaded loading
 of the index for the whole test suite by bypassing the default number of
 cache entries and thread minimums. Setting this to 1 will make the