[PATCH] push: make "HEAD:tags/my-tag" consistently push to a branch

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

When a refspec like "HEAD:tags/my-tag" is pushed where "HEAD" is a
branch, we'll push a *branch* that'll be located at
"refs/heads/tags/my-tag". This is part of the rather straightforward
rules I documented in 2219c09e23 ("push doc: document the DWYM
behavior pushing to unqualified <dst>", 2018-11-13).

However, if there exists a "refs/tags/my-tag" on the remote the
count_refspec_match() logic will, as a result of calling
refname_match(), match partially-qualified RHS of the refspec
"refs/tags/my-tag", because it's in a loop where it tries to match
"tags/my-tag" to "refs/tags/my-tag', then "refs/tags/tags/my-tag" etc.

This resulted in a case[1] where someone on LKML did:

    git push kvm +HEAD:tags/for-linus

Which would have created a new "refs/heads/tags/for-linus" branch in
their "kvm" repository. But since they happened to have an existing
"refs/tags/for-linus" reference we pushed there instead, and replaced
an annotated tag with a lightweight tag.

We do want a RHS ref like "master" to match "refs/heads/master", but
it's confusing and dangerous that the DWYM behavior for matching
partial RHS refspecs acts differently when the start of the RHS
happens to be a second-level namespace under "refs/" namespace like
"tags".

Now we'll print out the following advice when this happens, and act
differently as described therein:

    hint: The <dst> part of the refspec matched both of:
    hint:
    hint:   1. tags/my-tag -> refs/tags/my-tag
    hint:   2. tags/my-tag -> refs/heads/tags/my-tag
    hint:
    hint: Earlier versions of git would have picked (1) as the RHS starts
    hint: with a second-level ref prefix which could be fully-qualified by
    hint: adding 'refs/' in front of it. We now pick (2) which uses the prefix
    hint: inferred from the <src> part of the refspec.
    hint:
    hint: See the "<refspec>..." rules  discussed in 'git help push'.

An earlier version of this patch[2] used the much more heavy-handed
approach of changing this logic in refname_match(). As shown from the
tests that patch needed to modify that results in changes that are
overzealous for fixing this push-specific issue.

The right place to fix this is in match_explicit(). There we can see
if we have both a DWYM match and a match based on the prefix of the
LHS of the refspec, in those cases the match based on the LHS's ref
prefix should win.

1. https://lore.kernel.org/lkml/2d55fd2a-afbf-1b7c-ca82-8bffaa18e0d0@redhat.com/
2. https://public-inbox.org/git/20190526225445.21618-1-avarab@gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

Now that the 2.22.0 release is out I cleaned this up into a more
sensible patch.

 Documentation/config/advice.txt |  7 +++++++
 Documentation/git-push.txt      | 13 +++++++++++++
 advice.c                        |  2 ++
 advice.h                        |  1 +
 remote.c                        | 23 ++++++++++++++++++++++-
 t/t5505-remote.sh               | 18 ++++++++++++++++++
 6 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index ec4f6ae658..36cb3db63a 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -37,6 +37,13 @@ advice.*::
 		we can still suggest that the user push to either
 		refs/heads/* or refs/tags/* based on the type of the
 		source object.
+	pushPartialAmbigiousName::
+		Shown when linkgit:git-push[1] is given a refspec
+		where the <src> in earlier versions of Git would have
+		matched a <dst> on the remote based on its existence
+		over appending a prefix based on the type of the
+		<src>. See the "<refspec>..." documentation in
+		linkgit:git-push[1] for details.
 	statusHints::
 		Show directions on how to proceed from the current
 		state in the output of linkgit:git-status[1], in
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 6a8a0d958b..5c46ef5e59 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -84,6 +84,19 @@ is ambiguous.
 
 * If <src> resolves to a ref starting with refs/heads/ or refs/tags/,
   then prepend that to <dst>.
++
+Versions of Git before 2.23.0 would override this rule and match
+e.g. `HEAD:tags/mark` to either `refs/tags/mark` or `refs/tags/mark`
+depending on, respectively, if `refs/tags/mark` existed or not on the
+remote.
++
+We'll now consistently pick `refs/heads/tags/mark` based on this rule
+and so that we're not so eager in guessing the <dst> on the remote
+that we'll pick a different <dst> based on what refs exist there
+already than we otherwise would have. This exception guards for cases
+where the match would be different due to a subsequent
+"refs/{tags,heads,remotes}/" matching rule". than a plain "refs/"
+prefix match.
 
 * Other ambiguity resolutions might be added in the future, but for
   now any other cases will error out with an error indicating what we
diff --git a/advice.c b/advice.c
index ce5f374ecd..c9217834e2 100644
--- a/advice.c
+++ b/advice.c
@@ -10,6 +10,7 @@ int advice_push_already_exists = 1;
 int advice_push_fetch_first = 1;
 int advice_push_needs_force = 1;
 int advice_push_unqualified_ref_name = 1;
+int advice_partial_ambiguous_ref_name = 1;
 int advice_status_hints = 1;
 int advice_status_u_option = 1;
 int advice_commit_before_merge = 1;
@@ -66,6 +67,7 @@ static struct {
 	{ "pushFetchFirst", &advice_push_fetch_first },
 	{ "pushNeedsForce", &advice_push_needs_force },
 	{ "pushUnqualifiedRefName", &advice_push_unqualified_ref_name },
+	{ "pushPartialAmbigiousName", &advice_partial_ambiguous_ref_name },
 	{ "statusHints", &advice_status_hints },
 	{ "statusUoption", &advice_status_u_option },
 	{ "commitBeforeMerge", &advice_commit_before_merge },
diff --git a/advice.h b/advice.h
index e50f02cdfe..027ec396cf 100644
--- a/advice.h
+++ b/advice.h
@@ -10,6 +10,7 @@ extern int advice_push_already_exists;
 extern int advice_push_fetch_first;
 extern int advice_push_needs_force;
 extern int advice_push_unqualified_ref_name;
+extern int advice_partial_ambiguous_ref_name;
 extern int advice_status_hints;
 extern int advice_status_u_option;
 extern int advice_commit_before_merge;
diff --git a/remote.c b/remote.c
index e50f7602ed..be226fac11 100644
--- a/remote.c
+++ b/remote.c
@@ -1066,7 +1066,7 @@ static int match_explicit(struct ref *src, struct ref *dst,
 			  struct ref ***dst_tail,
 			  struct refspec_item *rs)
 {
-	struct ref *matched_src, *matched_dst;
+	struct ref *matched_src, *matched_dst, *tmp;
 	int allocated_src;
 
 	const char *dst_value = rs->dst;
@@ -1094,6 +1094,27 @@ static int match_explicit(struct ref *src, struct ref *dst,
 
 	switch (count_refspec_match(dst_value, dst, &matched_dst)) {
 	case 1:
+		if ((starts_with(dst_value, "tags/") ||
+		     starts_with(dst_value, "heads/") ||
+		     starts_with(dst_value, "remotes/")) &&
+		    (dst_guess = guess_ref(dst_value, matched_src))) {
+			tmp = make_linked_ref(dst_guess, dst_tail);
+			if (advice_partial_ambiguous_ref_name)
+				advise(_("The <dst> part of the refspec matched both of:\n"
+					 "\n"
+					 "	1. %1$s -> %2$s\n"
+					 "	2. %1$s -> %3$s\n"
+					 "\n"
+					 "Earlier versions of git would have picked (1) as the RHS starts\n"
+					 "with a second-level ref prefix which could be fully-qualified by\n"
+					 "adding 'refs/' in front of it. We now pick (2) which uses the prefix\n"
+					 "inferred from the <src> part of the refspec.\n"
+					 "\n"
+					 "See the \"<refspec>...\" rules  discussed in 'git help push'.\n"),
+					dst_value, matched_dst->name, tmp->name);
+			matched_dst = tmp;
+		}
+
 		break;
 	case 0:
 		if (starts_with(dst_value, "refs/")) {
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 883b32efa0..4d54f90ae3 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -1277,4 +1277,22 @@ test_expect_success 'refs/remotes/* <src> refspec and unqualified <dst> DWIM and
 	)
 '
 
+test_expect_success 'HEAD:tags/A and HEAD:tags/B should not be different depending on if refs/tags/[AB] exists or not' '
+	git clone "file://$PWD/two" tags-match &&
+	(
+		cd tags-match &&
+		test_commit A &&
+		git rev-parse HEAD >expected &&
+
+		git push origin HEAD:tags/my-not-a-tag &&
+		git -C ../two rev-parse refs/heads/tags/my-not-a-tag >actual &&
+		test_cmp expected actual &&
+
+		git push origin HEAD:tags/my-tag 2>advice &&
+		test_i18ngrep "hint: The <dst> part of the refspec matched both of" advice &&
+		git -C ../two rev-parse refs/heads/tags/my-tag >actual &&
+		test_cmp expected actual
+	)
+'
+
 test_done
-- 
2.22.0.455.g172b71a6c5