"Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx>
writes:
> From: Johannes Schindelin <johannes.schindelin@xxxxxx>
>
> 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....
This is hardly the first conversion that we took the "build the
whole program piece by piece and flip the whole thing on with
usebuiltin" conversion successfully. Pratik's rebase-in-c series
comes to mind.
Personally, I do not think the first two paragraphs of the proposed
log message do not belong here. Cover letter is a different story
and it may make sense to explain why the approach was taken there,
but here, I'd prefer to see it more succinctly tell what approach is
taken and go directly to describe what this step in that approach
does to the readers, which is more important.
> diff --git a/Documentation/config/add.txt b/Documentation/config/add.txt
> 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.
Good.
> diff --git a/Makefile b/Makefile
> index 58b92af54b..6c4a1e0ee5 100644
> --- a/Makefile
> +++ b/Makefile
> LIB_OBJS += abspath.o
> +LIB_OBJS += add-interactive.o
> LIB_OBJS += advice.o
> LIB_OBJS += alias.o
> LIB_OBJS += alloc.o
OK.
> 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!"));
> +}
OK, with or without s/commands/sub&/;
> diff --git a/add-interactive.h b/add-interactive.h
> new file mode 100644
> index 0000000000..7043b8741d
> --- /dev/null
> +++ b/add-interactive.h
OK.
> 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);
Have blank here at the boundary between decl and stmt ...
> + if (use_builtin_add_i < 0)
> + git_config_get_bool("add.interactive.usebuiltin",
> + &use_builtin_add_i);
> +
... and lose it here (optional).
> + if (use_builtin_add_i == 1 && !patch_mode)
> + return !!run_add_i(the_repository, pathspec);
>
Strictly speaking, we can bypass the probing of environment and
config when upon the entry of the function, where patch_mode is
already known. I do not know offhand if rearranging the code to
take advantage of that fact would result in a flow that is also
easier to follow, but I suspect it would.
> +GIT_TEST_ADD_I_USE_BUILTIN=<boolean>, when true, enables the
> +built-in version of git add -i. See 'add.interactive.useBuiltin' in
> +git-config(1).
Makes sense.
Thanks.
