Re: [PATCH 5/7] submodule--helper module_list_compute: allow label or name arguments

From: Junio C Hamano <gitster@pobox.com>
Date: May 11, 2016, 02:24 UTC
Message-ID: <xmqqzirxjpqw.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqq8tzhl6v7.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 19 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> +static void split_argv_pathspec_groups(int argc, const char **argv,
>> +				       const char ***pathspec_argv,
>> +				       struct string_list *group)
>> +{
>> +	int i;
>> +	struct argv_array ps = ARGV_ARRAY_INIT;
>> +	for (i = 0; i < argc; i++) {
>> +		if (starts_with(argv[i], "*")
>> +		    || starts_with(argv[i], ":")) {
>> +			string_list_insert(group, argv[i]);
>> +		} else if (starts_with(argv[i], "./")) {
>> +			argv_array_push(&ps, argv[i]);
>
> I'd suggest stripping "./" when you do this.  That is, make the rule
> to be "*label is a label, :name is a name, and everything else is a
> path.  You can prefix ./ to an unfortunate path that begins with an
> asterisk or a colon and we will strip ./ disambiguator".

To clarify, "./$path and $path are the same so why strip it?" is an understandable, even though naive, question. The reason is because you do not want to contaminate the code that parses pathspecs with the knowledge of this submodule-group specific prefix. "./$path" and "$path" may be equivalent for a literal pathspec, but I'd want to see user be able to say

   git submodule foreach './:(icase)foo'

and find Foo, foo, FOO, etc.

I would also recommend to strip '*' and ':' from group names and module names, and maintain two separate lists. You would eventually want to do "*default*" to name all groups whose name matches with the pattern "default*", as if it were a pathspec matched against the available group names, and that will keep the door open for future extension like "*:(icase)default*".