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

From: Junio C Hamano <gitster@pobox.com>
Date: May 23, 2024, 15:29 UTC
Message-ID: <xmqqpltcwnqm.fsf@gitster.g>
In-Reply-To: <20240523043806.1223032-3-iwienand@redhat.com>

Ian Wienand <iwienand@redhat.com> writes:

Show 11 quoted lines

> 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.

Show 9 quoted lines

> 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 ...

Show 8 quoted lines

> 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.

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;
>  }

Nice addition that is obviously correct.

Show 14 quoted lines

> 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.