Re: [PATCH] alias: document caveats and add trace of prepared command

From: Junio C Hamano <gitster@pobox.com>
Date: May 22, 2024, 16:07 UTC
Message-ID: <xmqq8r017rtw.fsf@gitster.g>
In-Reply-To: <20240522024133.1108005-1-iwienand@redhat.com>

Ian Wienand <iwienand@redhat.com> writes:

Show 10 quoted lines

> For a "split" alias, e.g. test = "!echo $*" you will see
>
>  $ GIT_TRACE=1 git test hello
>  11:00:45.959420 git.c:755               trace: exec: git-test hello
>  11:00:45.959737 run-command.c:657       trace: run_command: git-test hello
>  11:00:45.961424 run-command.c:657       trace: run_command: 'echo $*' hello
>  11:00:45.961528 run-command.c:437       trace: prepare_cmd: /bin/sh -c 'echo $* "$@"' 'echo $*' hello
>  hello hello
>
> which clearly shows you the appended "$@".

A question and a comment on this part.

Who are "you" in this context?

It is somewhat surprising if we do not consistently add "$@" (or do an equivalent), as the point of adding "$@" is to allow an alias to specify "the initial part of a command line", e.g.

	[alias] lg = log --oneline

to let you say "git lg" to mean "git log --oneline" and also you can say "git lg --graph" to mean "git log --oneline --graph". In other words, what you type after "git lg" makes "the rest of the command line", while alias gives "the initial part".

Are you sure that with

	[alias] lg = log

you get the rest of the command line ignored, in other words, if you say "git lg --oneline", it does not do "git log --oneline"?

Show 38 quoted lines

>  Documentation/config/alias.txt | 26 +++++++++++++++++++++-----
>  run-command.c                  |  1 +
>  2 files changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/config/alias.txt b/Documentation/config/alias.txt
> index 01df96fab3..a3f090d79d 100644
> --- a/Documentation/config/alias.txt
> +++ b/Documentation/config/alias.txt
> @@ -21,8 +21,24 @@ If the alias expansion is prefixed with an exclamation point,
>  it will be treated as a shell command.  For example, defining
>  `alias.new = !gitk --all --not ORIG_HEAD`, the invocation
>  `git new` is equivalent to running the shell command
> -`gitk --all --not ORIG_HEAD`.  Note that shell commands will be
> -executed from the top-level directory of a repository, which may
> -not necessarily be the current directory.
> -`GIT_PREFIX` is set as returned by running `git rev-parse --show-prefix`
> -from the original current directory. See linkgit:git-rev-parse[1].
> +`gitk --all --not ORIG_HEAD`.  Note:
> ++
> +* Shell commands will be executed from the top-level directory of a
> +  repository, which may not necessarily be the current directory.
> +* `GIT_PREFIX` is set as returned by running `git rev-parse --show-prefix`
> +  from the original current directory. See linkgit:git-rev-parse[1].
> +* Single commands will be executed directly.  Commands that can be "split"
> +  (contain whitespace or shell-reserved characters) will be run as shell
> +  commands via an argument to `sh -c`.
> +* When arguments are present to a "split" command running in a shell,
> +  the shell command will have `"$@"` appended.  The first non-command
> +  argument to `sh -c` (i.e. `$0` to the command) is always the alias
> +  string, and other user specified arguments will follow.
> +** This may initially be confusing if your command references argument
> +   variables or is not expecting the presence of `"$@"`.  For example:
> +   `alias.echo = "!echo $1"` when run as `git echo arg` will execute
> +   `sh -c "echo $1 $@" "echo $1" "1"` resulting in output `1 1`.
> +   An alias `alias.for = "!for i in 1 2 3; do echo $i; done"` will fail
> +   if any arguments are specified to `git for` as the appended `"$@"` will
> +   create invalid shell syntax.  Setting `GIT_TRACE=1` can help debug
> +   the command being run.

The above does a bit too many things in a single patch to be reviewable. Perhaps make the above change in two patches?

 (1) Reformulate the existing test into "Note:" followed by a
     bulletted list.

 (2) Add new items to the updated and easier-to-read form prepared
     by the first patch.

You have a lengthy new text in the documentation to help confused users, but it looks to me that it places too much stress on the mechanics (i.e. '$@' is added to the command line) without saying much about the intent (i.e. you need to use '$@' to allow the command line invocation to supply "the rest of the command line" when you are running the alias via the shell). I've learned over the course of this project that readers learn better when the intent behind the mechanics is given in an understandable form.

I think the idea is "we want the 'concatenation of what the alias supplies and what the user gives when invoking the alias from the command line' to work sensibly". The most generic way to do so when processing "git lg --graph -3" with "[alias] lg = !git log --oneline" is

    sh -c 'git log --oneline "$@"' - '--graph' '-3'

(readers can try this at home by adding 'echo ' before 'log'). We may try to "optimize" the most generic pattern when there is no need to use "sh -c" go a more direct route. For example, if you have

    [alias] !foo = bar

then there is no need to use "sh -c" in the first place. "git foo baz quux" should behave just like when you said "sh bar baz quux" on the command line, i.e. execute your shell script "bar" and give "baz" and "quux" as two arguments to that script. We can prepare argv[] = ("sh", "bar", "baz", "quux") and fork-exec that, so the command line the underlying exec(2) call sees may not have "$@" (and it will not have "-c", either).

You can undo the "optimization" and rewrite the command line to

    sh -c 'bar "$@"' - 'baz' 'quux'

and it should mean the same thing.

Show 11 quoted lines

> diff --git a/run-command.c b/run-command.c
> index 1b821042b4..36b2b2f194 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -435,6 +435,7 @@ static int prepare_cmd(struct strvec *out, const struct child_process *cmd)
>  		}
>  	}
>  
> +	trace_argv_printf(&out->v[1], "trace: prepare_cmd:");
>  	return 0;
>  }

Hmph, we do not have much tests that look for 'trace: foo' in our test suite, but t0014 seems to use it. Don't we need to cover this new trace output in the test in the same patch (probably becomes patch 3 after the two documentation changes)?

Thanks.