From: Junio C Hamano <gitster@pobox.com>
To: "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Alban Gruin <alban.gruin@gmail.com>,
Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: Re: [PATCH] help: do not expect built-in commands to be hardlinked
Date: Wed, 07 Oct 2020 10:48:41 -0700 [thread overview]
Message-ID: <xmqqh7r62ah2.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <xmqqpn5u2bps.fsf@gitster.c.googlers.com> (Junio C. Hamano's message of "Wed, 07 Oct 2020 10:21:51 -0700")
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.
>> 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
next prev parent reply other threads:[~2020-10-07 17:48 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-07 12:43 [PATCH] help: do not expect built-in commands to be hardlinked Johannes Schindelin via GitGitGadget
2020-10-07 17:21 ` Junio C Hamano
2020-10-07 17:48 ` Junio C Hamano [this message]
2020-10-07 21:49 ` Johannes Schindelin
2020-10-07 22:24 ` Junio C Hamano
2020-10-07 21:43 ` Johannes Schindelin
2020-10-07 21:56 ` [PATCH v2] " Johannes Schindelin via GitGitGadget
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqh7r62ah2.fsf@gitster.c.googlers.com \
--to=gitster@pobox.com \
--cc=alban.gruin@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=johannes.schindelin@gmx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).