Hi Junio, On Wed, 7 Oct 2020, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx> > writes: > > > From: Johannes Schindelin <johannes.schindelin@xxxxxx> > > > > When building with SKIP_DASHED_BUILT_INS=YesPlease, the built-in > > commands are no longer present in the `PATH` as hardlinks to `git`. > > > > As a consequence, `load_command_list()` needs to be taught to find the > > names of the built-in commands from elsewhere. > > > > This only affected the output of `git --list-cmds=main`, but not the > > output of `git help -a` because the latter includes the built-in > > commands by virtue of them being listed in command-list.txt. > > > > The bug was detected via a patch series that turns the merge strategies > > included in Git into built-in commands: `git merge -s help` relies on > > `load_command_list()` to determine the list of available merge > > strategies. > > > > Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx> > > --- > > Fix the command list with SKIP_DASHED_BUILT_INS=YesPlease > > > > In a recent patch series > > [https://lore.kernel.org/git/20201005122646.27994-12-alban.gruin@xxxxxxxxx/#r] > > , the merge strategies were converted into built-ins, which is good. > > > > Together with the change where we stop hard-linking the built-in > > commands in CI builds, this broke t9902.199. > > > > The actual root cause is that git merge -s help relies on > > load_command_list() to find all available Git commands, and that > > function had the long-standing bug that it expects the built-in commands > > to be available in the PATH. > > > > That is not a bug in "merge -s help" or "longstanding" at all. It > has been a quite natural and long-standing expectation to find all > the merge strategies on PATH (after GIT_EXEC_PATH is added to it), > because that was the promise we gave to our users long time ago and > have kept. Sure, we promised to the outside world that those built-ins would always be in the PATH, but we highly recommended against using dashed invocations. To me, that means that _internally_ we should have been more stringent about how we do things ourselves. In any case, your complaint isn't about the commit message, so I hope it can advance? > The bug is in load_command_list() and it was introduced by the > recent SKIP_DASHED_BUILT_INS series. We forgot to teach the > function that in the new world order, what we see on disk plus what > we have in the built-in table are the set of subcommands available > to us, and the rule that was valid in the old world order can no > longer be relied upon, and nobody noticed the breakage while > developing or reviewing. > > > git.c | 13 +++++++++++++ > > help.c | 2 ++ > > help.h | 1 + > > 3 files changed, 16 insertions(+) > > > > diff --git a/git.c b/git.c > > index d51fb5d2bf..a6224badce 100644 > > --- a/git.c > > +++ b/git.c > > @@ -641,6 +641,19 @@ static void list_builtins(struct string_list *out, unsigned int exclude_option) > > } > > } > > > > +void load_builtin_commands(const char *prefix, struct cmdnames *cmds) > > +{ > > + const char *name; > > + int i; > > + > > + if (!skip_prefix(prefix, "git-", &prefix)) > > + return; > > Do we want to explain that this is for dropping "gitk" and the like > in a comment near here? I guess I have to explain this, as it is too easy to mistake this `skip_prefix()` to work on the actual command names rather than about the `prefix` parameter. The `commands[]` array in `git.c` stores only the command names, but `load_command_list()` is called with the prefix `git-` or `git-merge-`. Therefore, `load_builtin_commands()` skips the prefix `git-` *from the `prefix` itself*. I'll send the next iteration shortly. Ciao, Dscho > > > + for (i = 0; i < ARRAY_SIZE(commands); i++) > > + if (skip_prefix(commands[i].cmd, prefix, &name)) > > + add_cmdname(cmds, name, strlen(name)); > > +} > > + > > #ifdef STRIP_EXTENSION > > static void strip_extension(const char **argv) > > { > > diff --git a/help.c b/help.c > > index 4e2468a44d..919cbb9206 100644 > > --- a/help.c > > +++ b/help.c > > @@ -263,6 +263,8 @@ void load_command_list(const char *prefix, > > const char *env_path = getenv("PATH"); > > const char *exec_path = git_exec_path(); > > > > + load_builtin_commands(prefix, main_cmds); > > + > > if (exec_path) { > > list_commands_in_dir(main_cmds, exec_path, prefix); > > QSORT(main_cmds->names, main_cmds->cnt, cmdname_compare); > > diff --git a/help.h b/help.h > > index dc02458855..5871e93ba2 100644 > > --- a/help.h > > +++ b/help.h > > @@ -32,6 +32,7 @@ const char *help_unknown_cmd(const char *cmd); > > void load_command_list(const char *prefix, > > struct cmdnames *main_cmds, > > struct cmdnames *other_cmds); > > +void load_builtin_commands(const char *prefix, struct cmdnames *cmds); > > void add_cmdname(struct cmdnames *cmds, const char *name, int len); > > /* Here we require that excludes is a sorted list. */ > > void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes); > > > > base-commit: 8f7759d2c8c13716bfdb9ae602414fd987787e8d >
