{"thread":{"id":"49001","subject":"[PATCH v2] remote: make refspec follow the same disambiguation rule as local refs","startedAt":"2018-08-01T17:41:41Z","lastAt":"2018-08-02T22:21:53Z","messageCount":7,"participants":["Junio C Hamano","Jonathan Tan","Jonathan Nieder"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"354171","messageId":"xmqq8t5q821c.fsf@gitster-ct.c.googlers.com","threadId":"49001","inReplyTo":null,"subject":"[PATCH v2] remote: make refspec follow the same disambiguation rule as local refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-01T16:49:51Z","receivedAt":"2018-08-01T17:41:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When matching a non-wildcard LHS of a refspec against a list of\nrefs, find_ref_by_name_abbrev() returns the first ref that matches\nusing any DWIM rules used by refname_match() in refs.c, even if a\nbetter match occurs later in the list of refs.\n\nThis causes unexpected behavior when (for example) fetching using\nthe refspec \"refs/heads/s:<something>\" from a remote with both\n\"refs/heads/refs/heads/s\" and \"refs/heads/s\"; even if the former was\ninadvertently created, one would still expect the latter to be\nfetched.  Similarly, when both a tag T and a branch T exist,\nfetching T should favor the tag, just like how local refname\ndisambiguation rule works.  But because the code walks over\nls-remote output from the remote, which happens to be sorted in\nalphabetical order and has refs/heads/T before refs/tags/T, a\nrequest to fetch T is (mis)interpreted as fetching refs/heads/T.\n\nUpdate refname_match(), all of whose current callers care only if it\nreturns non-zero (i.e. matches) to see if an abbreviated name can\nmean the full name being tested, so that it returns a positive\ninteger whose magnitude can be used to tell the precedence, and fix\nthe find_ref_by_name_abbrev() function not to stop at the first\nmatch but find the match with the highest precedence.\n\nThis is based on an earlier work, which special cased only the exact\nmatches, by Jonathan Tan.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * This time, with a log message, updated \"precedence\" number, a bit\n   of in-code comment and a new test to show that the fix extends to\n   non-exact disambiguation as well.\n\n refs.c           | 16 +++++++++++-----\n remote.c         | 13 ++++++++++---\n t/t5510-fetch.sh | 35 +++++++++++++++++++++++++++++++++++\n 3 files changed, 56 insertions(+), 8 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 20ba82b434..d1be61b1b5 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -487,16 +487,22 @@ static const char *ref_rev_parse_rules[] = {\n \tNULL\n };\n \n+/*\n+ * Is it possible that the caller meant full_name with abbrev_name?\n+ * If so return a non-zero value to signal \"yes\"; the magnitude of\n+ * the returned value gives the precedence used for disambiguation.\n+ *\n+ * If abbrev_name cannot mean full_name, return 0.\n+ */\n int refname_match(const char *abbrev_name, const char *full_name)\n {\n \tconst char **p;\n \tconst int abbrev_name_len = strlen(abbrev_name);\n+\tconst int num_rules = ARRAY_SIZE(ref_rev_parse_rules) - 1;\n \n-\tfor (p = ref_rev_parse_rules; *p; p++) {\n-\t\tif (!strcmp(full_name, mkpath(*p, abbrev_name_len, abbrev_name))) {\n-\t\t\treturn 1;\n-\t\t}\n-\t}\n+\tfor (p = ref_rev_parse_rules; *p; p++)\n+\t\tif (!strcmp(full_name, mkpath(*p, abbrev_name_len, abbrev_name)))\n+\t\t\treturn &ref_rev_parse_rules[num_rules] - p;\n \n \treturn 0;\n }\ndiff --git a/remote.c b/remote.c\nindex c10d87c246..4a3e7ba136 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1880,11 +1880,18 @@ static struct ref *get_expanded_map(const struct ref *remote_refs,\n static const struct ref *find_ref_by_name_abbrev(const struct ref *refs, const char *name)\n {\n \tconst struct ref *ref;\n+\tconst struct ref *best_match = NULL;\n+\tint best_score = 0;\n+\n \tfor (ref = refs; ref; ref = ref->next) {\n-\t\tif (refname_match(name, ref->name))\n-\t\t\treturn ref;\n+\t\tint score = refname_match(name, ref->name);\n+\n+\t\tif (best_score < score) {\n+\t\t\tbest_match = ref;\n+\t\t\tbest_score = score;\n+\t\t}\n \t}\n-\treturn NULL;\n+\treturn best_match;\n }\n \n struct ref *get_remote_ref(const struct ref *remote_refs, const char *name)\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex da9ac00557..858381a788 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -535,6 +535,41 @@ test_expect_success \"should be able to fetch with duplicate refspecs\" '\n \t)\n '\n \n+test_expect_success 'LHS of refspec follows ref disambiguation rules' '\n+\tmkdir lhs-ambiguous &&\n+\t(\n+\t\tcd lhs-ambiguous &&\n+\t\tgit init server &&\n+\t\ttest_commit -C server unwanted &&\n+\t\ttest_commit -C server wanted &&\n+\n+\t\tgit init client &&\n+\n+\t\t# Check a name coming after \"refs\" alphabetically ...\n+\t\tgit -C server update-ref refs/heads/s wanted &&\n+\t\tgit -C server update-ref refs/heads/refs/heads/s unwanted &&\n+\t\tgit -C client fetch ../server +refs/heads/s:refs/heads/checkthis &&\n+\t\tgit -C server rev-parse wanted >expect &&\n+\t\tgit -C client rev-parse checkthis >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\t# ... and one before.\n+\t\tgit -C server update-ref refs/heads/q wanted &&\n+\t\tgit -C server update-ref refs/heads/refs/heads/q unwanted &&\n+\t\tgit -C client fetch ../server +refs/heads/q:refs/heads/checkthis &&\n+\t\tgit -C server rev-parse wanted >expect &&\n+\t\tgit -C client rev-parse checkthis >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\t# Tags are preferred over branches like refs/{heads,tags}/*\n+\t\tgit -C server update-ref refs/tags/t wanted &&\n+\t\tgit -C server update-ref refs/heads/t unwanted &&\n+\t\tgit -C client fetch ../server +t:refs/heads/checkthis &&\n+\t\tgit -C server rev-parse wanted >expect &&\n+\t\tgit -C client rev-parse checkthis >actual\n+\t)\n+'\n+\n # configured prune tests\n \n set_config_tristate () {\n-- \n2.18.0-321-gffc6fa0e39\n\n"},{"id":"354173","messageId":"20180801181348.106679-1-jonathantanmy@google.com","threadId":"49001","inReplyTo":"xmqq8t5q821c.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] remote: make refspec follow the same disambiguation rule as local refs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-08-01T18:13:48Z","receivedAt":"2018-08-01T18:13:55Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> +test_expect_success 'LHS of refspec follows ref disambiguation rules' '\n> +\tmkdir lhs-ambiguous &&\n> +\t(\n> +\t\tcd lhs-ambiguous &&\n> +\t\tgit init server &&\n> +\t\ttest_commit -C server unwanted &&\n> +\t\ttest_commit -C server wanted &&\n> +\n> +\t\tgit init client &&\n> +\n> +\t\t# Check a name coming after \"refs\" alphabetically ...\n> +\t\tgit -C server update-ref refs/heads/s wanted &&\n> +\t\tgit -C server update-ref refs/heads/refs/heads/s unwanted &&\n> +\t\tgit -C client fetch ../server +refs/heads/s:refs/heads/checkthis &&\n> +\t\tgit -C server rev-parse wanted >expect &&\n> +\t\tgit -C client rev-parse checkthis >actual &&\n> +\t\ttest_cmp expect actual &&\n> +\n> +\t\t# ... and one before.\n> +\t\tgit -C server update-ref refs/heads/q wanted &&\n> +\t\tgit -C server update-ref refs/heads/refs/heads/q unwanted &&\n> +\t\tgit -C client fetch ../server +refs/heads/q:refs/heads/checkthis &&\n> +\t\tgit -C server rev-parse wanted >expect &&\n> +\t\tgit -C client rev-parse checkthis >actual &&\n> +\t\ttest_cmp expect actual &&\n> +\n> +\t\t# Tags are preferred over branches like refs/{heads,tags}/*\n> +\t\tgit -C server update-ref refs/tags/t wanted &&\n> +\t\tgit -C server update-ref refs/heads/t unwanted &&\n> +\t\tgit -C client fetch ../server +t:refs/heads/checkthis &&\n> +\t\tgit -C server rev-parse wanted >expect &&\n> +\t\tgit -C client rev-parse checkthis >actual\n> +\t)\n> +'\n\nThanks, this looks good to me. Also thanks for adding the \"+\" in the\nfetch commands in the test.\n"},{"id":"354176","messageId":"xmqqo9em6j0a.fsf@gitster-ct.c.googlers.com","threadId":"49001","inReplyTo":"20180801181348.106679-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] remote: make refspec follow the same disambiguation rule as local refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-01T18:26:13Z","receivedAt":"2018-08-01T18:26:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n>> +test_expect_success 'LHS of refspec follows ref disambiguation rules' '\n>> +\tmkdir lhs-ambiguous &&\n>> +\t(\n>> +\t\tcd lhs-ambiguous &&\n>> +\t\tgit init server &&\n>> +\t\ttest_commit -C server unwanted &&\n>> +\t\ttest_commit -C server wanted &&\n>> +\n>> +\t\tgit init client &&\n>> +\n>> +\t\t# Check a name coming after \"refs\" alphabetically ...\n>> +\t\tgit -C server update-ref refs/heads/s wanted &&\n>> +\t\tgit -C server update-ref refs/heads/refs/heads/s unwanted &&\n>> +\t\tgit -C client fetch ../server +refs/heads/s:refs/heads/checkthis &&\n>> +\t\tgit -C server rev-parse wanted >expect &&\n>> +\t\tgit -C client rev-parse checkthis >actual &&\n>> +\t\ttest_cmp expect actual &&\n>> +\n>> +\t\t# ... and one before.\n>> +\t\tgit -C server update-ref refs/heads/q wanted &&\n>> +\t\tgit -C server update-ref refs/heads/refs/heads/q unwanted &&\n>> +\t\tgit -C client fetch ../server +refs/heads/q:refs/heads/checkthis &&\n>> +\t\tgit -C server rev-parse wanted >expect &&\n>> +\t\tgit -C client rev-parse checkthis >actual &&\n>> +\t\ttest_cmp expect actual &&\n>> +\n>> +\t\t# Tags are preferred over branches like refs/{heads,tags}/*\n>> +\t\tgit -C server update-ref refs/tags/t wanted &&\n>> +\t\tgit -C server update-ref refs/heads/t unwanted &&\n>> +\t\tgit -C client fetch ../server +t:refs/heads/checkthis &&\n>> +\t\tgit -C server rev-parse wanted >expect &&\n>> +\t\tgit -C client rev-parse checkthis >actual\n>> +\t)\n>> +'\n>\n> Thanks, this looks good to me. Also thanks for adding the \"+\" in the\n> fetch commands in the test.\n\nYup, otherwise the fetch may fail because \"checkthis\" may have to be\nrewound when we fetch different things.\n"},{"id":"354207","messageId":"20180801230802.GC189024@aiede.svl.corp.google.com","threadId":"49001","inReplyTo":"xmqq8t5q821c.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] remote: make refspec follow the same disambiguation rule as local refs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-01T23:08:02Z","receivedAt":"2018-08-01T23:08:07Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJunio C Hamano wrote:\n\n> When matching a non-wildcard LHS of a refspec against a list of\n> refs, find_ref_by_name_abbrev() returns the first ref that matches\n> using any DWIM rules used by refname_match() in refs.c, even if a\n> better match occurs later in the list of refs.\n\nNicely explained.\n\n[...]\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -487,16 +487,22 @@ static const char *ref_rev_parse_rules[] = {\n>  \tNULL\n>  };\n>  \n> +/*\n> + * Is it possible that the caller meant full_name with abbrev_name?\n> + * If so return a non-zero value to signal \"yes\"; the magnitude of\n> + * the returned value gives the precedence used for disambiguation.\n> + *\n> + * If abbrev_name cannot mean full_name, return 0.\n> + */\n>  int refname_match(const char *abbrev_name, const char *full_name)\n>  {\n>  \tconst char **p;\n>  \tconst int abbrev_name_len = strlen(abbrev_name);\n> +\tconst int num_rules = ARRAY_SIZE(ref_rev_parse_rules) - 1;\n\nThis is assuming ref_rev_parse_rules consists exactly of its items\nfollowed by a NULL terminator, which is potentially a bit subtle.  I\nwonder if we should put\n\n\tstatic const char *ref_rev_parse_rules[] = {\n\t\t\"%.*s\",\n\t\t\"refs/%.*s\",\n\t\t\"refs/tags/%.*s\",\n\t\t\"refs/heads/%.*s\",\n\t\t\"refs/remotes/%.*s\",\n\t\t\"refs/remotes/%.*s/HEAD\",\n\t\tNULL\n\t};\n\t#define NUM_REV_PARSE_RULES (ARRAY_SIZE(ref_rev_parse_rules) - 1)\n\nand then use something like\n\n\tconst int num_rules = NUM_REV_PARSE_RULES;\n\nso that this dependency is more obvious if the ref_rev_parse_rules\nconvention changes later.\n\nAlternatively, what would you think of using the simpler return\nconvention\n\n\treturn p - ref_rev_parse_rules + 1;\n\n?  Or even\n\n\treturn p - ref_rev_parse_rules;\n\nand -1 for \"no match\"?\n\n[...]\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -1880,11 +1880,18 @@ static struct ref *get_expanded_map(const struct ref *remote_refs,\n>  static const struct ref *find_ref_by_name_abbrev(const struct ref *refs, const char *name)\n>  {\n>  \tconst struct ref *ref;\n> +\tconst struct ref *best_match = NULL;\n> +\tint best_score = 0;\n> +\n>  \tfor (ref = refs; ref; ref = ref->next) {\n> -\t\tif (refname_match(name, ref->name))\n> -\t\t\treturn ref;\n> +\t\tint score = refname_match(name, ref->name);\n> +\n> +\t\tif (best_score < score) {\n> +\t\t\tbest_match = ref;\n> +\t\t\tbest_score = score;\n> +\t\t}\n>  \t}\n> -\treturn NULL;\n> +\treturn best_match;\n\nSensible and simple.  If we wanted to make items earlier in the list\nreturn a lower value from refname_match, then we'd need a !best_score\ntest here, which might be what motivates that return value convention.\n\n[...]\n> --- a/t/t5510-fetch.sh\n> +++ b/t/t5510-fetch.sh\n> @@ -535,6 +535,41 @@ test_expect_success \"should be able to fetch with duplicate refspecs\" '\n>  \t)\n>  '\n>  \n> +test_expect_success 'LHS of refspec follows ref disambiguation rules' '\n\nClearly illustrates the bug this fixes, in a way that makes it obvious\nthat a user would prefer the new behavior.  Good.\n\nWith or without the tweak of introducing NUM_REV_PARSE_RULES mentioned\nabove,\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks.\n"},{"id":"354209","messageId":"xmqqo9el4ige.fsf@gitster-ct.c.googlers.com","threadId":"49001","inReplyTo":"20180801230802.GC189024@aiede.svl.corp.google.com","subject":"Re: [PATCH v2] remote: make refspec follow the same disambiguation rule as local refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-02T02:21:05Z","receivedAt":"2018-08-02T02:21:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n>> +\tconst int num_rules = ARRAY_SIZE(ref_rev_parse_rules) - 1;\n>\n> This is assuming ref_rev_parse_rules consists exactly of its items\n> followed by a NULL terminator, which is potentially a bit subtle.  I\n> wonder if we should put\n>\n> \tstatic const char *ref_rev_parse_rules[] = {\n> \t\t\"%.*s\",\n> \t\t\"refs/%.*s\",\n> \t\t\"refs/tags/%.*s\",\n> \t\t\"refs/heads/%.*s\",\n> \t\t\"refs/remotes/%.*s\",\n> \t\t\"refs/remotes/%.*s/HEAD\",\n> \t\tNULL\n> \t};\n> \t#define NUM_REV_PARSE_RULES (ARRAY_SIZE(ref_rev_parse_rules) - 1)\n>\n> and then use something like\n>\n> \tconst int num_rules = NUM_REV_PARSE_RULES;\n\nPerhaps.  If we were to go that length, I'd rather first see if we\ncan lose the sentinel NULL, though.\n\n> Alternatively, what would you think of using the simpler return\n> convention\n>\n> \treturn p - ref_rev_parse_rules + 1;\n>\n> ?  Or even\n>\n> \treturn p - ref_rev_parse_rules;\n>\n> and -1 for \"no match\"?\n\nHeh, that is what I did in the \"how about this\" patch, which made\nthe caller a bit more cumbersome by two comparisons, which in turn\nwas why I rejected the approach.\n\n> Sensible and simple.  If we wanted to make items earlier in the list\n> return a lower value from refname_match, then we'd need a !best_score\n> test here, which might be what motivates that return value convention.\n\nExactly.  See the discussion between JTan and me on his original\npatch.\n\n> [...]\n>> --- a/t/t5510-fetch.sh\n>> +++ b/t/t5510-fetch.sh\n>> @@ -535,6 +535,41 @@ test_expect_success \"should be able to fetch with duplicate refspecs\" '\n>>  \t)\n>>  '\n>>  \n>> +test_expect_success 'LHS of refspec follows ref disambiguation rules' '\n>\n> Clearly illustrates the bug this fixes, in a way that makes it obvious\n> that a user would prefer the new behavior.  Good.\n>\n> With or without the tweak of introducing NUM_REV_PARSE_RULES mentioned\n> above,\n>\n> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nOne thing I forgot to mention.\n\nWhen asking to fetch T, in order to be able to favor refs/tags/T\nover refs/heads/T at the fetching end, you would have to be able to\n*see* both, so all 6 variants \"T\", \"refs/tags/T\", \"refs/heads/T\",\n\"refs/remotes/T\", \"refs/remotes/T/HEAD\" must be asked to be shown\nwhen the ls-remote limiting is in effect.  Since the ls-remote\nfiltering is relatively new development, we may further find subtle\nremaining bugs, if there still are some.\n\n"},{"id":"354210","messageId":"20180802022603.GE189024@aiede.svl.corp.google.com","threadId":"49001","inReplyTo":"xmqqo9el4ige.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] remote: make refspec follow the same disambiguation rule as local refs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-02T02:26:03Z","receivedAt":"2018-08-02T02:26:08Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> One thing I forgot to mention.\n>\n> When asking to fetch T, in order to be able to favor refs/tags/T\n> over refs/heads/T at the fetching end, you would have to be able to\n> *see* both, so all 6 variants \"T\", \"refs/tags/T\", \"refs/heads/T\",\n> \"refs/remotes/T\", \"refs/remotes/T/HEAD\" must be asked to be shown\n> when the ls-remote limiting is in effect.  Since the ls-remote\n> filtering is relatively new development, we may further find subtle\n> remaining bugs, if there still are some.\n\nFortunately, the fetch code does already do that. ;-)\n\nJonathan\n"},{"id":"354335","messageId":"xmqq8t5oz9xi.fsf_-_@gitster-ct.c.googlers.com","threadId":"49001","inReplyTo":"xmqqo9el4ige.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v3] remote: make refspec follow the same disambiguation rule as local refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-02T22:21:45Z","receivedAt":"2018-08-02T22:21:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When matching a non-wildcard LHS of a refspec against a list of\nrefs, find_ref_by_name_abbrev() returns the first ref that matches\nusing any DWIM rules used by refname_match() in refs.c, even if a\nbetter match occurs later in the list of refs.\n\nThis causes unexpected behavior when (for example) fetching using\nthe refspec \"refs/heads/s:<something>\" from a remote with both\n\"refs/heads/refs/heads/s\" and \"refs/heads/s\"; even if the former was\ninadvertently created, one would still expect the latter to be\nfetched.  Similarly, when both a tag T and a branch T exist,\nfetching T should favor the tag, just like how local refname\ndisambiguation rule works.  But because the code walks over\nls-remote output from the remote, which happens to be sorted in\nalphabetical order and has refs/heads/T before refs/tags/T, a\nrequest to fetch T is (mis)interpreted as fetching refs/heads/T.\n\nUpdate refname_match(), all of whose current callers care only if it\nreturns non-zero (i.e. matches) to see if an abbreviated name can\nmean the full name being tested, so that it returns a positive\ninteger whose magnitude can be used to tell the precedence, and fix\nthe find_ref_by_name_abbrev() function not to stop at the first\nmatch but find the match with the highest precedence.\n\nThis is based on an earlier work, which special cased only the exact\nmatches, by Jonathan Tan.\n\nHelped-by: Jonathan Tan <jonathantanmy@google.com>\nHelped-by: Jonathan Nieder <jrnieder@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * OK, with the NUM_REV_PARSE_RULES constant this time.\n\n refs.c           | 18 +++++++++++++-----\n remote.c         | 13 ++++++++++---\n t/t5510-fetch.sh | 35 +++++++++++++++++++++++++++++++++++\n 3 files changed, 58 insertions(+), 8 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 20ba82b434..2ca715d099 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -487,16 +487,24 @@ static const char *ref_rev_parse_rules[] = {\n \tNULL\n };\n \n+#define NUM_REV_PARSE_RULES (ARRAY_SIZE(ref_rev_parse_rules) - 1)\n+\n+/*\n+ * Is it possible that the caller meant full_name with abbrev_name?\n+ * If so return a non-zero value to signal \"yes\"; the magnitude of\n+ * the returned value gives the precedence used for disambiguation.\n+ *\n+ * If abbrev_name cannot mean full_name, return 0.\n+ */\n int refname_match(const char *abbrev_name, const char *full_name)\n {\n \tconst char **p;\n \tconst int abbrev_name_len = strlen(abbrev_name);\n+\tconst int num_rules = NUM_REV_PARSE_RULES;\n \n-\tfor (p = ref_rev_parse_rules; *p; p++) {\n-\t\tif (!strcmp(full_name, mkpath(*p, abbrev_name_len, abbrev_name))) {\n-\t\t\treturn 1;\n-\t\t}\n-\t}\n+\tfor (p = ref_rev_parse_rules; *p; p++)\n+\t\tif (!strcmp(full_name, mkpath(*p, abbrev_name_len, abbrev_name)))\n+\t\t\treturn &ref_rev_parse_rules[num_rules] - p;\n \n \treturn 0;\n }\ndiff --git a/remote.c b/remote.c\nindex c10d87c246..4a3e7ba136 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1880,11 +1880,18 @@ static struct ref *get_expanded_map(const struct ref *remote_refs,\n static const struct ref *find_ref_by_name_abbrev(const struct ref *refs, const char *name)\n {\n \tconst struct ref *ref;\n+\tconst struct ref *best_match = NULL;\n+\tint best_score = 0;\n+\n \tfor (ref = refs; ref; ref = ref->next) {\n-\t\tif (refname_match(name, ref->name))\n-\t\t\treturn ref;\n+\t\tint score = refname_match(name, ref->name);\n+\n+\t\tif (best_score < score) {\n+\t\t\tbest_match = ref;\n+\t\t\tbest_score = score;\n+\t\t}\n \t}\n-\treturn NULL;\n+\treturn best_match;\n }\n \n struct ref *get_remote_ref(const struct ref *remote_refs, const char *name)\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex da9ac00557..858381a788 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -535,6 +535,41 @@ test_expect_success \"should be able to fetch with duplicate refspecs\" '\n \t)\n '\n \n+test_expect_success 'LHS of refspec follows ref disambiguation rules' '\n+\tmkdir lhs-ambiguous &&\n+\t(\n+\t\tcd lhs-ambiguous &&\n+\t\tgit init server &&\n+\t\ttest_commit -C server unwanted &&\n+\t\ttest_commit -C server wanted &&\n+\n+\t\tgit init client &&\n+\n+\t\t# Check a name coming after \"refs\" alphabetically ...\n+\t\tgit -C server update-ref refs/heads/s wanted &&\n+\t\tgit -C server update-ref refs/heads/refs/heads/s unwanted &&\n+\t\tgit -C client fetch ../server +refs/heads/s:refs/heads/checkthis &&\n+\t\tgit -C server rev-parse wanted >expect &&\n+\t\tgit -C client rev-parse checkthis >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\t# ... and one before.\n+\t\tgit -C server update-ref refs/heads/q wanted &&\n+\t\tgit -C server update-ref refs/heads/refs/heads/q unwanted &&\n+\t\tgit -C client fetch ../server +refs/heads/q:refs/heads/checkthis &&\n+\t\tgit -C server rev-parse wanted >expect &&\n+\t\tgit -C client rev-parse checkthis >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\t# Tags are preferred over branches like refs/{heads,tags}/*\n+\t\tgit -C server update-ref refs/tags/t wanted &&\n+\t\tgit -C server update-ref refs/heads/t unwanted &&\n+\t\tgit -C client fetch ../server +t:refs/heads/checkthis &&\n+\t\tgit -C server rev-parse wanted >expect &&\n+\t\tgit -C client rev-parse checkthis >actual\n+\t)\n+'\n+\n # configured prune tests\n \n set_config_tristate () {\n-- \n2.18.0-321-gffc6fa0e39\n\n"}]}