[PATCH] Makefile: mark git-maintenance as a builtin

[PATCH] Makefile: mark git-maintenance as a builtin

[Jeff King]

We normally get the list of builtin commands by expanding BUILTIN_OBJS.
But for commands which are embedded inside another's source file (e.g.,
cmd_show() in builtin/log.c), the Makefile needs to be told explicitly
about them.

Since cmd_maintenance() is inside buitin/gc.c, it should be listed
explicitly in the BUILT_INS list in the Makefile. Not doing so isn't
_too_ tragic, as it simply means we will not make a git-maintenance
symlink in libexec/git-core. Since we encourage people to use the "git
foo" form, even in scripts which have put libexec into their PATH,
nobody seems to have noticed.

Signed-off-by: Jeff King <peff@peff.net>
---
I don't really care that much. I just happened to notice there is a
git-maintenance pattern in .gitignore which will not ever trigger.

I could actually see an argument that this is not worth doing for new
commands. The dashed forms of the other commands have worked for a long
time, so losing them would be a regression. But since git-maintenance
would never have worked, presumably everybody who cares is using the
recommended "git maintenance" form already.

So I'm happy with that direction, too, but in that case we should
probably remove the .gitignore entry. :)

 Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index d3a531d3c6..1e507b9de0 100644
--- a/Makefile
+++ b/Makefile
@@ -769,6 +769,7 @@ BUILT_INS += git-cherry-pick$X
 BUILT_INS += git-format-patch$X
 BUILT_INS += git-fsck-objects$X
 BUILT_INS += git-init$X
+BUILT_INS += git-maintenance$X
 BUILT_INS += git-merge-subtree$X
 BUILT_INS += git-restore$X
 BUILT_INS += git-show$X
-- 
2.29.2.894.g2dadb8c6b8

Re: [PATCH] Makefile: mark git-maintenance as a builtin

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> We normally get the list of builtin commands by expanding BUILTIN_OBJS.
> But for commands which are embedded inside another's source file (e.g.,
> cmd_show() in builtin/log.c), the Makefile needs to be told explicitly
> about them.
>
> Since cmd_maintenance() is inside buitin/gc.c, it should be listed
> explicitly in the BUILT_INS list in the Makefile. Not doing so isn't
> _too_ tragic, as it simply means we will not make a git-maintenance
> symlink in libexec/git-core. Since we encourage people to use the "git
> foo" form, even in scripts which have put libexec into their PATH,
> nobody seems to have noticed.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I don't really care that much. I just happened to notice there is a
> git-maintenance pattern in .gitignore which will not ever trigger.
>
> I could actually see an argument that this is not worth doing for new
> commands. The dashed forms of the other commands have worked for a long
> time, so losing them would be a regression. But since git-maintenance
> would never have worked, presumably everybody who cares is using the
> recommended "git maintenance" form already.

I do not care too deeply, but being inconsistent means users have to
remember which ones can still be used in the dashed form when they
use the PATH=$(git --exec-path):$PATH escape hatch, and which ones
cannot.  It strongly discourages folks from writing new scripts with
dashed form "git" commands, which might be a good thing, but it goes
against our commitment to keep dashed form working, so...

> So I'm happy with that direction, too, but in that case we should
> probably remove the .gitignore entry. :)
>
>  Makefile | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/Makefile b/Makefile
> index d3a531d3c6..1e507b9de0 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -769,6 +769,7 @@ BUILT_INS += git-cherry-pick$X
>  BUILT_INS += git-format-patch$X
>  BUILT_INS += git-fsck-objects$X
>  BUILT_INS += git-init$X
> +BUILT_INS += git-maintenance$X
>  BUILT_INS += git-merge-subtree$X
>  BUILT_INS += git-restore$X
>  BUILT_INS += git-show$X

Re: [PATCH] Makefile: mark git-maintenance as a builtin

[Derrick Stolee]

On 12/2/2020 5:49 PM, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
>> We normally get the list of builtin commands by expanding BUILTIN_OBJS.
>> But for commands which are embedded inside another's source file (e.g.,
>> cmd_show() in builtin/log.c), the Makefile needs to be told explicitly
>> about them.

TIL! Thanks for noticing.

>> Since cmd_maintenance() is inside buitin/gc.c, it should be listed
>> explicitly in the BUILT_INS list in the Makefile. Not doing so isn't
>> _too_ tragic, as it simply means we will not make a git-maintenance
>> symlink in libexec/git-core. Since we encourage people to use the "git
>> foo" form, even in scripts which have put libexec into their PATH,
>> nobody seems to have noticed.
>>
>> Signed-off-by: Jeff King <peff@peff.net>
>> ---
>> I don't really care that much. I just happened to notice there is a
>> git-maintenance pattern in .gitignore which will not ever trigger.
>>
>> I could actually see an argument that this is not worth doing for new
>> commands. The dashed forms of the other commands have worked for a long
>> time, so losing them would be a regression. But since git-maintenance
>> would never have worked, presumably everybody who cares is using the
>> recommended "git maintenance" form already.
> 
> I do not care too deeply, but being inconsistent means users have to
> remember which ones can still be used in the dashed form when they
> use the PATH=$(git --exec-path):$PATH escape hatch, and which ones
> cannot.  It strongly discourages folks from writing new scripts with
> dashed form "git" commands, which might be a good thing, but it goes
> against our commitment to keep dashed form working, so...

As long as we continue to support the dashed form, we should be
consistent with other builtins. Sorry for not noticing that this
would be different than previous builtins I've added.

This patch LGTM.

Thanks,
-Stolee