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

[Jacob Keller <jacob.keller@gmail.com>]

On Wed, Jan 18, 2017 at 12:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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".

You are correct, that is a typo.

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

Yes.

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

I'm not sure which reasoning makes most sense. Any other opinions?

Thanks,
Jake