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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 7, 2020, 17:48 UTC
Message-ID: <xmqqh7r62ah2.fsf@gitster.c.googlers.com>
In-Reply-To: <xmqqpn5u2bps.fsf@gitster.c.googlers.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 5 quoted lines

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

Show 13 quoted lines

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

Show 14 quoted lines

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