{"thread":{"id":"48996","subject":"[PATCH] remote: prefer exact matches when using refspecs","startedAt":"2018-07-31T21:18:39Z","lastAt":"2018-08-01T00:32:43Z","messageCount":6,"participants":["Jonathan Tan","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"354126","messageId":"20180731211832.142014-1-jonathantanmy@google.com","threadId":"48996","inReplyTo":null,"subject":"[PATCH] remote: prefer exact matches when using refspecs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-07-31T21:18:32Z","receivedAt":"2018-07-31T21:18:39Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When matching a non-wildcard LHS of a refspec against a list of refs,\nfind_ref_by_name_abbrev() returns the first ref that matches using the\nDWIM rules used by refname_match() in refs.c, even if an exact match\noccurs later in the list of refs.\n\nThis causes unexpected behavior when (for example) fetching using the\nrefspec \"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 fetched.)\n\nThis problem has only been observed when the desired ref comes after the\nundesired ref in alphabetical order. However, for completeness, the test\nin this patch also checks what happens when the desired ref comes first\nalphabetically.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n remote.c         |  7 +++++--\n t/t5510-fetch.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 33 insertions(+), 2 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 3fd43453f..eeffe3488 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1687,12 +1687,15 @@ static struct ref *get_expanded_map(const struct ref *remote_refs,\n \n static const struct ref *find_ref_by_name_abbrev(const struct ref *refs, const char *name)\n {\n+\tconst struct ref *best_match = NULL;\n \tconst struct ref *ref;\n \tfor (ref = refs; ref; ref = ref->next) {\n-\t\tif (refname_match(name, ref->name))\n+\t\tif (!strcmp(name, ref->name))\n \t\t\treturn ref;\n+\t\tif (refname_match(name, ref->name))\n+\t\t\tbest_match = ref;\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 e402aee6a..da88f35f0 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -535,6 +535,34 @@ test_expect_success \"should be able to fetch with duplicate refspecs\" '\n \t)\n '\n \n+test_expect_success 'LHS of refspec prefers exact matches' '\n+\tmkdir lhs-exact &&\n+\t(\n+\t\tcd lhs-exact &&\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+\t)\n+'\n+\n # configured prune tests\n \n set_config_tristate () {\n-- \n2.18.0.345.g5c9ce644c3-goog\n\n"},{"id":"354127","messageId":"20180731213120.GA113554@aiede.svl.corp.google.com","threadId":"48996","inReplyTo":"20180731211832.142014-1-jonathantanmy@google.com","subject":"Re: [PATCH] remote: prefer exact matches when using refspecs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-07-31T21:31:20Z","receivedAt":"2018-07-31T21:31:46Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJonathan Tan wrote:\n\n> When matching a non-wildcard LHS of a refspec against a list of refs,\n> find_ref_by_name_abbrev() returns the first ref that matches using the\n> DWIM rules used by refname_match() in refs.c, even if an exact match\n> occurs later in the list of refs.\n>\n> This causes unexpected behavior when (for example) fetching using the\n> 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\n> inadvertently created, one would still expect the latter to be fetched.)\n>\n> This problem has only been observed when the desired ref comes after the\n> undesired ref in alphabetical order. However, for completeness, the test\n> in this patch also checks what happens when the desired ref comes first\n> alphabetically.\n>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n>  remote.c         |  7 +++++--\n>  t/t5510-fetch.sh | 28 ++++++++++++++++++++++++++++\n>  2 files changed, 33 insertions(+), 2 deletions(-)\n\nVery clear analysis --- thank you.\n\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -1687,12 +1687,15 @@ static struct ref *get_expanded_map(const struct ref *remote_refs,\n>  \n>  static const struct ref *find_ref_by_name_abbrev(const struct ref *refs, const char *name)\n>  {\n> +\tconst struct ref *best_match = NULL;\n>  \tconst struct ref *ref;\n>  \tfor (ref = refs; ref; ref = ref->next) {\n> -\t\tif (refname_match(name, ref->name))\n> +\t\tif (!strcmp(name, ref->name))\n>  \t\t\treturn ref;\n> +\t\tif (refname_match(name, ref->name))\n\nShould this check be\n\n\t\tif (!best_match && refname_match(name, ref->name))\n\n?  Otherwise, this would make us prefer the last ref instead of the\nfirst (which probably doesn't matter but would be an unintended\nbehavior change).\n\n> +\t\t\tbest_match = ref;\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)\n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> index e402aee6a..da88f35f0 100755\n> --- a/t/t5510-fetch.sh\n> +++ b/t/t5510-fetch.sh\n> @@ -535,6 +535,34 @@ test_expect_success \"should be able to fetch with duplicate refspecs\" '\n>  \t)\n>  '\n>  \n> +test_expect_success 'LHS of refspec prefers exact matches' '\n\nNice.\n\nWith or without the suggested tweak,\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks for a pleasant read.\n\n> +\tmkdir lhs-exact &&\n> +\t(\n> +\t\tcd lhs-exact &&\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> +\t)\n> +'\n> +\n>  # configured prune tests\n>  \n>  set_config_tristate () {\n"},{"id":"354130","messageId":"xmqq600v9imy.fsf@gitster-ct.c.googlers.com","threadId":"48996","inReplyTo":"20180731211832.142014-1-jonathantanmy@google.com","subject":"Re: [PATCH] remote: prefer exact matches when using refspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-31T21:53:41Z","receivedAt":"2018-07-31T21:53:46Z","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> When matching a non-wildcard LHS of a refspec against a list of refs,\n> find_ref_by_name_abbrev() returns the first ref that matches using the\n> DWIM rules used by refname_match() in refs.c, even if an exact match\n> occurs later in the list of refs.\n\nWhen you have refs/heads/refs/heads/s and refs/heads/s, and if you\nask for refs/heads/s, you want that exact match (i.e. the latter) to\ntake precedence over DWIMmed refs/heads/refs/heads/s.\n\nWhat is unfortunate is that ref_rev_parse_rules[] array already\nexpresses that preference by listing the \"fullname\" choice \"%.*s\"\nbefore other DWIM choices like \"refs/heads/%.*s\".\n\nNow we iterate over refs we say from ls-remote output, and with the\nupdated one, the logic _manually_ gives the precedence to the first\nentry in ref_rev_parse_rules[], so in that \"I have a branch s and\nalso another branch refs/heads/s\" case, that may happen to work, but\nwould the updated code do the right thing when you have entries that\ncan match, say, both second and third entry in the rules[] and\ntiebreak correctly the same way?  Say you ask for \"tags/T\" when\nthere are \"refs/tags/T\" and \"refs/heads/tags/T\" at the same time in\nthe refs linked list.  None of the ref on refs list trigger\n!strcmp() as there is no exact mqatch, and refname_match() would say\n\"Yeah I see a match\" when checking \"refs/heads/tags/T\" and say it is\nthe best match.  Then it finds \"refs/tags/T\" also on the refs list\nand finds it also matches user-supplied \"tags/T\".\n\nIn order to resolve this correctly with the precedence rules, I\nthink you need to make refname_match() return the precedence number\n(e.g. give 1 to \"%.*s\", 2 to \"refs/%.*s\", etc., using the index in\nref_rev_parse_rules[] array), and make this loop keep track of the\n\"best\" match paying attention to the returned precedence.\n\n> This causes unexpected behavior when (for example) fetching using the\n> 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\n> inadvertently created, one would still expect the latter to be fetched.)\n>\n> This problem has only been observed when the desired ref comes after the\n> undesired ref in alphabetical order. However, for completeness, the test\n> in this patch also checks what happens when the desired ref comes first\n> alphabetically.\n>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n>  remote.c         |  7 +++++--\n>  t/t5510-fetch.sh | 28 ++++++++++++++++++++++++++++\n>  2 files changed, 33 insertions(+), 2 deletions(-)\n>\n> diff --git a/remote.c b/remote.c\n> index 3fd43453f..eeffe3488 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -1687,12 +1687,15 @@ static struct ref *get_expanded_map(const struct ref *remote_refs,\n>  \n>  static const struct ref *find_ref_by_name_abbrev(const struct ref *refs, const char *name)\n>  {\n> +\tconst struct ref *best_match = NULL;\n>  \tconst struct ref *ref;\n>  \tfor (ref = refs; ref; ref = ref->next) {\n> -\t\tif (refname_match(name, ref->name))\n> +\t\tif (!strcmp(name, ref->name))\n>  \t\t\treturn ref;\n> +\t\tif (refname_match(name, ref->name))\n> +\t\t\tbest_match = ref;\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)\n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> index e402aee6a..da88f35f0 100755\n> --- a/t/t5510-fetch.sh\n> +++ b/t/t5510-fetch.sh\n> @@ -535,6 +535,34 @@ test_expect_success \"should be able to fetch with duplicate refspecs\" '\n>  \t)\n>  '\n>  \n> +test_expect_success 'LHS of refspec prefers exact matches' '\n> +\tmkdir lhs-exact &&\n> +\t(\n> +\t\tcd lhs-exact &&\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> +\t)\n> +'\n> +\n>  # configured prune tests\n>  \n>  set_config_tristate () {\n"},{"id":"354131","messageId":"xmqq1sbj9h08.fsf@gitster-ct.c.googlers.com","threadId":"48996","inReplyTo":"xmqq600v9imy.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] remote: prefer exact matches when using refspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-31T22:28:55Z","receivedAt":"2018-07-31T22:29:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> In order to resolve this correctly with the precedence rules, I\n> think you need to make refname_match() return the precedence number\n> (e.g. give 1 to \"%.*s\", 2 to \"refs/%.*s\", etc., using the index in\n> ref_rev_parse_rules[] array), and make this loop keep track of the\n> \"best\" match paying attention to the returned precedence.\n\nThat is, something like this, perhaps.  The resulting behaviour\nshould match how \"git rev-parse X\" would give precedence to tag X\nover branch X by going this route.  What do you think?\n\n refs.c   |  8 +++-----\n remote.c | 13 ++++++++++---\n 2 files changed, 13 insertions(+), 8 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 457fb78057..fd1a7f7478 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -495,11 +495,9 @@ int refname_match(const char *abbrev_name, const char *full_name)\n \tconst char **p;\n \tconst int abbrev_name_len = strlen(abbrev_name);\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 (p - ref_rev_parse_rules) + 1;\n \n \treturn 0;\n }\ndiff --git a/remote.c b/remote.c\nindex 86e6098774..ed2f80e45c 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1689,11 +1689,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 = -1;\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 ((score && (best_score < 0 || score < best_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)\n"},{"id":"354136","messageId":"20180731233332.187328-1-jonathantanmy@google.com","threadId":"48996","inReplyTo":"xmqq1sbj9h08.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] remote: prefer exact matches when using refspecs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-07-31T23:33:32Z","receivedAt":"2018-07-31T23:33:37Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> That is, something like this, perhaps.  The resulting behaviour\n> should match how \"git rev-parse X\" would give precedence to tag X\n> over branch X by going this route.  What do you think?\n\n[snip]\n\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 = -1;\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 ((score && (best_score < 0 || score < best_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\nThis looks good to me. I've checked that refname_match (and\nbranch_merge_matches(), which returns the result of refname_match()\ndirectly) is only used in \"if\" contexts, so making it return a value\nother than 1 is fine.\n\nI would initialize best_score to INT_MAX to avoid needing the\n\"best_score < 0\" comparison, but don't feel strongly about it.\n"},{"id":"354140","messageId":"xmqqr2jj7wpk.fsf@gitster-ct.c.googlers.com","threadId":"48996","inReplyTo":"20180731233332.187328-1-jonathantanmy@google.com","subject":"Re: [PATCH] remote: prefer exact matches when using refspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-01T00:32:39Z","receivedAt":"2018-08-01T00:32:43Z","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> This looks good to me. I've checked that refname_match (and\n> branch_merge_matches(), which returns the result of refname_match()\n> directly) is only used in \"if\" contexts, so making it return a value\n> other than 1 is fine.\n\nYes, the log message should say that existing callers only care if\nthe returned value is 0 or not (i.e. if we have any match).\n\n> I would initialize best_score to INT_MAX to avoid needing the\n> \"best_score < 0\" comparison, but don't feel strongly about it.\n\nIf we want to lose that \"have we seen any possible result?\" check, I\nthink defining (ARRAY_SIZE(ref_rev_parse_rules) - p) as the score,\nso that the \"full path\" gets score 6 (or whatever) and \"some remote\ntracking name\" (like \"origin->refs/remotes/origin/HEAD\") gets score\nof 1 (smallest but true) may make more sense.  Then, start the best\nscore at 0 and every time we get a score strictly better than the\nbest so far, we overwrite the best.  That way, we can even lose the\n\"did we get any positive score?\" check, too, and making the\ncondition in the inner loop quite simple, i.e.\n\n\tint best_score = 0;\n\t...\n\tfor (ref = refs; ref; ref = ref->next) {\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\nWe need a commit log message (hopefully we can lift most parts from\nyour patch in this thread) and a test update to ensure that the same\nprecedence order used as ref resolution (i.e. tags get higher prio\nover branches, etc.) in addition to the test you had in your patch.\n\nThanks.\n"}]}