Re: [PATCH v3 2/5] name-rev: extend --refs to accept multiple patterns

[Junio C Hamano <gitster@pobox.com>]

Jacob Keller <jacob.e.keller@intel.com> writes:

> From: Jacob Keller <jacob.keller@gmail.com>
>
> Teach git name-rev to take a string list of patterns from --refs instead
> of only a single pattern. The list of patterns will be matched
> inclusively, such that a ref only needs to match one pattern to be
> included. If a ref will only be excluded if it does not match any of the
> patterns.

I think "If a" in the last sentence should be "A".

>  --refs=<pattern>::
>  	Only use refs whose names match a given shell pattern.  The pattern
> -	can be one of branch name, tag name or fully qualified ref name.
> +	can be one of branch name, tag name or fully qualified ref name. If
> +	given multiple times, use refs whose names match any of the given shell
> +	patterns. Use `--no-refs` to clear any previous ref patterns given.

Unlike 1/5, this is facing the end-users, and the last sentence is
very appropriate.

> +	if (data->ref_filters.nr) {
> +		struct string_list_item *item;
> +		int matched = 0;
> +
> +		/* See if any of the patterns match. */
> +		for_each_string_list_item(item, &data->ref_filters) {
> +			/*
> +			 * We want to check every pattern even if we already
> +			 * found a match, just in case one of the later
> +			 * patterns could abbreviate the output.
> +			 */
> +			switch (subpath_matches(path, item->string)) {
> +			case -1: /* did not match */
> +				break;
> +			case 0: /* matched fully */
> +				matched = 1;
> +				break;
> +			default: /* matched subpath */
> +				matched = 1;
> +				can_abbreviate_output = 1;
> +				break;
> +			}
>  		}

I agree that we cannot short-cut on the first match to make sure
that the outcome is stable, but I wondered what should be shown when
we do have multiple matches.  Say I gave

    --refs="v*" --refs="refs/tags/v1.*"

and refs/tags/v1.0 matched.  The above code would say we can
abbreviate.

What is the reason behind this design decision?  Is it because it is
clear that the user shows her willingness to accept more compact
form by having --refs="v*" that would allow shortening?  If that is
the case, I think I agree with the reasoning.  But we probably want
to write it down somewhere, because another reasoning, which may
also be valid, would call for an opposite behaviour (i.e. the more
specific --refs="refs/tags/v1.*" also matched, so let's show that
fact by not shortening).