Re: [PATCH] remote: prefer exact matches when using refspecs
From: Junio C Hamano <hidden>
Date: 2018-07-31 21:53:46
Jonathan Tan [off-list ref] writes:
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 the DWIM rules used by refname_match() in refs.c, even if an exact match occurs later in the list of refs.
When you have refs/heads/refs/heads/s and refs/heads/s, and if you ask for refs/heads/s, you want that exact match (i.e. the latter) to take precedence over DWIMmed refs/heads/refs/heads/s. What is unfortunate is that ref_rev_parse_rules[] array already expresses that preference by listing the "fullname" choice "%.*s" before other DWIM choices like "refs/heads/%.*s". Now we iterate over refs we say from ls-remote output, and with the updated one, the logic _manually_ gives the precedence to the first entry in ref_rev_parse_rules[], so in that "I have a branch s and also another branch refs/heads/s" case, that may happen to work, but would the updated code do the right thing when you have entries that can match, say, both second and third entry in the rules[] and tiebreak correctly the same way? Say you ask for "tags/T" when there are "refs/tags/T" and "refs/heads/tags/T" at the same time in the refs linked list. None of the ref on refs list trigger !strcmp() as there is no exact mqatch, and refname_match() would say "Yeah I see a match" when checking "refs/heads/tags/T" and say it is the best match. Then it finds "refs/tags/T" also on the refs list and finds it also matches user-supplied "tags/T". In order to resolve this correctly with the precedence rules, I think you need to make refname_match() return the precedence number (e.g. give 1 to "%.*s", 2 to "refs/%.*s", etc., using the index in ref_rev_parse_rules[] array), and make this loop keep track of the "best" match paying attention to the returned precedence.
quoted hunk
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.) This problem has only been observed when the desired ref comes after the undesired ref in alphabetical order. However, for completeness, the test in this patch also checks what happens when the desired ref comes first alphabetically. Signed-off-by: Jonathan Tan <redacted> --- remote.c | 7 +++++-- t/t5510-fetch.sh | 28 ++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-)diff --git a/remote.c b/remote.c index 3fd43453f..eeffe3488 100644 --- a/remote.c +++ b/remote.c@@ -1687,12 +1687,15 @@ 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 *best_match = NULL; const struct ref *ref; for (ref = refs; ref; ref = ref->next) { - if (refname_match(name, ref->name)) + if (!strcmp(name, ref->name)) return ref; + if (refname_match(name, ref->name)) + best_match = ref; } - 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 e402aee6a..da88f35f0 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh@@ -535,6 +535,34 @@ test_expect_success "should be able to fetch with duplicate refspecs" ' ) ' +test_expect_success 'LHS of refspec prefers exact matches' ' + mkdir lhs-exact && + ( + cd lhs-exact && + 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 + ) +' + # configured prune tests set_config_tristate () {