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
