Re: [PATCH v2 4/7] for-each-repo: run subcommands on configured repos

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 25, 2020, 22:19 UTC
Message-ID: <xmqq8se2z7uh.fsf@gitster.c.googlers.com>
In-Reply-To: <0314258c5cbb8fd771c35e433bf6be95297c4597.1598380805.git.gitgitgadget@gmail.com>

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 8 quoted lines

> +SYNOPSIS
> +--------
> +[verse]
> +'git for-each-repo' --config=<config> [--] <arguments>
> + ...
> +--config=<config>::
> +	Use the given config variable as a multi-valued list storing
> +	absolute path names.

Would it make sense to allow this config to be read from the current repository, I wonder. It is probably designed to be written to either ~/.gitconfig or /etc/gitconfig because it is probably a need that is not per-repository to list repositories for various purposes specified by the config key, but I suspect there _might_ be a good use case for storing some custom list of repositories in the configuration file local to a repository, but it is not quite obvious what it is.

If we have a good example, we may want to spell it out---that would help future readers who wonder about this (just like I am doing now).

Also, if we do read from local config, should there be a way to say "ah, you may have read values from /etc/gitconfig and ~/.gitconfig, but please forget them---I have a full list I care when you are running in this repository", i.e. clear the list. It is purely a convention and there is no built-in mechanism for this in the config API, but often it is signalled by giving an empty string as a value.

By the way, I do not have a good concrete suggestion, but can we use something better than <config> as the placeholder? I first thought this was naming the name of a file that lists repositories, not the config variable name in our usual config namespace.

> +static int run_command_on_repo(const char *path,
> +			       void *cbdata)

Is that on repo or in repo? When I saw "-C" on the command line, I immediately thought of "in repo".

Show 10 quoted lines

> +{
> +	int i;
> +	struct child_process child = CHILD_PROCESS_INIT;
> +	struct strvec *args = (struct strvec *)cbdata;
> +
> +	child.git_cmd = 1;
> +	strvec_pushl(&child.args, "-C", path, NULL);
> +
> +	for (i = 0; i < args->nr; i++)
> +		strvec_push(&child.args, args->v[i]);

Would strvec_pushv() work, or is args->v[] not NULL terminated?

> +	return run_command(&child);
> +}

> +	values = repo_config_get_value_multi(the_repository,
> +					     config_key);

Not your fault, but it is a bit unsatisfactory that we do not have special "type" meant for paths in the config API, unlike the parse-options API where there is a "filename" type that is a bit richer than a vanilla "string" type by allowing "prefix" handling. For the purposes of this, as the values are limited to absolute/full pathnames, it does not hurt as much, though.

Thanks.