for-each-repo: work correctly in a worktree #2056
+77
−24
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
|
|
||
|
There was a problem hiding this comment. Jeff King wrote on the Git mailing list (how to reply to this email): On Mon, Mar 02, 2026 at 03:36:44PM +0000, Derrick Stolee via GitGitGadget wrote:
> @@ -15,10 +16,11 @@ static const char * const for_each_repo_usage[] = {
>
> static int run_command_on_repo(const char *path, int argc, const char ** argv)
> {
> - int i;
> struct child_process child = CHILD_PROCESS_INIT;
> char *abspath = interpolate_path(path, 0);
>
> + clear_local_repo_env(&child.env);
> +
> child.git_cmd = 1;
> strvec_pushl(&child.args, "-C", abspath, NULL);
The second part of the hunk here is as expected, but the first one looks
wrong. We didn't remove any references to "i", so either it was
redundant to start with (and the compiler should have complained), or
now we've broken compilation.
Looks like the latter, but we recover when we switch to using pushv in
patch 4. So I think the declaration of "i" should move to that patch.
-PeffThere was a problem hiding this comment. Derrick Stolee wrote on the Git mailing list (how to reply to this email): On 3/2/2026 1:06 PM, Jeff King wrote:
> On Mon, Mar 02, 2026 at 03:36:44PM +0000, Derrick Stolee via GitGitGadget wrote:
>
>> @@ -15,10 +16,11 @@ static const char * const for_each_repo_usage[] = {
>>
>> static int run_command_on_repo(const char *path, int argc, const char ** argv)
>> {
>> - int i;
>> struct child_process child = CHILD_PROCESS_INIT;
>> char *abspath = interpolate_path(path, 0);
>>
>> + clear_local_repo_env(&child.env);
>> +
>> child.git_cmd = 1;
>> strvec_pushl(&child.args, "-C", abspath, NULL);
>
> The second part of the hunk here is as expected, but the first one looks
> wrong. We didn't remove any references to "i", so either it was
> redundant to start with (and the compiler should have complained), or
> now we've broken compilation.
You are correct. I did a --fixup here and it messed up the diff. I should
have double-checked the commit-by-commit compilation and testing post-
rebase.
> Looks like the latter, but we recover when we switch to using pushv in
> patch 4. So I think the declaration of "i" should move to that patch.
Can do. Looks like a small v4 update _is_ required.
Thanks,
-Stolee
There was a problem hiding this comment. Junio C Hamano wrote on the Git mailing list (how to reply to this email): Derrick Stolee <stolee@gmail.com> writes:
> On 3/2/2026 1:06 PM, Jeff King wrote:
>> On Mon, Mar 02, 2026 at 03:36:44PM +0000, Derrick Stolee via GitGitGadget wrote:
>>
>>> @@ -15,10 +16,11 @@ static const char * const for_each_repo_usage[] = {
>>>
>>> static int run_command_on_repo(const char *path, int argc, const char ** argv)
>>> {
>>> - int i;
>>> struct child_process child = CHILD_PROCESS_INIT;
>>> char *abspath = interpolate_path(path, 0);
>>>
>>> + clear_local_repo_env(&child.env);
>>> +
>>> child.git_cmd = 1;
>>> strvec_pushl(&child.args, "-C", abspath, NULL);
>>
>> The second part of the hunk here is as expected, but the first one looks
>> wrong. We didn't remove any references to "i", so either it was
>> redundant to start with (and the compiler should have complained), or
>> now we've broken compilation.
>
> You are correct. I did a --fixup here and it messed up the diff. I should
> have double-checked the commit-by-commit compilation and testing post-
> rebase.
>
>> Looks like the latter, but we recover when we switch to using pushv in
>> patch 4. So I think the declaration of "i" should move to that patch.
>
> Can do. Looks like a small v4 update _is_ required.
I could do this too ;-) but I probably won't get to queuing this
topic before you update on your own, I suspect, so ... |
||
| #include "builtin.h" | ||
|
There was a problem hiding this comment. Eric Sunshine wrote on the Git mailing list (how to reply to this email): [Cc:+peff]
On Mon, Feb 23, 2026 at 10:32 PM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> When run in a worktree, the GIT_DIR directory is set in a different way
> than in a typical repository. Show this by updating t0068 to include a
> worktree and add a test that runs from that worktree. This requires
> moving the repo.key config into a global config instead of the base test
> repository's local config (demonstrating that it worked with
> non-worktree Git repositories).
>
> The fix is simple: unset the environment variable before looping over
> the repos.
>
> Signed-off-by: Derrick Stolee <stolee@gmail.com>
> ---
> 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)`"There was a problem hiding this comment. Jeff King wrote on the Git mailing list (how to reply to this email): 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).
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.
-PeffThere was a problem hiding this comment. Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Mon, Feb 23, 2026 at 10:34:30PM -0500, Eric Sunshine wrote:
> [Cc:+peff]
>
> On Mon, Feb 23, 2026 at 10:32 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > When run in a worktree, the GIT_DIR directory is set in a different way
> > than in a typical repository. Show this by updating t0068 to include a
> > worktree and add a test that runs from that worktree. This requires
> > moving the repo.key config into a global config instead of the base test
> > repository's local config (demonstrating that it worked with
> > non-worktree Git repositories).
> >
> > The fix is simple: unset the environment variable before looping over
> > the repos.
> >
> > Signed-off-by: Derrick Stolee <stolee@gmail.com>
> > ---
> > 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.
Good question indeed. We have the `local_repo_env` array that contains
all the environment variables that may influence repository discovery.
PatrickThere was a problem hiding this comment. Derrick Stolee wrote on the Git mailing list (how to reply to this email): 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 <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,
-StoleeThere was a problem hiding this comment. Jeff King wrote on the Git mailing list (how to reply to this email): On Tue, Feb 24, 2026 at 07:11:13AM -0500, Derrick Stolee wrote:
> > 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.)
I can see why you'd be surprised if you think of "-C" as "change to this
git repo". But it really is "change to this directory". It is perfectly
OK to "git -C" into a non-toplevel directory of a repo (and continue
respecting any repo discovery that happened already and is in the
environment), or even weird stuff like:
GIT_DIR=/some/repo.git git -C /some/worktree add foo
What you almost kind-of want is "--git-dir", except it puts the onus on
the caller to find the actual repo directory (so detecting bare vs
discovering the .git). Part of the point of introducing -C long ago was
because --git-dir was so annoying to use.
Probably there is room for some middle-ground option, which is "do repo
detection starting in this directory and use that as the --git-dir" (and
I guess also do worktree discovery in the same way). But I don't think
we would ever switch -C to that. It would almost certainly break lots of
people if we changed it now.
-PeffThere was a problem hiding this comment. Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Tue, Feb 24, 2026 at 03:32:29AM +0000, Derrick Stolee via GitGitGadget wrote:
> diff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c
> index 325a7925f1..478ccf1287 100644
> --- a/builtin/for-each-repo.c
> +++ b/builtin/for-each-repo.c
> @@ -1,5 +1,3 @@
> -#define USE_THE_REPOSITORY_VARIABLE
> -
> #include "builtin.h"
> #include "config.h"
> #include "gettext.h"
> @@ -33,7 +31,7 @@ static int run_command_on_repo(const char *path, int argc, const char ** argv)
> int cmd_for_each_repo(int argc,
> const char **argv,
> const char *prefix,
> - struct repository *repo UNUSED)
> + struct repository *repo)
> {
> static const char *config_key = NULL;
> int keep_going = 0;
> @@ -55,7 +53,7 @@ int cmd_for_each_repo(int argc,
> if (!config_key)
> die(_("missing --config=<config>"));
>
> - err = repo_config_get_string_multi(the_repository, config_key, &values);
> + err = repo_config_get_string_multi(repo, config_key, &values);
> if (err < 0)
> usage_msg_optf(_("got bad config --config=%s"),
> for_each_repo_usage, options, config_key);
The command is marked as `RUN_SETUP_GENTLY`, so it may run in a context
where there is no repository. In such cases, `repo` would be `NULL`, and
that would cause the command to segfault here, wouldn't it?
PatrickThere was a problem hiding this comment. Derrick Stolee wrote on the Git mailing list (how to reply to this email): On 2/24/26 4:18 AM, Patrick Steinhardt wrote:
> On Tue, Feb 24, 2026 at 03:32:29AM +0000, Derrick Stolee via GitGitGadget wrote:
>> diff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c
>> index 325a7925f1..478ccf1287 100644
>> --- a/builtin/for-each-repo.c
>> +++ b/builtin/for-each-repo.c
>> @@ -1,5 +1,3 @@
>> -#define USE_THE_REPOSITORY_VARIABLE
>> -
>> #include "builtin.h"
>> #include "config.h"
>> #include "gettext.h"
>> @@ -33,7 +31,7 @@ static int run_command_on_repo(const char *path, int argc, const char ** argv)
>> int cmd_for_each_repo(int argc,
>> const char **argv,
>> const char *prefix,
>> - struct repository *repo UNUSED)
>> + struct repository *repo)
>> {
>> static const char *config_key = NULL;
>> int keep_going = 0;
>> @@ -55,7 +53,7 @@ int cmd_for_each_repo(int argc,
>> if (!config_key)
>> die(_("missing --config=<config>"));
>> >> - err = repo_config_get_string_multi(the_repository, config_key, &values);
>> + err = repo_config_get_string_multi(repo, config_key, &values);
>> if (err < 0)
>> usage_msg_optf(_("got bad config --config=%s"),
>> for_each_repo_usage, options, config_key);
> > The command is marked as `RUN_SETUP_GENTLY`, so it may run in a context
> where there is no repository. In such cases, `repo` would be `NULL`, and
> that would cause the command to segfault here, wouldn't it?
Ah. That's an interesting subtlety of the setup that I did not know.
I'll make sure this is covered in tests, because the current tests run in
the default test repo but our expected use case should be outside a repo.
Thanks,
-Stolee |
||
| #include "config.h" | ||
| #include "environment.h" | ||
| #include "gettext.h" | ||
| #include "parse-options.h" | ||
| #include "path.h" | ||
|
|
@@ -13,17 +14,16 @@ static const char * const for_each_repo_usage[] = { | |
| NULL | ||
|
There was a problem hiding this comment. Jeff King wrote on the Git mailing list (how to reply to this email): On Mon, Mar 02, 2026 at 03:36:45PM +0000, Derrick Stolee via GitGitGadget wrote:
> This change simplifies the code somewhat from its original
> implementation.
Yeah, I think this is worth doing.
-Peff |
||
| }; | ||
|
|
||
| static int run_command_on_repo(const char *path, const char **argv) | ||
| { | ||
| struct child_process child = CHILD_PROCESS_INIT; | ||
| char *abspath = interpolate_path(path, 0); | ||
|
|
||
| sanitize_repo_env(&child.env); | ||
|
|
||
| child.git_cmd = 1; | ||
| strvec_pushl(&child.args, "-C", abspath, NULL); | ||
| strvec_pushv(&child.args, argv); | ||
|
|
||
| free(abspath); | ||
|
|
||
|
|
@@ -63,7 +63,7 @@ int cmd_for_each_repo(int argc, | |
| return 0; | ||
|
|
||
| for (size_t i = 0; i < values->nr; i++) { | ||
| int ret = run_command_on_repo(values->items[i].string, argv); | ||
| if (ret) { | ||
| if (!keep_going) | ||
| return ret; | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1847,7 +1847,7 @@ int run_auto_maintenance(int quiet) | |
| return run_command(&maint); | ||
|
There was a problem hiding this comment. Jeff King wrote on the Git mailing list (how to reply to this email): On Mon, Mar 02, 2026 at 03:36:43PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <stolee@gmail.com>
>
> The current prepare_other_repo_env() does two distinct things:
>
> 1. Strip certain known environment variables that should be set by a
> child process based on a different repository.
>
> 2. Set the GIT_DIR variable to avoid repository discovery.
>
> The second item is valuable for child processes that operate on
> submodules, where the repo discovery could be mistaken for the parent
> repository.
>
> In the next change, we will see an important case where only the first
> item is required as the GIT_DIR discovery should happen naturally from
> the '-C' parameter in the child process.
Yep, this is the refactoring I expected.
> +/**
> + * Unset all local-repo GIT_* variables in env; see local_repo_env in
> + * environment.h. GIT_CONFIG_PARAMETERS and GIT_CONFIG_COUNT are preserved
> + * to pass -c and --config-env options from the parent process.
> + */
> +void clear_local_repo_env(struct strvec *env);
I worry that the name is potentially confusing here, since it is not
just clearing local_repo_env, but making a few exceptions. But I don't
really have a better name. We called this "other_repo_env" in the
existing function, which is equally opaque. I dunno, maybe the
documentation you added would be sufficient.
Speaking of which, the documentation for prepare_other_repo_env() is now
somewhat redundant. If we ever change the behavior here, we'll have to
remember to touch both spots.
So what about squashing in:
diff --git a/run-command.h b/run-command.h
index 76b29d4832..882caeccc8 100644
--- a/run-command.h
+++ b/run-command.h
@@ -518,11 +518,9 @@ void clear_local_repo_env(struct strvec *env);
/**
* 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.
+ * new repo. This removes variables pointing to the local repository (using
+ * clear_local_repo_env() above), and adds an environment variable pointing to
+ * new_git_dir.
*/
void prepare_other_repo_env(struct strvec *env, const char *new_git_dir);
-PeffThere was a problem hiding this comment. Junio C Hamano wrote on the Git mailing list (how to reply to this email): Jeff King <peff@peff.net> writes:
>> +/**
>> + * Unset all local-repo GIT_* variables in env; see local_repo_env in
>> + * environment.h. GIT_CONFIG_PARAMETERS and GIT_CONFIG_COUNT are preserved
>> + * to pass -c and --config-env options from the parent process.
>> + */
>> +void clear_local_repo_env(struct strvec *env);
>
> I worry that the name is potentially confusing here, since it is not
> just clearing local_repo_env, but making a few exceptions. But I don't
> really have a better name. We called this "other_repo_env" in the
> existing function, which is equally opaque. I dunno, maybe the
> documentation you added would be sufficient.
perhaps "clear_local" -> "sanitize" or something, with "env" ->
"other_env" to clarify that we are not emptying ours, but the one
that will be used by somebody else?
> Speaking of which, the documentation for prepare_other_repo_env() is now
> somewhat redundant. If we ever change the behavior here, we'll have to
> remember to touch both spots.
>
> So what about squashing in:
> diff --git a/run-command.h b/run-command.h
> index 76b29d4832..882caeccc8 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -518,11 +518,9 @@ void clear_local_repo_env(struct strvec *env);
>
> /**
> * 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.
> + * new repo. This removes variables pointing to the local repository (using
> + * clear_local_repo_env() above), and adds an environment variable pointing to
> + * new_git_dir.
> */
> void prepare_other_repo_env(struct strvec *env, const char *new_git_dir);
That reads very well.
There was a problem hiding this comment. Derrick Stolee wrote on the Git mailing list (how to reply to this email): On 3/2/2026 1:03 PM, Jeff King wrote:
> So what about squashing in:
>
> diff --git a/run-command.h b/run-command.h
> index 76b29d4832..882caeccc8 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -518,11 +518,9 @@ void clear_local_repo_env(struct strvec *env);
>
> /**
> * 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.
> + * new repo. This removes variables pointing to the local repository (using
> + * clear_local_repo_env() above), and adds an environment variable pointing to
> + * new_git_dir.
> */
> void prepare_other_repo_env(struct strvec *env, const char *new_git_dir);
I'm happy to squash this in.
Perhaps Junio can do it if we don't need other changes to v3. (I haven't
read the rest of the feedback.)
Thanks,
-Stolee |
||
| } | ||
|
|
||
| void sanitize_repo_env(struct strvec *env) | ||
| { | ||
| const char * const *var; | ||
|
|
||
|
|
@@ -1856,6 +1856,11 @@ void prepare_other_repo_env(struct strvec *env, const char *new_git_dir) | |
| strcmp(*var, CONFIG_COUNT_ENVIRONMENT)) | ||
| strvec_push(env, *var); | ||
| } | ||
| } | ||
|
|
||
| void prepare_other_repo_env(struct strvec *env, const char *new_git_dir) | ||
| { | ||
| sanitize_repo_env(env); | ||
| strvec_pushf(env, "%s=%s", GIT_DIR_ENVIRONMENT, new_git_dir); | ||
| } | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,17 +2,23 @@ | |
|
|
||
|
There was a problem hiding this comment. Jeff King wrote on the Git mailing list (how to reply to this email): On Mon, Mar 02, 2026 at 03:36:42PM +0000, Derrick Stolee via GitGitGadget wrote:
> test_description='git for-each-repo builtin'
>
> +# We need to test running 'git for-each-repo' outside of a repo context.
> +TEST_NO_CREATE_REPO=1
> +
> . ./test-lib.sh
Interesting. I was going to point out that this won't do what you want
by itself, because Git will keep walking out of the trash directory and
may find the containing repository.
But it looks like this should be enough due to 614c3d8f2e (test-lib: set
GIT_CEILING_DIRECTORIES to protect the surrounding repository,
2021-08-29). Supporting this case wasn't the intent of that patch, but I
don't see any reason why it should not work reliably.
-PeffThere was a problem hiding this comment. Junio C Hamano wrote on the Git mailing list (how to reply to this email): Jeff King <peff@peff.net> writes:
> On Mon, Mar 02, 2026 at 03:36:42PM +0000, Derrick Stolee via GitGitGadget wrote:
>
>> test_description='git for-each-repo builtin'
>>
>> +# We need to test running 'git for-each-repo' outside of a repo context.
>> +TEST_NO_CREATE_REPO=1
>> +
>> . ./test-lib.sh
>
> Interesting. I was going to point out that this won't do what you want
> by itself, because Git will keep walking out of the trash directory and
> may find the containing repository.
>
> But it looks like this should be enough due to 614c3d8f2e (test-lib: set
> GIT_CEILING_DIRECTORIES to protect the surrounding repository,
> 2021-08-29). Supporting this case wasn't the intent of that patch, but I
> don't see any reason why it should not work reliably.
I am surprised that use of GIT_CEILING_DIRECTORIES was not done
until 2021, actually. The reason the configuration variable was
invented for is exactly to avoid discovery processes going upward
and ending up in a repository different from what we mean to work
with.
There was a problem hiding this comment. Derrick Stolee wrote on the Git mailing list (how to reply to this email): On 3/2/2026 1:31 PM, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
>> On Mon, Mar 02, 2026 at 03:36:42PM +0000, Derrick Stolee via GitGitGadget wrote:
>>
>>> test_description='git for-each-repo builtin'
>>>
>>> +# We need to test running 'git for-each-repo' outside of a repo context.
>>> +TEST_NO_CREATE_REPO=1
>>> +
>>> . ./test-lib.sh
>>
>> Interesting. I was going to point out that this won't do what you want
>> by itself, because Git will keep walking out of the trash directory and
>> may find the containing repository.
>>
>> But it looks like this should be enough due to 614c3d8f2e (test-lib: set
>> GIT_CEILING_DIRECTORIES to protect the surrounding repository,
>> 2021-08-29). Supporting this case wasn't the intent of that patch, but I
>> don't see any reason why it should not work reliably.
>
> I am surprised that use of GIT_CEILING_DIRECTORIES was not done
> until 2021, actually. The reason the configuration variable was
> invented for is exactly to avoid discovery processes going upward
> and ending up in a repository different from what we mean to work
> with.
I didn't know about these historical details. All I know is that I
wrote these changes on top of the buggy patch in [1] and confirmed that
it failed with a segfault. Thanks for confirming the reason that this
works!
[1] https://lore.kernel.org/git/86cd83f65b30aab3233e27b3e5c4f03041e68766.1771903950.git.gitgitgadget@gmail.com/
Thanks,
-Stolee
|
||
| test_description='git for-each-repo builtin' | ||
|
|
||
| # We need to test running 'git for-each-repo' outside of a repo context. | ||
| TEST_NO_CREATE_REPO=1 | ||
|
|
||
| . ./test-lib.sh | ||
|
|
||
| test_expect_success 'run based on configured value' ' | ||
| git init --initial-branch=one one && | ||
| git init --initial-branch=two two && | ||
| git -C two worktree add --orphan ../three && | ||
| git -C three checkout -b three && | ||
| git init --initial-branch=four ~/four && | ||
|
|
||
| git -C two commit --allow-empty -m "DID NOT RUN" && | ||
| git config --global run.key "$TRASH_DIRECTORY/one" && | ||
| git config --global --add run.key "$TRASH_DIRECTORY/three" && | ||
| git config --global --add run.key "~/four" && | ||
|
|
||
| git for-each-repo --config=run.key commit --allow-empty -m "ran" && | ||
| git -C one log -1 --pretty=format:%s >message && | ||
| grep ran message && | ||
|
|
@@ -22,6 +28,7 @@ test_expect_success 'run based on configured value' ' | |
| grep ran message && | ||
| git -C ~/four log -1 --pretty=format:%s >message && | ||
| grep ran message && | ||
|
|
||
| git for-each-repo --config=run.key -- commit --allow-empty -m "ran again" && | ||
| git -C one log -1 --pretty=format:%s >message && | ||
| grep again message && | ||
|
|
@@ -30,7 +37,43 @@ test_expect_success 'run based on configured value' ' | |
| git -C three log -1 --pretty=format:%s >message && | ||
| grep again message && | ||
| git -C ~/four log -1 --pretty=format:%s >message && | ||
| grep again message && | ||
|
|
||
| git -C three for-each-repo --config=run.key -- \ | ||
| commit --allow-empty -m "ran from worktree" && | ||
| git -C one log -1 --pretty=format:%s >message && | ||
| test_grep "ran from worktree" message && | ||
| git -C two log -1 --pretty=format:%s >message && | ||
| test_grep ! "ran from worktree" message && | ||
| git -C three log -1 --pretty=format:%s >message && | ||
| test_grep "ran from worktree" message && | ||
| git -C ~/four log -1 --pretty=format:%s >message && | ||
| test_grep "ran from worktree" message && | ||
|
|
||
| # Test running with config values set by environment | ||
| cat >expect <<-EOF && | ||
| ran from worktree (HEAD -> refs/heads/one) | ||
| ran from worktree (HEAD -> refs/heads/three) | ||
| ran from worktree (HEAD -> refs/heads/four) | ||
| EOF | ||
|
|
||
| GIT_CONFIG_PARAMETERS="${SQ}log.decorate=full${SQ}" \ | ||
| git -C three for-each-repo --config=run.key -- log --format="%s%d" -1 >out && | ||
| test_cmp expect out && | ||
|
|
||
| cat >test-config <<-EOF && | ||
| [run] | ||
| key = $(pwd)/one | ||
| key = $(pwd)/three | ||
| key = $(pwd)/four | ||
|
|
||
| [log] | ||
| decorate = full | ||
| EOF | ||
|
|
||
| GIT_CONFIG_GLOBAL="$(pwd)/test-config" \ | ||
| git -C three for-each-repo --config=run.key -- log --format="%s%d" -1 >out && | ||
| test_cmp expect out | ||
| ' | ||
|
|
||
| test_expect_success 'do nothing on empty config' ' | ||
|
|
@@ -46,7 +89,7 @@ test_expect_success 'error on bad config keys' ' | |
| ' | ||
|
|
||
| test_expect_success 'error on NULL value for config keys' ' | ||
| cat >>.gitconfig <<-\EOF && | ||
| [empty] | ||
| key | ||
| EOF | ||
|
|
@@ -59,8 +102,8 @@ test_expect_success 'error on NULL value for config keys' ' | |
| ' | ||
|
|
||
| test_expect_success '--keep-going' ' | ||
| git config --global keep.going non-existing && | ||
| git config --global --add keep.going one && | ||
|
|
||
| test_must_fail git for-each-repo --config=keep.going \ | ||
| -- branch >out 2>err && | ||
|
|
@@ -70,7 +113,7 @@ test_expect_success '--keep-going' ' | |
| test_must_fail git for-each-repo --config=keep.going --keep-going \ | ||
| -- branch >out 2>err && | ||
| test_grep "cannot change to .*non-existing" err && | ||
| git -C one branch >expect && | ||
| test_cmp expect out | ||
| ' | ||
|
|
||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Derrick Stolee wrote on the Git mailing list (how to reply to this email):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jeff King wrote on the Git mailing list (how to reply to this email):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Derrick Stolee wrote on the Git mailing list (how to reply to this email):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Derrick Stolee wrote on the Git mailing list (how to reply to this email):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jeff King wrote on the Git mailing list (how to reply to this email):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jeff King wrote on the Git mailing list (how to reply to this email):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Derrick Stolee wrote on the Git mailing list (how to reply to this email):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jeff King wrote on the Git mailing list (how to reply to this email):