From: Derrick Stolee Date: Mon, 02 Mar 2026 15:31:48 GMT Subject: Re: [PATCH v2 2/2] for-each-repo: work correctly in a worktree Message-ID: In-Reply-To: <20260227224238.GA2956443@coredump.intra.peff.net> 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 = >> * and --config-env== 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