Re: [PATCH 2/2] for-each-repo: work correctly in a worktree
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Feb 24, 2026, 12:11 UTC
- Message-ID
- <fce7662f-d741-41e1-93dd-f82e65e04f41@gmail.com>
- In-Reply-To
- <20260224091806.GC986367@coredump.intra.peff.net>
On 2/24/26 4:18 AM, Jeff King wrote:
Show 64 quoted lines
> 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 <repo>' 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