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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 18, 2017, 20:04 UTC
Message-ID: <xmqqy3y8878k.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170118000930.5431-3-jacob.e.keller@intel.com>

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

Show 7 quoted lines

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

Show 6 quoted lines

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

Show 23 quoted lines

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