Re: [PATCH v6 04/11] run-command: use the async-signal-safe execv instead of execvp

[Junio C Hamano <gitster@pobox.com>]

Brandon Williams <bmwill@google.com> writes:

> @@ -238,6 +238,12 @@ static void prepare_cmd(struct argv_array *out, const struct child_process *cmd)
>  	if (!cmd->argv[0])
>  		die("BUG: command is empty");
>  
> +	/*
> +	 * Add SHELL_PATH so in the event exec fails with ENOEXEC we can
> +	 * attempt to interpret the command with 'sh'.
> +	 */
> +	argv_array_push(out, SHELL_PATH);
> +
>  	if (cmd->git_cmd) {
>  		argv_array_push(out, "git");
>  		argv_array_pushv(out, cmd->argv);


So, given "cat-file", "-t", "HEAD" with cmd->git_cmd == TRUE, this
will now prepare ("/bin/sh", "git", "cat-file", "-t", "HEAD", NULL) in
the argv-array, and then ...

> @@ -246,6 +252,20 @@ static void prepare_cmd(struct argv_array *out, const struct child_process *cmd)
>  	} else {
>  		argv_array_pushv(out, cmd->argv);
>  	}
> +
> +	/*
> +	 * If there are no '/' characters in the command then perform a path
> +	 * lookup and use the resolved path as the command to exec.  If there
> +	 * are no '/' characters or if the command wasn't found in the path,
> +	 * have exec attempt to invoke the command directly.
> +	 */
> +	if (!strchr(out->argv[1], '/')) {
> +		char *program = locate_in_PATH(out->argv[1]);
> +		if (program) {
> +			free((char *)out->argv[1]);
> +			out->argv[1] = program;
> +		}
> +	}

... turn the first element from "git" to "/usr/bin/git", i.e.

	("/bin/sh", "/usr/bin/git", "cat-file", "-t", "HEAD", NULL)

which ...

>  #endif
>  
> @@ -445,7 +465,15 @@ int start_command(struct child_process *cmd)
>  			}
>  		}
>  
> -		sane_execvp(argv.argv[0], (char *const *) argv.argv);
> +		/*
> +		 * Attempt to exec using the command and arguments starting at
> +		 * argv.argv[1].  argv.argv[0] contains SHELL_PATH which will
> +		 * be used in the event exec failed with ENOEXEC at which point
> +		 * we will try to interpret the command using 'sh'.
> +		 */
> +		execv(argv.argv[1], (char *const *) argv.argv + 1);

... first is given without the leading "/bin/sh", as the end-user
intended (sort of), but if it fails

> +		if (errno == ENOEXEC)
> +			execv(argv.argv[0], (char *const *) argv.argv);

"/bin/sh" tries to run "/usr/bin/git" that was not executable (well,
the one in "usr/bin/" would have +x bit, but let's pretend that we
are trying to run one from bin-wrappers/ and somehow forgot +x bit)?

I think all of that is sensible, but there is one "huh?" I can't
figure out.  Typically we do "sh -c git cat-file -t HEAD" but this
lacks the "-c" (cf. the original prepare_shell_cmd()); why do we not
need it in this case?

Thanks.