Re: [PATCH v3 3/3] run-command: show prepared command

[Junio C Hamano <gitster@pobox.com>]

Ian Wienand <iwienand@redhat.com> 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.