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

[Derrick Stolee <stolee@gmail.com>]

On 8/25/2020 6:19 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> +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.

I guess I should test this, but if I ask for a multi-valued config,
will I not get _all_ of the results from /etc/gitconfig, ~/.gitconfig,
AND .git/config? That was my expectation, which is why I don't specify
"local" or "global" config anywhere in the discussion.

> 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.
Sure. How about "<key>"?

>> +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".

"in" is better.

>> +{
>> +	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?

Yeah, pushv should work.

>> +	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.

Interesting. Noted.

Thanks,
-Stolee