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