remote: make refspec follow the same disambiguation rule as local refs

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>

diff --git a/refs.c b/refs.c
index 20ba82b4343ff2ef72cea32deec8a8d7fbd6def7..2ca715d099ab0970ad0c95993c4bdbd9c998cb8f 100644 (file)
--- 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 c10d87c24615e9d6497b46a69a82a71d3c1735a6..4a3e7ba13613d044cb4ba6e7e025eb51962ba92f 100644 (file)
--- 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 da9ac0055721237f177d3d475e56ddb38b25eff1..858381a7881f430c5e779c2e385ebda4bed3a6dc 100755 (executable)
--- 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 () {