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

[Ian Wienand <iwienand@redhat.com>]

On Thu, May 23, 2024 at 08:29:21AM -0700, Junio C Hamano wrote:
> > 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.

Right; I was seeing this in a more convoluted way via our tool but
essentially the same issue.  I was just looking for the simplest thing
that also gave the syntax error output, which I thought was something
people might search for (the "unexpected "$@" stuff).

Should I just leave as is?

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

OK, should this perhaps just look for '^trace: prepare_cmd.*'?

My initial thinking was to enforce seeing the "$@" appended, but
perhaps that is implementation details that don't really need to be
covered; the interesting thing is we show the person tracing the full
command as constructed, so this is useful just to ensure the
tracepoint remains in place?

-i