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, 01:29 UTC
Message-ID: <xmqq8tzhl6v7.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1462928397-1708-6-git-send-email-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 12 quoted lines

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

Show 10 quoted lines

> +		} else {
> +			/*
> +			* NEEDSWORK:
> +			* Improve autodetection of items with no prefixes,
> +			* roughly like
> +			* if (label_or_name_exists(argv[i]))
> +			*	string_list_insert(group, argv[i]);
> +			* else
> +			*	argv_array_push(&ps, argv[i]);
> +			*/

I do not think this is desirable. "label" that changes its meaning between "a path ./label" to "a label *label" would force people to give unnecessary prefix "./" when naming their path, from fear of colliding with a label that may or may not exist.

> +			argv_array_push(&ps, argv[i]);
> +		}
> +	}

Show 20 quoted lines

> +		if (!group.nr) {
> +			if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),
> +					    0, ps_matched, 1) ||
> +			    !S_ISGITLINK(ce->ce_mode))
> +				continue;
> +		} else {
> +			const struct submodule *sub;
> +			int ps = 0, gr = 0;
> +			if (!S_ISGITLINK(ce->ce_mode))
> +				continue;
> +			sub = submodule_from_path(null_sha1, ce->name);
> +
> +			ps = match_pathspec(pathspec, ce->name, ce_namelen(ce),
> +					    0, ps_matched, 1);
> +			gr = submodule_in_group(&group, sub);
> +
> +			if (!pathspec->nr && !gr)
> +				continue;
> +			if (!ps && !gr)
> +				continue;

Hmph, so the rule is "a submodule that is in the specified group is chosen, even if it is outside the subset of paths narrowed by the given pathspec." I think that is consistent with the way how the list of arguments given to module_list (i.e. a pathspec element plus a group specifier OR together just like two pathspec elements do not force a path to match both, and instead they OR together).

Is that rule (i.e. specifiers are ORed together) written down somewhere for users?