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

From: Junio C Hamano <gitster@pobox.com>
Date: May 24, 2024, 15:33 UTC
Message-ID: <xmqqsey7ns14.fsf@gitster.g>
In-Reply-To: <ZlA_WIn1y7Oo_D88@fedora19.localdomain>

Ian Wienand <iwienand@redhat.com> writes:

Show 8 quoted lines

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