Re: [PATCH v2] remote: make refspec follow the same disambiguation rule as local refs

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

Jonathan Nieder <jrnieder@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 <jrnieder@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.