Jonathan Nieder <jrnie...@gmail.com> writes:
>> + const int num_rules = ARRAY_SIZE(ref_rev_parse_rules) - 1;
>
> This is assuming ref_rev_parse_rules consists exactly of its items
> followed by a NULL terminator, which is potentially a bit subtle. I
> wonder if we should put
>
> static const char *ref_rev_parse_rules[] = {
> "%.*s",
> "refs/%.*s",
> "refs/tags/%.*s",
> "refs/heads/%.*s",
> "refs/remotes/%.*s",
> "refs/remotes/%.*s/HEAD",
> NULL
> };
> #define NUM_REV_PARSE_RULES (ARRAY_SIZE(ref_rev_parse_rules) - 1)
>
> and then use something like
>
> const int num_rules = NUM_REV_PARSE_RULES;
Perhaps. If we were to go that length, I'd rather first see if we
can lose the sentinel NULL, though.
> Alternatively, what would you think of using the simpler return
> convention
>
> return p - ref_rev_parse_rules + 1;
>
> ? Or even
>
> return p - ref_rev_parse_rules;
>
> and -1 for "no match"?
Heh, that is what I did in the "how about this" patch, which made
the caller a bit more cumbersome by two comparisons, which in turn
was why I rejected the approach.
> Sensible and simple. If we wanted to make items earlier in the list
> return a lower value from refname_match, then we'd need a !best_score
> test here, which might be what motivates that return value convention.
Exactly. See the discussion between JTan and me on his original
patch.
> [...]
>> --- a/t/t5510-fetch.sh
>> +++ b/t/t5510-fetch.sh
>> @@ -535,6 +535,41 @@ test_expect_success "should be able to fetch with
>> duplicate refspecs" '
>> )
>> '
>>
>> +test_expect_success 'LHS of refspec follows ref disambiguation rules' '
>
> Clearly illustrates the bug this fixes, in a way that makes it obvious
> that a user would prefer the new behavior. Good.
>
> With or without the tweak of introducing NUM_REV_PARSE_RULES mentioned
> above,
>
> Reviewed-by: Jonathan Nieder <jrnie...@gmail.com>
One thing I forgot to mention.
When asking to fetch T, in order to be able to favor refs/tags/T
over refs/heads/T at the fetching end, you would have to be able to
*see* both, so all 6 variants "T", "refs/tags/T", "refs/heads/T",
"refs/remotes/T", "refs/remotes/T/HEAD" must be asked to be shown
when the ls-remote limiting is in effect. Since the ls-remote
filtering is relatively new development, we may further find subtle
remaining bugs, if there still are some.
