threads / patch / 64147

patchgit: run alias subprocess according to the API

Subject: [PATCH] git: run alias subprocess according to the API

## tl;dr

5 messages between Sep 14, 2025 and Sep 15, 2025. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

kristofferhaugsbakk@fastmail.com· Sep 14, 2025, 09:32 UTC · lore
From: Kristoffer Haugsbakk <code@khaugsbakk.name>
`run-command.h` tells us that this is how we should run git(1) commands.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
    From: https://lore.kernel.org/git/a8874cde-cd00-41b0-ba41-ab2fd52ce45d@inria.fr/T/#m2667cc5ffed44fb7b73d349b1e6dba281e3e0384
    
    The alias question.
    
    But we just end up running what is named `git`.  So it has no bearing on
    that question.
    
    § Code spelunking
    
    I traced the push-`git`-to-command code back to `argv_array_push(&args,
    "git");` which lead me back to ee4512ed481 (trace2: create new combined
    trace facility, 2019-02-22).  It didn’t look relevant.
    
    § Other code
    
    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`.
 git.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to git.c +1 −1
diff --git a/git.c b/git.c
index d020eef021c..f16a8fbb55d 100644
--- a/git.c
+++ b/git.c
@@ -825,18 +825,18 @@ static int run_argv(struct strvec *args)
 			 * process will log the actual verb when it runs.
 			 */
 			trace2_cmd_name("_run_git_alias_");
 
 			commit_pager_choice();
 
-			strvec_push(&cmd.args, "git");
 			for (size_t i = 0; i < args->nr; i++)
 				strvec_push(&cmd.args, args->v[i]);
 
 			trace_argv_printf(cmd.args.v, "trace: exec:");
 
+			cmd.git_cmd = 1;
 			/*
 			 * if we fail because the command is not found, it is
 			 * OK to return. Otherwise, we just pass along the status code.
 			 */
 			cmd.silent_exec_failure = 1;
 			cmd.clean_on_exit = 1;

base-commit: ab427cd991100e94792fce124b0934135abdea4b

-- 
2.51.0.16.gcd94ab5bf81
Junio C Hamano· Sep 15, 2025, 08:33 UTC · re: kristofferhaugsbakk@fastmail.com · lore

Re: [PATCH] git: run alias subprocess according to the API

kristofferhaugsbakk@fastmail.com writes:
> 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?
Show 7 quoted lines
>     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?

Show 29 quoted lines
>  git.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git.c b/git.c
> index d020eef021c..f16a8fbb55d 100644
> --- a/git.c
> +++ b/git.c
> @@ -825,18 +825,18 @@ static int run_argv(struct strvec *args)
>  			 * process will log the actual verb when it runs.
>  			 */
>  			trace2_cmd_name("_run_git_alias_");
>  
>  			commit_pager_choice();
>  
> -			strvec_push(&cmd.args, "git");
>  			for (size_t i = 0; i < args->nr; i++)
>  				strvec_push(&cmd.args, args->v[i]);
>  
>  			trace_argv_printf(cmd.args.v, "trace: exec:");
>  
> +			cmd.git_cmd = 1;
>  			/*
>  			 * if we fail because the command is not found, it is
>  			 * OK to return. Otherwise, we just pass along the status code.
>  			 */
>  			cmd.silent_exec_failure = 1;
>  			cmd.clean_on_exit = 1;
>
> base-commit: ab427cd991100e94792fce124b0934135abdea4b
Jeff King· Sep 15, 2025, 18:08 UTC · re: Junio C Hamano · lore

Re: [PATCH] git: run alias subprocess according to the API

On Mon, Sep 15, 2025 at 01:33:01AM -0700, Junio C Hamano wrote:
Show 25 quoted lines
> > 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
Junio C Hamano· Sep 15, 2025, 19:09 UTC · re: Jeff King · lore

Re: [PATCH] git: run alias subprocess according to the API

Jeff King <peff@peff.net> writes:
Show 16 quoted lines
> 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.

Ah, OK. I just was confused by the explanation in the middle that appeared to be saying that they are not truly equivalent. If not, then I am perfectly fine.

I would actually prefer to get rid of the .git_cmd member altogether to make it simpler to code. The run_command() function should be able to internally decide to short-circuit and invoke "git" directly without the shell by looking at the first element of the argv[] array, One less thing for human programmers to worry about.

Jeff King· Sep 15, 2025, 19:31 UTC · re: Junio C Hamano · lore

Re: [PATCH] git: run alias subprocess according to the API

On Mon, Sep 15, 2025 at 12:09:14PM -0700, Junio C Hamano wrote:
Show 22 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > 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.
> 
> Ah, OK.  I just was confused by the explanation in the middle that
> appeared to be saying that they are not truly equivalent.  If not,
> then I am perfectly fine.
Yeah, I think Kristoffer's explanation was just confusing.
Show 5 quoted lines
> I would actually prefer to get rid of the .git_cmd member altogether
> to make it simpler to code.  The run_command() function should be
> able to internally decide to short-circuit and invoke "git" directly
> without the shell by looking at the first element of the argv[]
> array,  One less thing for human programmers to worry about.

I don't have any problem with that direction. In theory we might do something special for a .git_cmd (maybe run a builtin directly?), but in 20 years of run-command we have not bothered to do anything besides prepend "git".

I think we already should skip the shell in that case, as the caller would not generally be setting .use_shell. And if they are, they are perhaps trying to do something clever and we should not override it. So I think you could literally just drop .git_cmd, replace all the callers with pushing "git" at the start, and be done.

-Peff

← back to recent threads