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

[Jeff Hostetler <git@jeffhostetler.com>]

On 4/10/2019 1:37 PM, Johannes Schindelin via GitGitGadget wrote:
> 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. 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 <johannes.schindelin@gmx.de>
> ---
>   Documentation/config/add.txt |  5 +++++
>   Makefile                     |  1 +
>   add-interactive.c            | 13 +++++++++++++
>   add-interactive.h            | 10 ++++++++++
>   builtin/add.c                | 16 +++++++++++++++-
>   t/README                     |  4 ++++
>   6 files changed, 48 insertions(+), 1 deletion(-)
>   create mode 100644 add-interactive.c
>   create mode 100644 add-interactive.h
> 
> 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 c5240942f2..18e656a32f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -848,6 +848,7 @@ LIB_H = $(shell $(FIND) . \
>   	-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..540bf185d8
> --- /dev/null
> +++ b/add-interactive.c
> @@ -0,0 +1,13 @@
> +#include "cache.h"
> +#include "add-interactive.h"
> +#include "config.h"
> +
> +int add_i_config(const char *var, const char *value, void *cb)
> +{
> +	return git_default_config(var, value, cb);
> +}
[...]
> diff --git a/builtin/add.c b/builtin/add.c
> index db2dfa4350..5a32a755c8 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
[...]
> +static int use_builtin_add_i;
[...]
> @@ -319,7 +324,12 @@ 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);
> +	if (!strcmp(var, "add.interactive.usebuiltin")) {
> +		use_builtin_add_i = git_config_bool(var, value);
> +		return 0;
> +	}
> +
> +	return add_i_config(var, value, cb);
 >   }

Something about this split between add_config() in this file
and add_i_config() in add-interactive.c bothers me.  I'm not
saying it is wrong, but it bothers me.

Perhaps it is that we moved the call to git_default_config()
to add-interactive.c -- so correct behavior of the non-interactive
case depends on add-interactive.c to do the right thing.

Also, since we can't control the order of observed the k/v pairs,
we can't do "if (use_builtin_add_i) return add_i_config(...);"
And that wouldn't allow both versions to pickup a common config
setting (in their own static variables).

Currently, neither function looks at any other k/v pairs, so
this is a bit of a moot point, but I'm wondering if this should
look like this:

     int add_config(...)
     {
         // give add-interactive.c a chance to look at k/v pair, but
         // do not short-cut because we don't know yet whether we
         // will be interactive or not yet.
         (void)add_i_config(...);

         ...ignore_add_errors...
         ...use_builtin_add_i...

         return git_default_config(...);
     }

     int add_i_config(...)
     {
         return 0;
     }

or just inline everything here in add_config() and be done with it.


Jeff