Re: [PATCH] help: do not expect built-in commands to be hardlinked

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Wed, 7 Oct 2020, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
> > ... 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.
>
> >> 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);
>
> I wondered if we need, after this change, to worry about duplicates,
> because some Git subcommands, even after they made into a built-in
> and callable internally, must have on-disk footprint.
>
> It turns out that after the post-context in this hunk we do make a
> call to uniq(main_cmds) so it is fine.
>
> This was unexpected to me, as we read only from a single directory
> "exec_path" and the need to call uniq() in the old world order would
> have meant that readdir in exec_path gave us duplicate entries.
>
> In fact, the very original version of load_command_list() did not
> have this unnecessary call to uniq().  It was introduced in 1f08e5ce
> (Allow git help work without PATH set, 2008-08-28); perhaps Alex saw
> 12 years into the future and predicted that we would start needing
> it ;-)
>
> In any case, the patch is good thanks to that existing uniq() call.

Yep, I was fully prepared to add that `uniq()` call and was surprised to
find it. I guess it was "for good measure" because the same commit also
added the same `qsort(); uniq()` combo another time, a little further down
in that function.

Now, what I would have expected you to say when you found the `uniq()`
function is: Johannes, why don't you call `QSORT(); uniq()` after the call
to `load_builtin_commands()`? After all, `exec_path` and `env_path` might
both be `NULL`...

Well, the answer to that question is _not_ "but without `env_path` nothing
works anyway" even if that would be pretty valid. The answer is that the
`commands[]` list in `git.c` is already sorted alphabetically.

Thanks,
Dscho

>
> >> 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
>