| Message ID | 5d9962d4344fa182b37cd8d969da01bc603414be.1573648866.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git add -i: add a rudimentary version in C (supporting only status and help so far) | expand |
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > The very real downside of the approach we have to take here is that the > test suite won't pass with `GIT_TEST_ADD_I_USE_BUILTIN=true` until the > conversion is complete (the `--helper` approach would have let it pass, > even at each of the incremental conversion steps). That actually is an issue of the quality of the tests you add for this series. If they are organized along the arc of run_add_i() gaining features, the invocation of "git add -i" in the tests can selectively use USE_BUILTIN=true to verify what has been rewritten so far. In any case, I think I've seen the patches in this part of the metaseries enough and I think they are quickly stabilizing. Let's see if others can find and raise issues and otherwise start cooking in 'next' sometime next week. Thanks.
Hi Junio, On Thu, 14 Nov 2019, Junio C Hamano wrote: > any case, I think I've seen the patches in this part of the metaseries > enough and I think they are quickly stabilizing. Let's see if others > can find and raise issues and otherwise start cooking in 'next' > sometime next week. While reviewing the next patch series in this arc, I *just* noticed a buffer overrun: in the main loop, `path + sep` might point to the trailing `NUL`, but we assign `p += sep + 1;` at the end (which is only correct when `path + sep` points at whitespace). The fix is already pushed up into gitgitgadget/git#170, and the relevant part of the range-diff reads like this: @@ add-interactive.c: static void list(struct add_i_state *s, struct string_list *l + index = -1; + } + -+ p[sep] = '\0'; ++ if (p[sep]) ++ p[sep++] = '\0'; + if (index < 0 || index >= items->nr) + printf(_("Huh (%s)?\n"), p); + else { @@ add-interactive.c: static void list(struct add_i_state *s, struct string_list *l + break; + } + -+ p += sep + 1; ++ p += sep; + } + + if (res != LIST_AND_CHOOSE_ERROR) I plan on waiting for the PR build to finish, and maybe wait until tomorrow just in case any further suggestion rolls in, then submit the hopefully final iteration. And yes, I think it is a good time to start cooking this in `next`, I, for one, am prone to overlook anything crucial in those patches because I have stared at them so often. Ciao, Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > I plan on waiting for the PR build to finish, and maybe wait until > tomorrow just in case any further suggestion rolls in, then submit the > hopefully final iteration. Thanks.
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 58b92af54b..6c4a1e0ee5 100644 --- a/Makefile +++ b/Makefile @@ -823,6 +823,7 @@ LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!Documentat -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..d4686d5218 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,16 @@ 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 (!patch_mode) { + if (use_builtin_add_i < 0) + git_config_get_bool("add.interactive.usebuiltin", + &use_builtin_add_i); + if (use_builtin_add_i == 1) + return !!run_add_i(the_repository, pathspec); + } argv_array_push(&argv, "add--interactive"); if (patch_mode) @@ -319,6 +330,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 60d5b77bcc..5132ec83f8 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 +built-in 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