Re: [PATCH v2] remote: make refspec follow the same disambiguation rule as local refs
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 2, 2018, 02:21 UTC
- Message-ID
- <xmqqo9el4ige.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20180801230802.GC189024@aiede.svl.corp.google.com>
Jonathan Nieder <jrnieder@gmail.com> writes:
Show 20 quoted lines
>> + 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.
Show 10 quoted lines
> 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.
Show 16 quoted lines
> [...] >> --- 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.