From: Jeff King <peff@peff.net>
Date: Mon, 15 Sep 2025 18:08:42 GMT
Subject: Re: [PATCH] git: run alias subprocess according to the API
Message-ID: <20250915180842.GA642496@coredump.intra.peff.net>
In-Reply-To: <xmqqms6wi08y.fsf@gitster.g>

On Mon, Sep 15, 2025 at 01:33:01AM -0700, Junio C Hamano wrote:

> > From: Kristoffer Haugsbakk <code@khaugsbakk.name>
> >
> > `run-command.h` tells us that this is how we should run git(1) commands.
> 
> Perhaps that comment needs to be updated at the same time?
> 
> >     The only other thing I found was in `upload-pack.c`.
> >     
> >         strvec_push(&pack_objects.args, "git");
> >     
> >     But that one is intentional; this is inside an else-block and the
> >     if-block has `pack_objects.git_cmd = 1;`.  And attempts to refactor it
> >     broke `t5544-pack-objects-hook.sh`.
> 
> Hmph, doesn't it indicate that it is wrong to use ".git_cmd = 1"
> unconditionally, no?  I.e., the instruction you found might make it
> sound as if the only difference is to spawn "git" directly or doing
> it via the "shell", but because it is not, the result of your update
> broke that test, right?
> 
> So, we want to tell programmers when to use it, with the comment
> that says "most of the time, if you are spawning a "git" command,
> use the .git_cmd = 1 mechanism, but if you are doing X or Y then,
> spawn that "git" command just like any other programs spawned via
> the shell, because of such and such reasons", no?

I'm not sure there _is_ any reason to use a raw "git" instead of
".git_cmd". The case in upload-pack is _not_ running a git command
directly. It is passing the name of a git command to the hook. So you
end up running:

  /path/to/my/hook git pack-objects ...args...

So I don't think there is any X or Y, and the rule can just be "you
should use .git_cmd".

That said, I do not think it is particularly important to use .git_cmd;
it really is just prepending "git" to argv. It does also override
.use_shell, but it is nonsense to set both flags in the first place
(which is something we could perhaps flag with a BUG() if we cared to).
But I have no problem with preferring .git_cmd to "git" as a style
preference.

-Peff