{"thread":{"id":"54364","subject":"[PATCH] help: do not expect built-in commands to be hardlinked","startedAt":"2020-10-07T12:43:16Z","lastAt":"2020-10-07T22:24:51Z","messageCount":7,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"407048","messageId":"pull.745.git.1602074589460.gitgitgadget@gmail.com","threadId":"54364","inReplyTo":null,"subject":"[PATCH] help: do not expect built-in commands to be hardlinked","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-07T12:43:09Z","receivedAt":"2020-10-07T12:43:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen building with SKIP_DASHED_BUILT_INS=YesPlease, the built-in\ncommands are no longer present in the `PATH` as hardlinks to `git`.\n\nAs a consequence, `load_command_list()` needs to be taught to find the\nnames of the built-in commands from elsewhere.\n\nThis only affected the output of `git --list-cmds=main`, but not the\noutput of `git help -a` because the latter includes the built-in\ncommands by virtue of them being listed in command-list.txt.\n\nThe bug was detected via a patch series that turns the merge strategies\nincluded in Git into built-in commands: `git merge -s help` relies on\n`load_command_list()` to determine the list of available merge\nstrategies.\n\nSigned-off-by: Johannes Schindelin \n---\n Fix the command list with SKIP_DASHED_BUILT_INS=YesPlease\n \n In a recent patch series\n [https://lore.kernel.org/git/20201005122646.27994-12-alban.gruin@gmail.com/#r]\n , the merge strategies were converted into built-ins, which is good.\n \n Together with the change where we stop hard-linking the built-in\n commands in CI builds, this broke t9902.199.\n \n The actual root cause is that git merge -s help relies on \n load_command_list() to find all available Git commands, and that\n function had the long-standing bug that it expects the built-in commands\n to be available in the PATH.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-745%2Fdscho%2Falways-include-builtins-in-commands-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-745/dscho/always-include-builtins-in-commands-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/745\n\n git.c | 13 +++++++++++++\n help.c | 2 ++\n help.h | 1 +\n 3 files changed, 16 insertions(+)\n\ndiff --git a/git.c b/git.c\nindex d51fb5d2bf..a6224badce 100644\n--- a/git.c\n+++ b/git.c\n@@ -641,6 +641,19 @@ static void list_builtins(struct string_list *out, unsigned int exclude_option)\n \t}\n }\n \n+void load_builtin_commands(const char *prefix, struct cmdnames *cmds)\n+{\n+\tconst char *name;\n+\tint i;\n+\n+\tif (!skip_prefix(prefix, \"git-\", &prefix))\n+\t\treturn;\n+\n+\tfor (i = 0; i < ARRAY_SIZE(commands); i++)\n+\t\tif (skip_prefix(commands[i].cmd, prefix, &name))\n+\t\t\tadd_cmdname(cmds, name, strlen(name));\n+}\n+\n #ifdef STRIP_EXTENSION\n static void strip_extension(const char **argv)\n {\ndiff --git a/help.c b/help.c\nindex 4e2468a44d..919cbb9206 100644\n--- a/help.c\n+++ b/help.c\n@@ -263,6 +263,8 @@ void load_command_list(const char *prefix,\n \tconst char *env_path = getenv(\"PATH\");\n \tconst char *exec_path = git_exec_path();\n \n+\tload_builtin_commands(prefix, main_cmds);\n+\n \tif (exec_path) {\n \t\tlist_commands_in_dir(main_cmds, exec_path, prefix);\n \t\tQSORT(main_cmds->names, main_cmds->cnt, cmdname_compare);\ndiff --git a/help.h b/help.h\nindex dc02458855..5871e93ba2 100644\n--- a/help.h\n+++ b/help.h\n@@ -32,6 +32,7 @@ const char *help_unknown_cmd(const char *cmd);\n void load_command_list(const char *prefix,\n \t\t struct cmdnames *main_cmds,\n \t\t struct cmdnames *other_cmds);\n+void load_builtin_commands(const char *prefix, struct cmdnames *cmds);\n void add_cmdname(struct cmdnames *cmds, const char *name, int len);\n /* Here we require that excludes is a sorted list. */\n void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes);\n\nbase-commit: 8f7759d2c8c13716bfdb9ae602414fd987787e8d\n-- \ngitgitgadget\n"},{"id":"407075","messageId":"xmqqpn5u2bps.fsf@gitster.c.googlers.com","threadId":"54364","inReplyTo":"pull.745.git.1602074589460.gitgitgadget@gmail.com","subject":"Re: [PATCH] help: do not expect built-in commands to be hardlinked","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-07T17:21:51Z","receivedAt":"2020-10-07T17:21:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> When building with SKIP_DASHED_BUILT_INS=YesPlease, the built-in\n> commands are no longer present in the `PATH` as hardlinks to `git`.\n>\n> As a consequence, `load_command_list()` needs to be taught to find the\n> names of the built-in commands from elsewhere.\n>\n> This only affected the output of `git --list-cmds=main`, but not the\n> output of `git help -a` because the latter includes the built-in\n> commands by virtue of them being listed in command-list.txt.\n>\n> The bug was detected via a patch series that turns the merge strategies\n> included in Git into built-in commands: `git merge -s help` relies on\n> `load_command_list()` to determine the list of available merge\n> strategies.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> Fix the command list with SKIP_DASHED_BUILT_INS=YesPlease\n> \n> In a recent patch series\n> [https://lore.kernel.org/git/20201005122646.27994-12-alban.gruin@gmail.com/#r]\n> , the merge strategies were converted into built-ins, which is good.\n> \n> Together with the change where we stop hard-linking the built-in\n> commands in CI builds, this broke t9902.199.\n> \n> The actual root cause is that git merge -s help relies on \n> load_command_list() to find all available Git commands, and that\n> function had the long-standing bug that it expects the built-in commands\n> to be available in the PATH.\n>\n\nThat is not a bug in \"merge -s help\" or \"longstanding\" at all. It\nhas been a quite natural and long-standing expectation to find all\nthe merge strategies on PATH (after GIT_EXEC_PATH is added to it),\nbecause that was the promise we gave to our users long time ago and\nhave kept.\n\nThe bug is in load_command_list() and it was introduced by the\nrecent SKIP_DASHED_BUILT_INS series. We forgot to teach the\nfunction that in the new world order, what we see on disk plus what\nwe have in the built-in table are the set of subcommands available\nto us, and the rule that was valid in the old world order can no\nlonger be relied upon, and nobody noticed the breakage while\ndeveloping or reviewing.\n\n> git.c | 13 +++++++++++++\n> help.c | 2 ++\n> help.h | 1 +\n> 3 files changed, 16 insertions(+)\n>\n> diff --git a/git.c b/git.c\n> index d51fb5d2bf..a6224badce 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -641,6 +641,19 @@ static void list_builtins(struct string_list *out, unsigned int exclude_option)\n> \t}\n> }\n> \n> +void load_builtin_commands(const char *prefix, struct cmdnames *cmds)\n> +{\n> +\tconst char *name;\n> +\tint i;\n> +\n> +\tif (!skip_prefix(prefix, \"git-\", &prefix))\n> +\t\treturn;\n\nDo we want to explain that this is for dropping \"gitk\" and the like\nin a comment near here?\n\n> +\tfor (i = 0; i < ARRAY_SIZE(commands); i++)\n> +\t\tif (skip_prefix(commands[i].cmd, prefix, &name))\n> +\t\t\tadd_cmdname(cmds, name, strlen(name));\n> +}\n> +\n> #ifdef STRIP_EXTENSION\n> static void strip_extension(const char **argv)\n> {\n> diff --git a/help.c b/help.c\n> index 4e2468a44d..919cbb9206 100644\n> --- a/help.c\n> +++ b/help.c\n> @@ -263,6 +263,8 @@ void load_command_list(const char *prefix,\n> \tconst char *env_path = getenv(\"PATH\");\n> \tconst char *exec_path = git_exec_path();\n> \n> +\tload_builtin_commands(prefix, main_cmds);\n> +\n> \tif (exec_path) {\n> \t\tlist_commands_in_dir(main_cmds, exec_path, prefix);\n> \t\tQSORT(main_cmds->names, main_cmds->cnt, cmdname_compare);\n> diff --git a/help.h b/help.h\n> index dc02458855..5871e93ba2 100644\n> --- a/help.h\n> +++ b/help.h\n> @@ -32,6 +32,7 @@ const char *help_unknown_cmd(const char *cmd);\n> void load_command_list(const char *prefix,\n> \t\t struct cmdnames *main_cmds,\n> \t\t struct cmdnames *other_cmds);\n> +void load_builtin_commands(const char *prefix, struct cmdnames *cmds);\n> void add_cmdname(struct cmdnames *cmds, const char *name, int len);\n> /* Here we require that excludes is a sorted list. */\n> void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes);\n>\n> base-commit: 8f7759d2c8c13716bfdb9ae602414fd987787e8d\n"},{"id":"407077","messageId":"xmqqh7r62ah2.fsf@gitster.c.googlers.com","threadId":"54364","inReplyTo":"xmqqpn5u2bps.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] help: do not expect built-in commands to be hardlinked","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-07T17:48:41Z","receivedAt":"2020-10-07T17:48:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> ... in the new world order, what we see on disk plus what\n> we have in the built-in table are the set of subcommands available\n> to us, and the rule that was valid in the old world order can no\n> longer be relied upon, and nobody noticed the breakage while\n> developing or reviewing.\n\n>> diff --git a/help.c b/help.c\n>> index 4e2468a44d..919cbb9206 100644\n>> --- a/help.c\n>> +++ b/help.c\n>> @@ -263,6 +263,8 @@ void load_command_list(const char *prefix,\n>> \tconst char *env_path = getenv(\"PATH\");\n>> \tconst char *exec_path = git_exec_path();\n>> \n>> +\tload_builtin_commands(prefix, main_cmds);\n>> +\n>> \tif (exec_path) {\n>> \t\tlist_commands_in_dir(main_cmds, exec_path, prefix);\n>> \t\tQSORT(main_cmds->names, main_cmds->cnt, cmdname_compare);\n\nI wondered if we need, after this change, to worry about duplicates,\nbecause some Git subcommands, even after they made into a built-in\nand callable internally, must have on-disk footprint.\n\nIt turns out that after the post-context in this hunk we do make a\ncall to uniq(main_cmds) so it is fine.\n\nThis was unexpected to me, as we read only from a single directory\n\"exec_path\" and the need to call uniq() in the old world order would\nhave meant that readdir in exec_path gave us duplicate entries.\n\nIn fact, the very original version of load_command_list() did not\nhave this unnecessary call to uniq(). It was introduced in 1f08e5ce\n(Allow git help work without PATH set, 2008-08-28); perhaps Alex saw\n12 years into the future and predicted that we would start needing\nit ;-)\n\nIn any case, the patch is good thanks to that existing uniq() call.\n\n>> diff --git a/help.h b/help.h\n>> index dc02458855..5871e93ba2 100644\n>> --- a/help.h\n>> +++ b/help.h\n>> @@ -32,6 +32,7 @@ const char *help_unknown_cmd(const char *cmd);\n>> void load_command_list(const char *prefix,\n>> \t\t struct cmdnames *main_cmds,\n>> \t\t struct cmdnames *other_cmds);\n>> +void load_builtin_commands(const char *prefix, struct cmdnames *cmds);\n>> void add_cmdname(struct cmdnames *cmds, const char *name, int len);\n>> /* Here we require that excludes is a sorted list. */\n>> void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes);\n>>\n>> base-commit: 8f7759d2c8c13716bfdb9ae602414fd987787e8d\n"},{"id":"407105","messageId":"nycvar.QRO.7.76.6.2010072330090.50@tvgsbejvaqbjf.bet","threadId":"54364","inReplyTo":"xmqqpn5u2bps.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] help: do not expect built-in commands to be hardlinked","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-10-07T21:43:23Z","receivedAt":"2020-10-07T21:43:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 7 Oct 2020, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > From: Johannes Schindelin \n> >\n> > When building with SKIP_DASHED_BUILT_INS=YesPlease, the built-in\n> > commands are no longer present in the `PATH` as hardlinks to `git`.\n> >\n> > As a consequence, `load_command_list()` needs to be taught to find the\n> > names of the built-in commands from elsewhere.\n> >\n> > This only affected the output of `git --list-cmds=main`, but not the\n> > output of `git help -a` because the latter includes the built-in\n> > commands by virtue of them being listed in command-list.txt.\n> >\n> > The bug was detected via a patch series that turns the merge strategies\n> > included in Git into built-in commands: `git merge -s help` relies on\n> > `load_command_list()` to determine the list of available merge\n> > strategies.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > Fix the command list with SKIP_DASHED_BUILT_INS=YesPlease\n> >\n> > In a recent patch series\n> > [https://lore.kernel.org/git/20201005122646.27994-12-alban.gruin@gmail.com/#r]\n> > , the merge strategies were converted into built-ins, which is good.\n> >\n> > Together with the change where we stop hard-linking the built-in\n> > commands in CI builds, this broke t9902.199.\n> >\n> > The actual root cause is that git merge -s help relies on\n> > load_command_list() to find all available Git commands, and that\n> > function had the long-standing bug that it expects the built-in commands\n> > to be available in the PATH.\n> >\n>\n> That is not a bug in \"merge -s help\" or \"longstanding\" at all. It\n> has been a quite natural and long-standing expectation to find all\n> the merge strategies on PATH (after GIT_EXEC_PATH is added to it),\n> because that was the promise we gave to our users long time ago and\n> have kept.\n\nSure, we promised to the outside world that those built-ins would always\nbe in the PATH, but we highly recommended against using dashed\ninvocations.\n\nTo me, that means that _internally_ we should have been more stringent\nabout how we do things ourselves.\n\nIn any case, your complaint isn't about the commit message, so I hope it\ncan advance?\n\n> The bug is in load_command_list() and it was introduced by the\n> recent SKIP_DASHED_BUILT_INS series. We forgot to teach the\n> function that in the new world order, what we see on disk plus what\n> we have in the built-in table are the set of subcommands available\n> to us, and the rule that was valid in the old world order can no\n> longer be relied upon, and nobody noticed the breakage while\n> developing or reviewing.\n>\n> > git.c | 13 +++++++++++++\n> > help.c | 2 ++\n> > help.h | 1 +\n> > 3 files changed, 16 insertions(+)\n> >\n> > diff --git a/git.c b/git.c\n> > index d51fb5d2bf..a6224badce 100644\n> > --- a/git.c\n> > +++ b/git.c\n> > @@ -641,6 +641,19 @@ static void list_builtins(struct string_list *out, unsigned int exclude_option)\n> > \t}\n> > }\n> >\n> > +void load_builtin_commands(const char *prefix, struct cmdnames *cmds)\n> > +{\n> > +\tconst char *name;\n> > +\tint i;\n> > +\n> > +\tif (!skip_prefix(prefix, \"git-\", &prefix))\n> > +\t\treturn;\n>\n> Do we want to explain that this is for dropping \"gitk\" and the like\n> in a comment near here?\n\nI guess I have to explain this, as it is too easy to mistake this\n`skip_prefix()` to work on the actual command names rather than about the\n`prefix` parameter.\n\nThe `commands[]` array in `git.c` stores only the command names, but\n`load_command_list()` is called with the prefix `git-` or `git-merge-`.\nTherefore, `load_builtin_commands()` skips the prefix `git-` *from the\n`prefix` itself*.\n\nI'll send the next iteration shortly.\n\nCiao,\nDscho\n\n>\n> > +\tfor (i = 0; i < ARRAY_SIZE(commands); i++)\n> > +\t\tif (skip_prefix(commands[i].cmd, prefix, &name))\n> > +\t\t\tadd_cmdname(cmds, name, strlen(name));\n> > +}\n> > +\n> > #ifdef STRIP_EXTENSION\n> > static void strip_extension(const char **argv)\n> > {\n> > diff --git a/help.c b/help.c\n> > index 4e2468a44d..919cbb9206 100644\n> > --- a/help.c\n> > +++ b/help.c\n> > @@ -263,6 +263,8 @@ void load_command_list(const char *prefix,\n> > \tconst char *env_path = getenv(\"PATH\");\n> > \tconst char *exec_path = git_exec_path();\n> >\n> > +\tload_builtin_commands(prefix, main_cmds);\n> > +\n> > \tif (exec_path) {\n> > \t\tlist_commands_in_dir(main_cmds, exec_path, prefix);\n> > \t\tQSORT(main_cmds->names, main_cmds->cnt, cmdname_compare);\n> > diff --git a/help.h b/help.h\n> > index dc02458855..5871e93ba2 100644\n> > --- a/help.h\n> > +++ b/help.h\n> > @@ -32,6 +32,7 @@ const char *help_unknown_cmd(const char *cmd);\n> > void load_command_list(const char *prefix,\n> > \t\t struct cmdnames *main_cmds,\n> > \t\t struct cmdnames *other_cmds);\n> > +void load_builtin_commands(const char *prefix, struct cmdnames *cmds);\n> > void add_cmdname(struct cmdnames *cmds, const char *name, int len);\n> > /* Here we require that excludes is a sorted list. */\n> > void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes);\n> >\n> > base-commit: 8f7759d2c8c13716bfdb9ae602414fd987787e8d\n>\n"},{"id":"407107","messageId":"nycvar.QRO.7.76.6.2010072349280.50@tvgsbejvaqbjf.bet","threadId":"54364","inReplyTo":"xmqqh7r62ah2.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] help: do not expect built-in commands to be hardlinked","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-10-07T21:49:33Z","receivedAt":"2020-10-07T21:50:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 7 Oct 2020, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n>\n> > ... in the new world order, what we see on disk plus what\n> > we have in the built-in table are the set of subcommands available\n> > to us, and the rule that was valid in the old world order can no\n> > longer be relied upon, and nobody noticed the breakage while\n> > developing or reviewing.\n>\n> >> diff --git a/help.c b/help.c\n> >> index 4e2468a44d..919cbb9206 100644\n> >> --- a/help.c\n> >> +++ b/help.c\n> >> @@ -263,6 +263,8 @@ void load_command_list(const char *prefix,\n> >> \tconst char *env_path = getenv(\"PATH\");\n> >> \tconst char *exec_path = git_exec_path();\n> >>\n> >> +\tload_builtin_commands(prefix, main_cmds);\n> >> +\n> >> \tif (exec_path) {\n> >> \t\tlist_commands_in_dir(main_cmds, exec_path, prefix);\n> >> \t\tQSORT(main_cmds->names, main_cmds->cnt, cmdname_compare);\n>\n> I wondered if we need, after this change, to worry about duplicates,\n> because some Git subcommands, even after they made into a built-in\n> and callable internally, must have on-disk footprint.\n>\n> It turns out that after the post-context in this hunk we do make a\n> call to uniq(main_cmds) so it is fine.\n>\n> This was unexpected to me, as we read only from a single directory\n> \"exec_path\" and the need to call uniq() in the old world order would\n> have meant that readdir in exec_path gave us duplicate entries.\n>\n> In fact, the very original version of load_command_list() did not\n> have this unnecessary call to uniq(). It was introduced in 1f08e5ce\n> (Allow git help work without PATH set, 2008-08-28); perhaps Alex saw\n> 12 years into the future and predicted that we would start needing\n> it ;-)\n>\n> In any case, the patch is good thanks to that existing uniq() call.\n\nYep, I was fully prepared to add that `uniq()` call and was surprised to\nfind it. I guess it was \"for good measure\" because the same commit also\nadded the same `qsort(); uniq()` combo another time, a little further down\nin that function.\n\nNow, what I would have expected you to say when you found the `uniq()`\nfunction is: Johannes, why don't you call `QSORT(); uniq()` after the call\nto `load_builtin_commands()`? After all, `exec_path` and `env_path` might\nboth be `NULL`...\n\nWell, the answer to that question is _not_ \"but without `env_path` nothing\nworks anyway\" even if that would be pretty valid. The answer is that the\n`commands[]` list in `git.c` is already sorted alphabetically.\n\nThanks,\nDscho\n\n>\n> >> diff --git a/help.h b/help.h\n> >> index dc02458855..5871e93ba2 100644\n> >> --- a/help.h\n> >> +++ b/help.h\n> >> @@ -32,6 +32,7 @@ const char *help_unknown_cmd(const char *cmd);\n> >> void load_command_list(const char *prefix,\n> >> \t\t struct cmdnames *main_cmds,\n> >> \t\t struct cmdnames *other_cmds);\n> >> +void load_builtin_commands(const char *prefix, struct cmdnames *cmds);\n> >> void add_cmdname(struct cmdnames *cmds, const char *name, int len);\n> >> /* Here we require that excludes is a sorted list. */\n> >> void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes);\n> >>\n> >> base-commit: 8f7759d2c8c13716bfdb9ae602414fd987787e8d\n>\n"},{"id":"407108","messageId":"pull.745.v2.git.1602107811507.gitgitgadget@gmail.com","threadId":"54364","inReplyTo":"pull.745.git.1602074589460.gitgitgadget@gmail.com","subject":"[PATCH v2] help: do not expect built-in commands to be hardlinked","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-07T21:56:51Z","receivedAt":"2020-10-07T21:56:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen building with SKIP_DASHED_BUILT_INS=YesPlease, the built-in\ncommands are no longer present in the `PATH` as hardlinks to `git`.\n\nAs a consequence, `load_command_list()` needs to be taught to find the\nnames of the built-in commands from elsewhere.\n\nThis only affected the output of `git --list-cmds=main`, but not the\noutput of `git help -a` because the latter includes the built-in\ncommands by virtue of them being listed in command-list.txt.\n\nThe bug was detected via a patch series that turns the merge strategies\nincluded in Git into built-in commands: `git merge -s help` relies on\n`load_command_list()` to determine the list of available merge\nstrategies.\n\nSigned-off-by: Johannes Schindelin \n---\n Fix the command list with SKIP_DASHED_BUILT_INS=YesPlease\n \n In a recent patch series\n [https://lore.kernel.org/git/20201005122646.27994-12-alban.gruin@gmail.com/#r]\n , the merge strategies were converted into built-ins, which is good.\n \n Together with the change where we stop hard-linking the built-in\n commands in CI builds, this broke t9902.199.\n \n The actual root cause is that git merge -s help relies on \n load_command_list() to find all available Git commands, and that\n function expected to find the built-in commands to on the PATH.\n \n Changes since v1:\n \n * Clarified the prefix skipping.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-745%2Fdscho%2Falways-include-builtins-in-commands-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-745/dscho/always-include-builtins-in-commands-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/745\n\nRange-diff vs v1:\n\n 1: 74adb00d59 ! 1: c36a97f436 help: do not expect built-in commands to be hardlinked\n @@ git.c: static void list_builtins(struct string_list *out, unsigned int exclude_o\n +\tconst char *name;\n +\tint i;\n +\n ++\t/*\n ++\t * Callers can ask for a subset of the commands based on a certain\n ++\t * prefix, which is then dropped from the added names. The names in\n ++\t * the `commands[]` array do not have the `git-` prefix, though,\n ++\t * therefore we must expect the `prefix` to at least start with `git-`.\n ++\t */\n +\tif (!skip_prefix(prefix, \"git-\", &prefix))\n -+\t\treturn;\n ++\t\tBUG(\"prefix '%s' must start with 'git-'\", prefix);\n +\n +\tfor (i = 0; i < ARRAY_SIZE(commands); i++)\n +\t\tif (skip_prefix(commands[i].cmd, prefix, &name))\n\n\n git.c | 19 +++++++++++++++++++\n help.c | 2 ++\n help.h | 1 +\n 3 files changed, 22 insertions(+)\n\ndiff --git a/git.c b/git.c\nindex d51fb5d2bf..48fc81b92f 100644\n--- a/git.c\n+++ b/git.c\n@@ -641,6 +641,25 @@ static void list_builtins(struct string_list *out, unsigned int exclude_option)\n \t}\n }\n \n+void load_builtin_commands(const char *prefix, struct cmdnames *cmds)\n+{\n+\tconst char *name;\n+\tint i;\n+\n+\t/*\n+\t * Callers can ask for a subset of the commands based on a certain\n+\t * prefix, which is then dropped from the added names. The names in\n+\t * the `commands[]` array do not have the `git-` prefix, though,\n+\t * therefore we must expect the `prefix` to at least start with `git-`.\n+\t */\n+\tif (!skip_prefix(prefix, \"git-\", &prefix))\n+\t\tBUG(\"prefix '%s' must start with 'git-'\", prefix);\n+\n+\tfor (i = 0; i < ARRAY_SIZE(commands); i++)\n+\t\tif (skip_prefix(commands[i].cmd, prefix, &name))\n+\t\t\tadd_cmdname(cmds, name, strlen(name));\n+}\n+\n #ifdef STRIP_EXTENSION\n static void strip_extension(const char **argv)\n {\ndiff --git a/help.c b/help.c\nindex 4e2468a44d..919cbb9206 100644\n--- a/help.c\n+++ b/help.c\n@@ -263,6 +263,8 @@ void load_command_list(const char *prefix,\n \tconst char *env_path = getenv(\"PATH\");\n \tconst char *exec_path = git_exec_path();\n \n+\tload_builtin_commands(prefix, main_cmds);\n+\n \tif (exec_path) {\n \t\tlist_commands_in_dir(main_cmds, exec_path, prefix);\n \t\tQSORT(main_cmds->names, main_cmds->cnt, cmdname_compare);\ndiff --git a/help.h b/help.h\nindex dc02458855..5871e93ba2 100644\n--- a/help.h\n+++ b/help.h\n@@ -32,6 +32,7 @@ const char *help_unknown_cmd(const char *cmd);\n void load_command_list(const char *prefix,\n \t\t struct cmdnames *main_cmds,\n \t\t struct cmdnames *other_cmds);\n+void load_builtin_commands(const char *prefix, struct cmdnames *cmds);\n void add_cmdname(struct cmdnames *cmds, const char *name, int len);\n /* Here we require that excludes is a sorted list. */\n void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes);\n\nbase-commit: 8f7759d2c8c13716bfdb9ae602414fd987787e8d\n-- \ngitgitgadget\n"},{"id":"407110","messageId":"xmqqh7r5znbo.fsf@gitster.c.googlers.com","threadId":"54364","inReplyTo":"nycvar.QRO.7.76.6.2010072349280.50@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] help: do not expect built-in commands to be hardlinked","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-07T22:24:43Z","receivedAt":"2020-10-07T22:24:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Now, what I would have expected you to say when you found the `uniq()`\n> function is: Johannes, why don't you call `QSORT(); uniq()` after the call\n> to `load_builtin_commands()`? After all, `exec_path` and `env_path` might\n> both be `NULL`...\n\nNah, you are expecting too much out of me. I didn't ask because I\nknew we didn't need to, and I didn't particularly care if you lucked\nout or if you had the same understanding as I had how you arrived at\nthe right solution ;-)\n\n"}]}