From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 26 Feb 2026 16:21:23 GMT
Subject: Re: [PATCH v2 2/2] for-each-repo: work correctly in a worktree
Message-ID: <xmqqsean4gsc.fsf@gitster.g>
In-Reply-To: <08c6e203-3444-45c7-9bc9-cc2590be30c3@gmail.com>

Derrick Stolee <stolee@gmail.com> writes:

> On 2/25/2026 8:13 AM, Jeff King wrote:
>> On Wed, Feb 25, 2026 at 06:44:51AM -0500, Derrick Stolee wrote:
>> 
>>>> Looking at run-command.c:prep_childenv(), it seems that you can pass
>>>> "VAR=VAL" to "export VAR=VAL" in the child, and pass "VAR" to "unset
>>>> VAR" in the child.
>
>> But I really think you should consider keeping config-related variables
>> in place, as prepare_other_repo_env() does. Otherwise something like:
>> 
>>   git -c pack.threads=1 for-each-repo repack -ad
>> 
>> will ignore that config in the sub-processes (whereas it currently is
>> respected).
>> 
>> And for that, you do need to loop yourself.
>
> 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) &&

Minor nit: !starts_with() lets you avoid counting bytes yourself and
hardcoding "7" here.

> 		    strcmp(local_repo_env[i], NO_REPLACE_OBJECTS_ENVIRONMENT))
> 			strvec_push(&child.env, local_repo_env[i]);
>
> 		i++;
> 	}
>
> 	child.git_cmd = 1;
> 	strvec_pushl(&child.args, "-C", abspath, NULL);
>
> 	for (i = 0; i < argc; i++)
> 		strvec_push(&child.args, argv[i]);

If argv[argc] == NULL, then here is where we want strvec_pushv().

> 	free(abspath);
>
> 	return run_command(&child);
> }
>
> 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.

Hmph, wouldn't we want to use specified exec-path inside ...

    git --exec-path=~/my/git/libexec for-each-repo sh -c "do things"

... "do things" script when we find Git related binaries?  Or am I
not getting what you are describing here?

> 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?
>
> Thanks,
> -Stolee