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

Junio C Hamano
2024-05-24T15:33:48Z

Ian Wienand <iwienand@redhat.com> writes:

> On Thu, May 23, 2024 at 11:09:01PM -0700, Junio C Hamano wrote:
>> So with this applied on top of the topic, 'seen' does pass with
>> SHELL_PATH set to say /bin/dash, but this still fails CI jobs on
>> Windows.
>
> Sigh, it looks like windows uses prepare_shell_cmd() instead. I think
> it's still probably valid to dump the full command there for the same
> reasons, which I'll add in v4.

Ahh, it's the large conditional compilation in start_command().

On non-Windows side, we just do

if (prepare_cmd(&argv, cmd) < 0) {
failed_errno = errno;
...
goto end_of_spawn;
}
... pipe(), fork(), and exec() ...

but on Windows side, we have

if (cmd->git_cmd)
cmd->args.v = prepare_git_cmd(&nargv, sargv);
else if (cmd->use_shell)
cmd->args.v = prepare_shell_cmd(&nargv, sargv);

cmd->pid = mingw_spawnvpe(cmd->args.v[0], cmd->args.v,
(char**) cmd->env.v,
cmd->dir, fhin, fhout, fherr);

And the thing is, prepare_cmd() already has

if (cmd->git_cmd) {
prepare_git_cmd(out, cmd->args.v);
} else if (cmd->use_shell) {
prepare_shell_cmd(out, cmd->args.v);
} else {
strvec_pushv(out, cmd->args.v);
}

I am wondering (and not suggesting to do this as a part of this
series, but to decide if we should leave a left-over-bits note here)
if the Windows side can be updated to also use prepare_cmd(). Do
folks a lot more familiar with Windows than I (cc'ed j6t and dscho)
have comments on this?

Anyway.

If you unconditionally add the printf's to both prepare_cmd() and
prepare_shell_cmd(), you'll see two printf output on a non-Windows
platform if we are spawning a shell cmd.

It appears that the best place to "show the prepared command" is
*NOT* in any of these prepare_* helper functions, but after these
two callers in start_command() receives the prepared command from
these helpers, namely:

diff --git c/run-command.c w/run-command.c
index 1b821042b4..9d0fa6afe4 100644
--- c/run-command.c
+++ w/run-command.c
@@ -745,6 +745,7 @@ int start_command(struct child_process *cmd)
error_errno("cannot run %s", cmd->args.v[0]);
goto end_of_spawn;
}
+ /* Here show argv in the trace */

if (pipe(notify_pipe))
notify_pipe[0] = notify_pipe[1] = -1;
@@ -912,6 +913,7 @@ int start_command(struct child_process *cmd)
cmd->args.v = prepare_git_cmd(&nargv, sargv);
else if (cmd->use_shell)
cmd->args.v = prepare_shell_cmd(&nargv, sargv);
+ /* Here show cmd->args.v in the trace */

cmd->pid = mingw_spawnvpe(cmd->args.v[0], cmd->args.v,
(char**) cmd->env.v,

We would not have to do so and instead have trace-printf(s) only in
prepare_cmd() if/when the Windows side is updated to stop switching
between the prepare_git/prepare_shell and instead let prepare_cmd()
do the switching, (which may require an update to prepare_cmd() if
needed). But until that happens, logging at the caller seems to be
the best approach among equally bad ones.

Thanks.