Re: [PATCH 01/11] Start to implement a built-in version of `git add --interactive`
- From
- Jeff Hostetler <git@jeffhostetler.com>
- Date
- Apr 18, 2019, 14:31 UTC
- Message-ID
- <3149b9ba-2f5a-46ce-ad89-4105ec217795@jeffhostetler.com>
- In-Reply-To
- <12978dc248a2cd07c90559691b8a2add84f45394.1554917868.git.gitgitgadget@gmail.com>
On 4/10/2019 1:37 PM, Johannes Schindelin via GitGitGadget wrote:
Show 79 quoted lines
> 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;
[...]
Show 11 quoted lines
> @@ -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