Re: [PATCH v2 2/2] for-each-repo: work correctly in a worktree

[Derrick Stolee <stolee@gmail.com>]

On 2/27/2026 5:42 PM, Jeff King wrote:
> 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).

I agree that it would be best to have a single place.

I was looking at prepare_other_repo_env() and saw that it requires a
computed gitdir, which is not easy to compute. We want the child process
to perform that discovery based on the -C parameter.

However, we can extract the existing environment clearing logic and use
that here. I'll give that a try and confirm that it passes the tests
that I prepared to fix the bugs in this version.

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

It has GIT_CONFIG (the local Git config file), GIT_CONFIG_COUNT, and
GIT_CONFIG_PARAMETERS. My patch was wrong because of the string, showing
the value in having tests to confirm the right behavior.

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

OK. I won't special case this myself and will let this be changed
independently, if that is indeed valuable.

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

This is where I was landing, too.

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

This makes sense. In the meantime, having a single place that unsets
environment variables for certain child processes is good enough to
cover what we need here.

Thanks,
-Stolee