Re: [PATCH 4/4] drop git_exec_path() from non-Git commands' PATH

From: Patrick Steinhardt <ps@pks.im>
Date: May 21, 2025, 08:27 UTC
Message-ID: <aC2OaeLYJQAOE_S1@pks.im>
In-Reply-To: <20250520193506.95199-5-ben.knoble+github@gmail.com>

On Tue, May 20, 2025 at 03:34:58PM -0400, D. Ben Knoble wrote:

Show 11 quoted lines

> We setup_path() with git_exec_path() unconditionally (8e3462837b (Modify
> setup_path() to only add git_exec_path() to PATH, 2009-01-18)) when Git
> starts; as a result, all child processes see Git's exec-path when run,
> including editors and other programs that don't need it [1]. This can
> cause confusion for such programs or shells, especially when they rely
> on finding "git" in PATH to locate other nearby directories, in a
> similar vein as a0b4507ef7 (stop putting argv[0] dirname at front of
> PATH, 2015-04-22) solved.
> 
> Since we only need this for finding git-* subprocesses, drop it from
> child processes that aren't Git commands.

I agree with what Junio mentioned in a parallel thread, especially around Git hooks. The expectation there is that those may execute other Git commands, and that should typically be using the same execution environment as the original Git command that has been invoking the hook. So refining this patch so that the mechanism is opt-in probably makes sense.

    A slight tangent: I wonder whether it is even required nowadays to
    adapt PATH at all anymore. As far as I understand this was a
    requirement back when people still executed dashed binaries
    directly. But nowadays scripts don't really do that anymore, but
    instead use the git binary. And that one doesn't need PATH to be
    adapted at all, as it knows to listen to GIT_EXEC_PATH and its
    built-in path anyway.

    So I have to wonder whether this is something that we can deprecate
    to finalize the migration away from dashed builtins. There probably
    are edge cases though. Shell scripts for example still execute ".
    git-sh-setup", which I think relies on PATH to resolve the script's
    location.

    In any case, this is of course outside of the scope of this patch
    series.

> [1]: https://public-inbox.org/git/CALnO6CDtGRRav8zK2GKi1oHTZWrHFTxZNmnOWu64-ab+oY3_Lw@mail.gmail.com/
> 
> Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
> Suggested-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Same comment regarding the order of trailers.

Show 10 quoted lines

> diff --git a/run-command.c b/run-command.c
> index dee6ae3e62..8a8b5c8455 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -448,11 +448,51 @@ static int prepare_cmd(struct strvec *out, const struct child_process *cmd)
>  	return 0;
>  }
>  
> -static char **prep_childenv(const char *const *deltaenv)
> +static void remove_git_exec_path(struct string_list_item *path_item) {

Style: the curly brace should be on its own line.

Show 7 quoted lines

> +	struct strbuf buf = STRBUF_INIT;
> +	const char *exec_path = git_exec_path();
> +	size_t exec_len = strlen(exec_path);
> +	char *b, *p;
> +
> +	/* Value comes from environ; we should not modify it directly. But
> +	 * strbuf copies data, so we now have our own playground. */

Style:

    /*
     * Multi-line comments should be written like this, with their
     * opening and closing parts on their own lines.
     */

Show 7 quoted lines

> +	strbuf_addstr(&buf, (const char *)path_item->util);
> +
> +	/* skip past "PATH=" to start search */
> +	p = strchr(buf.buf, '=');
> +	if (!p || !*(p + 1))
> +		return;
> +	b = p + 1;

You could use `if (!skip_prefix(buf.buf, "PATH=", &p))` instead.

Show 22 quoted lines

> +
> +	for (p = strstr(b, exec_path); p; p = strstr(p, exec_path)) {
> +		if ((p[exec_len] && p[exec_len] != PATH_SEP) || (p != b && p[-1] != PATH_SEP))
> +			p += exec_len; /* false positive, skip */
> +		else {
> +			size_t offset = p - buf.buf, delete_len = exec_len;
> +			if (p != b) {
> +				/* include the preceding path separator */
> +				offset--;
> +				delete_len++;
> +			} else if (p[exec_len] == PATH_SEP) {
> +				/* include the path separator following GIT_EXEC_PATH */
> +				delete_len++;
> +			}
> +			strbuf_splice(&buf, offset, delete_len, "", 0);
> +		}
> +	}
> +
> +	/* Overwrite PATH value with new (owned) data. This leaks memory because
> +	 * the only future owner is a char** childenv, which is freed, but whose
> +	 * contents are not (because most of them come from environ). */
> +	path_item->util = (void *)strbuf_detach(&buf, NULL);

Same comment here regarding style of the comment.

Regarding the memory leak... you could probably track allocations in a separate array and free them via that array. Another alternative would be to just copy the whole environment in case we need to modify it. I doubt that the performance hit would matter given that the cost to spawn the new process is likely to significantly outweigh the additional memory allocations.

Patrick