git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Derrick Stolee <stolee@gmail.com>
To: Junio C Hamano <gitster@pobox.com>, Jeff King <peff@peff.net>
Cc: git@vger.kernel.org, Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH] Makefile: mark git-maintenance as a builtin
Date: Thu, 3 Dec 2020 08:51:56 -0500	[thread overview]
Message-ID: <ddbbd576-fb5e-f913-a7fd-e1590d40fc8c@gmail.com> (raw)
In-Reply-To: <xmqqa6uvlt6w.fsf@gitster.c.googlers.com>

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

      reply	other threads:[~2020-12-03 13:57 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-02  2:34 [PATCH] Makefile: mark git-maintenance as a builtin Jeff King
2020-12-02 22:49 ` Junio C Hamano
2020-12-03 13:51   ` Derrick Stolee [this message]

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

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ddbbd576-fb5e-f913-a7fd-e1590d40fc8c@gmail.com \
    --to=stolee@gmail.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    /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.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).