From: Jeff King <peff@peff.net>
Date: Fri, 27 Feb 2026 22:42:38 GMT
Subject: Re: [PATCH v2 2/2] for-each-repo: work correctly in a worktree
Message-ID: <20260227224238.GA2956443@coredump.intra.peff.net>
In-Reply-To: <08c6e203-3444-45c7-9bc9-cc2590be30c3@gmail.com>

On Thu, Feb 26, 2026 at 10:29:47AM -0500, Derrick Stolee wrote:

> Great point. Here's another attempt:
> 
> static int run_command_on_repo(const char *path, int argc, const char ** argv)
> {
> 	int i = 0;
> 	struct child_process child = CHILD_PROCESS_INIT;
> 	char *abspath = interpolate_path(path, 0);
> 
> 	while (local_repo_env[i]) {
> 		/*
> 		 * Preserve pre-builtin options:
> 		 * - CONFIG_ENVIRONMENT, CONFIG_DATA_ENVIRONMENT, and
> 		 *   CONFIG_COUNT_ENVIRONMENT persist -c <name>=<value>
> 		 *   and --config-env=<name>=<envvar> options.
> 		 * - NO_REPLACE_OBJECTS_ENVIRONMENT persists the
> 		 *   --no-replace-objects option.
> 		 *
> 		 * Note that the following options are not in local_repo_env:
> 		 * - EXEC_PATH_ENVIRONMENT persists --exec-path option.
> 		 */
> 		if (strncmp(local_repo_env[i], "CONFIG_", 7) &&
> 		    strcmp(local_repo_env[i], NO_REPLACE_OBJECTS_ENVIRONMENT))
> 			strvec_push(&child.env, local_repo_env[i]);

This is slightly different than what prepare_other_repo_env() does:

  - it doesn't drop GIT_CONFIG_*, but assumes that removing
    GIT_CONFIG_COUNT is enough for GIT_CONFIG_KEY/VALUE to be ignored
    (and then also removes GIT_CONFIG_PARAMETERS, of course)

  - it doesn't consider NO_REPLACE_OBJECTS at all

I think you could make arguments either way about what should happen
when spawning a command in another repo. But I'd really prefer for us to
have a single spot to specify that policy, and not subtly-different
behavior from different commands. So I'd really like to see this using
that other function (or the logic from it factored out into a helper).

And then we can consider whether to make changes to that policy.

Dropping GIT_CONFIG_* from the environment does make sense in general,
but it doesn't actually happen with the patch above (because only
GIT_CONFIG_COUNT is in the local_repo_env list; to find the others we'd
have to actually enumerate the current environment).

For NO_REPLACE_OBJECTS, I think you could argue that it should not be in
local_repo_env at all. It is more about the operation being performed,
not the repository itself. So for example in this command:

  git --no-replace-objects fetch

I would expect that NO_REPLACE_OBJECTS to make it down to any submodule
fetches we do. Likewise for other operation-level variables like
GIT_LITERAL_PATHSPECS, but those are already (correctly IMHO) omitted
from local_repo_env.

It looks like NO_REPLACE_OBJECTS got pulled from the connect.c code in
48a7c1c49d (Refactor list of of repo-local env vars, 2010-02-25). And I
could see somebody wanting to make sure that upload-pack behaved
predictably with respect to replace refs, but it already does: it
disables replace refs itself as part of its startup code.

> This comment details my findings from comparing the list in
> local_repo_env[] and the top-level options listed in
> Documentation/git.adoc. That's how I was able to find that
> --exec-path sets an environment variable that's NOT in the
> list and we want to be sure we don't set it.
> 
> Should we add the comparison to EXEC_PATH_ENVIRONMENT as a
> precaution to make sure it's not added to local_repo_env in
> the future? Or is that too defensive?

I don't think we need to bother. Obviously adding it to local_repo_env
would be the wrong thing, but that is true of lots of variables. Trying
to make a list is just going to result in a list that is out-of-date,
because there's nothing pushing people to update it when they introduce
a new variable.

You can imagine a different world, where we had a single list of all
environment variables, and new ones _had_ to be added to the list in
order to function, and each entry had a bitflag for "this is a
local-repo value", then that might force each new addition to consider
whether it should be added. But we don't have such a list, and I think
structuring things that way would introduce new complications and
awkwardness.

So IMHO we should just rely on review to reject a patch that tries
something silly like adding EXEC_PATH_ENVIRONMENT to local_repo_env.

-Peff