Ian Wienand <iwienand@xxxxxxxxxx> writes:
> 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"?
> 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.
> 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.
