Re: [PATCH v2] remote: make refspec follow the same disambiguation rule as local refs
Junio C Hamano wrote: > 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. Fortunately, the fetch code does already do that. ;-) Jonathan
Re: [PATCH v2] remote: make refspec follow the same disambiguation rule as local refs
Jonathan Nieder 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
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.
Re: [PATCH v2] remote: make refspec follow the same disambiguation rule as local refs
Hi,
Junio C Hamano wrote:
> When matching a non-wildcard LHS of a refspec against a list of
> refs, find_ref_by_name_abbrev() returns the first ref that matches
> using any DWIM rules used by refname_match() in refs.c, even if a
> better match occurs later in the list of refs.
Nicely explained.
[...]
> --- a/refs.c
> +++ b/refs.c
> @@ -487,16 +487,22 @@ static const char *ref_rev_parse_rules[] = {
> NULL
> };
>
> +/*
> + * Is it possible that the caller meant full_name with abbrev_name?
> + * If so return a non-zero value to signal "yes"; the magnitude of
> + * the returned value gives the precedence used for disambiguation.
> + *
> + * If abbrev_name cannot mean full_name, return 0.
> + */
> int refname_match(const char *abbrev_name, const char *full_name)
> {
> const char **p;
> const int abbrev_name_len = strlen(abbrev_name);
> + 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;
so that this dependency is more obvious if the ref_rev_parse_rules
convention changes later.
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"?
[...]
> --- a/remote.c
> +++ b/remote.c
> @@ -1880,11 +1880,18 @@ static struct ref *get_expanded_map(const struct ref
> *remote_refs,
> static const struct ref *find_ref_by_name_abbrev(const struct ref *refs,
> const char *name)
> {
> const struct ref *ref;
> + const struct ref *best_match = NULL;
> + int best_score = 0;
> +
> for (ref = refs; ref; ref = ref->next) {
> - if (refname_match(name, ref->name))
> - return ref;
> + int score = refname_match(name, ref->name);
> +
> + if (best_score < score) {
> + best_match = ref;
> + best_score = score;
> + }
> }
> - return NULL;
> + return best_match;
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.
[...]
> --- 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
Thanks.
Re: [PATCH v2] remote: make refspec follow the same disambiguation rule as local refs
Jonathan Tan writes:
>> +test_expect_success 'LHS of refspec follows ref disambiguation rules' '
>> +mkdir lhs-ambiguous &&
>> +(
>> +cd lhs-ambiguous &&
>> +git init server &&
>> +test_commit -C server unwanted &&
>> +test_commit -C server wanted &&
>> +
>> +git init client &&
>> +
>> +# Check a name coming after "refs" alphabetically ...
>> +git -C server update-ref refs/heads/s wanted &&
>> +git -C server update-ref refs/heads/refs/heads/s unwanted &&
>> +git -C client fetch ../server
>> +refs/heads/s:refs/heads/checkthis &&
>> +git -C server rev-parse wanted >expect &&
>> +git -C client rev-parse checkthis >actual &&
>> +test_cmp expect actual &&
>> +
>> +# ... and one before.
>> +git -C server update-ref refs/heads/q wanted &&
>> +git -C server update-ref refs/heads/refs/heads/q unwanted &&
>> +git -C client fetch ../server
>> +refs/heads/q:refs/heads/checkthis &&
>> +git -C server rev-parse wanted >expect &&
>> +git -C client rev-parse checkthis >actual &&
>> +test_cmp expect actual &&
>> +
>> +# Tags are preferred over branches like refs/{heads,tags}/*
>> +git -C server update-ref refs/tags/t wanted &&
>> +git -C server update-ref refs/heads/t unwanted &&
>> +git -C client fetch ../server +t:refs/heads/checkthis &&
>> +git -C server rev-parse wanted >expect &&
>> +git -C client rev-parse checkthis >actual
>> +)
>> +'
>
> Thanks, this looks good to me. Also thanks for adding the "+" in the
> fetch commands in the test.
Yup, otherwise the fetch may fail because "checkthis" may have to be
rewound when we fetch different things.
Re: [PATCH v2] remote: make refspec follow the same disambiguation rule as local refs
> +test_expect_success 'LHS of refspec follows ref disambiguation rules' '
> + mkdir lhs-ambiguous &&
> + (
> + cd lhs-ambiguous &&
> + git init server &&
> + test_commit -C server unwanted &&
> + test_commit -C server wanted &&
> +
> + git init client &&
> +
> + # Check a name coming after "refs" alphabetically ...
> + git -C server update-ref refs/heads/s wanted &&
> + git -C server update-ref refs/heads/refs/heads/s unwanted &&
> + git -C client fetch ../server
> +refs/heads/s:refs/heads/checkthis &&
> + git -C server rev-parse wanted >expect &&
> + git -C client rev-parse checkthis >actual &&
> + test_cmp expect actual &&
> +
> + # ... and one before.
> + git -C server update-ref refs/heads/q wanted &&
> + git -C server update-ref refs/heads/refs/heads/q unwanted &&
> + git -C client fetch ../server
> +refs/heads/q:refs/heads/checkthis &&
> + git -C server rev-parse wanted >expect &&
> + git -C client rev-parse checkthis >actual &&
> + test_cmp expect actual &&
> +
> + # Tags are preferred over branches like refs/{heads,tags}/*
> + git -C server update-ref refs/tags/t wanted &&
> + git -C server update-ref refs/heads/t unwanted &&
> + git -C client fetch ../server +t:refs/heads/checkthis &&
> + git -C server rev-parse wanted >expect &&
> + git -C client rev-parse checkthis >actual
> + )
> +'
Thanks, this looks good to me. Also thanks for adding the "+" in the
fetch commands in the test.
