From: Derrick Stolee Date: Tue, 24 Feb 2026 12:11:13 GMT Subject: Re: [PATCH 2/2] for-each-repo: work correctly in a worktree Message-ID: In-Reply-To: <20260224091806.GC986367@coredump.intra.peff.net> On 2/24/26 4:18 AM, Jeff King wrote: > On Mon, Feb 23, 2026 at 10:34:30PM -0500, Eric Sunshine wrote: > >>> diff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c >>> @@ -60,6 +61,9 @@ int cmd_for_each_repo(int argc, >>> + /* Be sure to not pass GIT_DIR to children. */ >>> + unsetenv(GIT_DIR_ENVIRONMENT); >> >> This only unsets GIT_DIR. Is that sufficient in the general case? >> Elsewhere, we recommend[*] unsetting all of Git's local environment >> variables. >> >> [*]: From the "githooks" man page: "Environment variables, such as >> GIT_DIR, GIT_WORK_TREE, etc., are exported so that Git commands run by >> the hook can correctly locate the repository. If your hook needs to >> invoke Git commands in a foreign repository or in a different working >> tree of the same repository, then it should clear these environment >> variables so they do not interfere with Git operations at the foreign >> location. For example: `unset $(git rev-parse --local-env-vars)`" > > Yeah, agreed. There's another subtle issue, which is that this is > unsetting GIT_DIR in the parent process. So any other code we call that > is meant to run in the original repo might get confused. I can well > believe there isn't any such code for a command like for-each-repo, but > as a general principle, the change should be made in the sub-process. > > You can stick the elements of local_repo_env into the "env" list of the > child_process struct. If you grep around, you can find some instances of > this. > > There's an open question there of how to handle config in the > environment, though. Depending on the sub-process, you may or may not > want such config to pass down to it. For for-each-repo, I'd guess that > you'd want: > > git -c foo.bar=baz for-each-repo ... > > to pass that foo.bar value. We do have a helper to handle that in > run-command.h: > > /** > * Convenience function which prepares env for a command to be run in a > * new repo. This adds all GIT_* environment variables to env with the > * exception of GIT_CONFIG_PARAMETERS and GIT_CONFIG_COUNT (which cause the > * corresponding environment variables to be unset in the subprocess) and adds > * an environment variable pointing to new_git_dir. See local_repo_env in > * environment.h for more information. > */ > void prepare_other_repo_env(struct strvec *env, const char *new_git_dir); > > Do be careful using it here, though. It expects to set GIT_DIR itself to > point to the new repo (which is passed in). But I'm not sure that's 100% > compatible with how for-each-repo works, which is using "git -C $repo" > under the hood, and letting the usual discovery happen. > > So for a bare repo, you'd want to pass the repo directory. But for a > non-bare one, you'd want $repo/.git. And there are even more weird > corner cases, like the fact that using "/my/repo/but/inside/a/subdir" > with for-each-repo will find "/my/repo". > > So you might need to refactor prepare_other_repo_env() to split out the > "everything but the config" logic versus the "set GIT_DIR" logic. Or > just inline the former in run_command_on_repo(), though it probably is > better to keep the logic in one place (it's not many lines, but it has > to know about all of the env variables that affect config). Thanks for the recommendations. I'll come back with a more sophisticated v2 that handles these issues. > Alternatively, for-each-repo could do repo discovery itself on the paths > it is passed, before calling sub-programs. That's a bigger change, but > possibly it could or should be flagging an error for some cases? I > dunno. I'm surprised that passing '-C ' doesn't already overwrite these variables but I suppose environment variables override arguments in this case. (This is the root of the bug.) Thanks, -Stolee