Ian Wienand <iwienand@xxxxxxxxxx> writes:
> This adds a trace point in prepare_cmd, so we can see the actual
> command being run without having to resort to strace/code inspection.
>
> e.g. "test = !echo" when run under GIT_TRACE will show:
>
> $ GIT_TRACE=1 git test hello
> 10:58:56.877234 git.c:755 trace: exec: git-test hello
> 10:58:56.877382 run-command.c:657 trace: run_command: git-test hello
> 10:58:56.878655 run-command.c:657 trace: run_command: echo hello
> 10:58:56.878747 run-command.c:437 trace: prepare_cmd: /usr/bin/echo hello
> hello
Nice.
> A "split" alias, e.g. test = "!echo $*" will show the shell wrapping
> and appending of "$@".
>
> $ 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
Nice again.
But ...
> For example, this can be very helpful when an alias is giving you an
> unexpected syntax error that is very difficult figure out from only
> the run_command trace point, e.g.
>
> test = "!for i in 1 2 3; do echo $i; done"
>
> will fail if there is an argument given, we can see why from the
> output.
... if the reader truly understands "the alias gives the command and
its leading arguments, to which the invocation can supply even more
arguments", the reader wouldn't be writing such a command line to
begin with, no?
So I find the example a bit suboptimal. Hopefully additional
explanation in patch 2/3 stressed on that point well enough with
much more stress than it talks about the implementation detail of
using "sh -c" and "$@", so that readers who read it would not even
dream of writing such an alias in the first place.
> 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;
> }
Nice addition that is obviously correct.
> diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh
> index 95568342be..75c8763a6c 100755
> --- a/t/t0014-alias.sh
> +++ b/t/t0014-alias.sh
> @@ -44,4 +44,11 @@ test_expect_success 'run-command formats empty args properly' '
> test_cmp expect actual
> '
>
> +test_expect_success 'tracing a shell alias with arguments shows full prepared command' '
> + git config alias.echo "!echo \$*" &&
> + env GIT_TRACE=1 git echo argument 2>output &&
> + cp output /tmp/output &&
> + test_grep "^trace: prepare_cmd: /bin/sh -c '\''echo \$\* \"\$@\"" output
> +'
This is probably too specific search string, I suspect, given that
runcommand.c:prepare_shell_cmd() uses SHELL_PATH or "sh" so if your
SHELL_PATH is anything but /bin/sh (or if you are unlucky enough to
be running this test on Windows), the pattern would not match.
You'd want to loosen it a bit, perhaps with "/bin/sh" -> ".*", as
the rest of the output are expected to stay constant.
By the way, common to this step and also to the previous step,
you'd want to use
echo "$@"
instead of
echo $*
to encourage better variable reference hygiene. It makes difference
when any arguments given to "e" has whitespace, i.e.
$ sh -c 'echo $*' - a 'b c'
a b c
$ sh -c 'echo "$@"' - a 'b c'
a b c
Thanks.
