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

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

[Junio C Hamano]

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.

This causes unexpected behavior when (for example) fetching using
the refspec "refs/heads/s:<something>" from a remote with both
"refs/heads/refs/heads/s" and "refs/heads/s"; even if the former was
inadvertently created, one would still expect the latter to be
fetched.  Similarly, when both a tag T and a branch T exist,
fetching T should favor the tag, just like how local refname
disambiguation rule works.  But because the code walks over
ls-remote output from the remote, which happens to be sorted in
alphabetical order and has refs/heads/T before refs/tags/T, a
request to fetch T is (mis)interpreted as fetching refs/heads/T.

Update refname_match(), all of whose current callers care only if it
returns non-zero (i.e. matches) to see if an abbreviated name can
mean the full name being tested, so that it returns a positive
integer whose magnitude can be used to tell the precedence, and fix
the find_ref_by_name_abbrev() function not to stop at the first
match but find the match with the highest precedence.

This is based on an earlier work, which special cased only the exact
matches, by Jonathan Tan.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This time, with a log message, updated "precedence" number, a bit
   of in-code comment and a new test to show that the fix extends to
   non-exact disambiguation as well.

 refs.c           | 16 +++++++++++-----
 remote.c         | 13 ++++++++++---
 t/t5510-fetch.sh | 35 +++++++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+), 8 deletions(-)

diff --git a/refs.c b/refs.c
index 20ba82b434..d1be61b1b5 100644
--- 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;
 
-	for (p = ref_rev_parse_rules; *p; p++) {
-		if (!strcmp(full_name, mkpath(*p, abbrev_name_len, abbrev_name))) {
-			return 1;
-		}
-	}
+	for (p = ref_rev_parse_rules; *p; p++)
+		if (!strcmp(full_name, mkpath(*p, abbrev_name_len, abbrev_name)))
+			return &ref_rev_parse_rules[num_rules] - p;
 
 	return 0;
 }
diff --git a/remote.c b/remote.c
index c10d87c246..4a3e7ba136 100644
--- 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;
 }
 
 struct ref *get_remote_ref(const struct ref *remote_refs, const char *name)
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index da9ac00557..858381a788 100755
--- 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' '
+	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
+	)
+'
+
 # configured prune tests
 
 set_config_tristate () {
-- 
2.18.0-321-gffc6fa0e39

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

[Jonathan Tan]

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

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

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> 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

[Jonathan Nieder]

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 <jrnieder@gmail.com>

Thanks.

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

[Junio C Hamano]

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.

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

[Jonathan Nieder]

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

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

[Junio C Hamano]

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.

This causes unexpected behavior when (for example) fetching using
the refspec "refs/heads/s:<something>" from a remote with both
"refs/heads/refs/heads/s" and "refs/heads/s"; even if the former was
inadvertently created, one would still expect the latter to be
fetched.  Similarly, when both a tag T and a branch T exist,
fetching T should favor the tag, just like how local refname
disambiguation rule works.  But because the code walks over
ls-remote output from the remote, which happens to be sorted in
alphabetical order and has refs/heads/T before refs/tags/T, a
request to fetch T is (mis)interpreted as fetching refs/heads/T.

Update refname_match(), all of whose current callers care only if it
returns non-zero (i.e. matches) to see if an abbreviated name can
mean the full name being tested, so that it returns a positive
integer whose magnitude can be used to tell the precedence, and fix
the find_ref_by_name_abbrev() function not to stop at the first
match but find the match with the highest precedence.

This is based on an earlier work, which special cased only the exact
matches, by Jonathan Tan.

Helped-by: Jonathan Tan <jonathantanmy@google.com>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * OK, with the NUM_REV_PARSE_RULES constant this time.

 refs.c           | 18 +++++++++++++-----
 remote.c         | 13 ++++++++++---
 t/t5510-fetch.sh | 35 +++++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+), 8 deletions(-)

diff --git a/refs.c b/refs.c
index 20ba82b434..2ca715d099 100644
--- a/refs.c
+++ b/refs.c
@@ -487,16 +487,24 @@ static const char *ref_rev_parse_rules[] = {
 	NULL
 };
 
+#define NUM_REV_PARSE_RULES (ARRAY_SIZE(ref_rev_parse_rules) - 1)
+
+/*
+ * 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 = NUM_REV_PARSE_RULES;
 
-	for (p = ref_rev_parse_rules; *p; p++) {
-		if (!strcmp(full_name, mkpath(*p, abbrev_name_len, abbrev_name))) {
-			return 1;
-		}
-	}
+	for (p = ref_rev_parse_rules; *p; p++)
+		if (!strcmp(full_name, mkpath(*p, abbrev_name_len, abbrev_name)))
+			return &ref_rev_parse_rules[num_rules] - p;
 
 	return 0;
 }
diff --git a/remote.c b/remote.c
index c10d87c246..4a3e7ba136 100644
--- 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;
 }
 
 struct ref *get_remote_ref(const struct ref *remote_refs, const char *name)
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index da9ac00557..858381a788 100755
--- 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' '
+	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
+	)
+'
+
 # configured prune tests
 
 set_config_tristate () {
-- 
2.18.0-321-gffc6fa0e39