Re: [GIT PULL] KVM changes for Linux 5.2-rc2

Re: [GIT PULL] KVM changes for Linux 5.2-rc2

[Linus Torvalds]

On Sun, May 26, 2019 at 10:53 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> The interesting thing is that not only git will treat lightweight tags
> like, well, tags:

Yeah, that's very much by design - lightweight tags are very
comvenient for local temporary stuff where you don't want signing etc
(think automated test infrastructure, or just local reminders).

> In addition, because I _locally_ had a tag object that
> pointed to the same commit and had the same name, git-request-pull
> included my local tag's message in its output!  I wonder if this could
> be considered a bug.

Yeah, I think git request-pull should at least *warn* about the tag
not being the same object locally as in the remote you're asking me to
pull.

Are you sure you didn't get a warning, and just missed it? But adding
Junio and the Git list just as a possible heads-up for this in case
git request-pull really only compares the object the tag points to,
rather than the SHA1 of the tag itself.

             Linus

[RFC/PATCH] refs: tone down the dwimmery in refname_match() for {heads,tags,remotes}/*

[Ævar Arnfjörð Bjarmason]

When a refspec like HEAD:tags/x is pushed where HEAD is a branch,
we'll push a *branch* that'll be located at "refs/heads/tags/x". 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/x on the remote the
count_refspec_match() logic will, as a result of calling
refname_match() match the detected branch type of the LHS of the
refspec to refs/tags/x, because it's in a loop where it tries to match
"tags/x" to "refs/tags/X', then "refs/tags/tags/x" 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 "tags/for-linus" branch in their "kvm"
repository, except because they happened to have an existing
"refs/tags/for-linus" reference we pushed there instead, and replaced
an annotated tag with a lightweight tag.

Let's tone this down a bit and not match the more general expansions
if they'd overlap with later expansions.

This patch is a hack, and should not be applied. We probably want to
fix this for "push", but we use refname_match() all over the place. We
probably want to start by undoing part of
54457fe509 ("refname_match(): always use the rules in
ref_rev_parse_rules", 2014-01-14) and having special rules just for
push.

Furthermore ref_rev_parse_rules is used elsewhere, should we be doing
this in other places? I think not if we undo most of 54457fe509 and
can just have a custom matcher just for count_refspec_match(). That
one shouldn't need any sort of magic, because elsewhere in the remote
push DWYM code we try to add implicit refs/{tags,heads}/ prefixes.

As the t/t5150-request-pull.sh change shows this results in a failing
test where a local "full" branch is being pushed to a remote
"refs/tags/full". So maybe this is something LKML people actually want
for some reason.

1. https://lore.kernel.org/lkml/2d55fd2a-afbf-1b7c-ca82-8bffaa18e0d0@redhat.com/

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

On Sun, May 26 2019, Linus Torvalds wrote:

> On Sun, May 26, 2019 at 10:53 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> The interesting thing is that not only git will treat lightweight tags
>> like, well, tags:
>
> Yeah, that's very much by design - lightweight tags are very
> comvenient for local temporary stuff where you don't want signing etc
> (think automated test infrastructure, or just local reminders).
>
>> In addition, because I _locally_ had a tag object that
>> pointed to the same commit and had the same name, git-request-pull
>> included my local tag's message in its output!  I wonder if this could
>> be considered a bug.
>
> Yeah, I think git request-pull should at least *warn* about the tag
> not being the same object locally as in the remote you're asking me to
> pull.
>
> Are you sure you didn't get a warning, and just missed it? But adding
> Junio and the Git list just as a possible heads-up for this in case
> git request-pull really only compares the object the tag points to,
> rather than the SHA1 of the tag itself.

This behavior looks like a bug to me. This RFC-quality patch is an
initial stab at fixing it, and is all I had time for today.

 refs.c                   |  8 +++++++-
 t/t5150-request-pull.sh  |  2 +-
 t/t5505-remote.sh        | 17 +++++++++++++++++
 t/t9101-git-svn-props.sh | 12 ++++++------
 4 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/refs.c b/refs.c
index 92d1f6dbdd..729b921328 100644
--- a/refs.c
+++ b/refs.c
@@ -514,9 +514,15 @@ int refname_match(const char *abbrev_name, const char *full_name)
 	const int abbrev_name_len = strlen(abbrev_name);
 	const int num_rules = NUM_REV_PARSE_RULES;
 
-	for (p = ref_rev_parse_rules; *p; p++)
+	for (p = ref_rev_parse_rules; *p; p++) {
+		if (!strcmp(*p, "refs/%.*s") &&
+		    (starts_with(abbrev_name, "tags/") ||
+		     starts_with(abbrev_name, "heads/") ||
+		     starts_with(abbrev_name, "remotes/")))
+			continue;
 		if (!strcmp(full_name, mkpath(*p, abbrev_name_len, abbrev_name)))
 			return &ref_rev_parse_rules[num_rules] - p;
+	}
 
 	return 0;
 }
diff --git a/t/t5150-request-pull.sh b/t/t5150-request-pull.sh
index fca001eb9b..0265871cf4 100755
--- a/t/t5150-request-pull.sh
+++ b/t/t5150-request-pull.sh
@@ -212,7 +212,7 @@ test_expect_success 'pull request format' '
 		cd local &&
 		git checkout initial &&
 		git merge --ff-only master &&
-		git push origin tags/full &&
+		git push origin full:refs/tags/full &&
 		git request-pull initial "$downstream_url" tags/full >../request
 	) &&
 	<request sed -nf fuzz.sed >request.fuzzy &&
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 883b32efa0..52507b9e50 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -1277,4 +1277,21 @@ 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 one of refs/tags/[AB] exists' '
+	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 &&
+		git -C ../two rev-parse refs/heads/tags/my-tag >actual &&
+		test_cmp expected actual
+	)
+'
+
 test_done
diff --git a/t/t9101-git-svn-props.sh b/t/t9101-git-svn-props.sh
index c26c4b0927..f9e43f4e97 100755
--- a/t/t9101-git-svn-props.sh
+++ b/t/t9101-git-svn-props.sh
@@ -73,11 +73,11 @@ test_expect_success 'fetch revisions from svn' 'git svn fetch'
 
 name='test svn:keywords ignoring'
 test_expect_success "$name" \
-	'git checkout -b mybranch remotes/git-svn &&
+	'git checkout -b mybranch refs/remotes/git-svn &&
 	echo Hi again >> kw.c &&
 	git commit -a -m "test keywords ignoring" &&
-	git svn set-tree remotes/git-svn..mybranch &&
-	git pull . remotes/git-svn'
+	git svn set-tree refs/remotes/git-svn..mybranch &&
+	git pull . refs/remotes/git-svn'
 
 expect='/* $Id$ */'
 got="$(sed -ne 2p kw.c)"
@@ -95,7 +95,7 @@ test_expect_success "propset CR on crlf files" '
 
 test_expect_success 'fetch and pull latest from svn and checkout a new wc' \
 	'git svn fetch &&
-	 git pull . remotes/git-svn &&
+	 git pull . refs/remotes/git-svn &&
 	 svn_cmd co "$svnrepo" new_wc'
 
 for i in crlf ne_crlf lf ne_lf cr ne_cr empty_cr empty_lf empty empty_crlf
@@ -117,7 +117,7 @@ cd test_wc
 	 svn_cmd commit -m "propset CRLF on cr files"'
 cd ..
 test_expect_success 'fetch and pull latest from svn' \
-	'git svn fetch && git pull . remotes/git-svn'
+	'git svn fetch && git pull . refs/remotes/git-svn'
 
 b_cr="$(git hash-object cr)"
 b_ne_cr="$(git hash-object ne_cr)"
@@ -168,7 +168,7 @@ cat >create-ignore-index.expect <<\EOF
 EOF
 
 test_expect_success 'test create-ignore' "
-	git svn fetch && git pull . remotes/git-svn &&
+	git svn fetch && git pull . refs/remotes/git-svn &&
 	git svn create-ignore &&
 	cmp ./.gitignore create-ignore.expect &&
 	cmp ./deeply/.gitignore create-ignore.expect &&
-- 
2.21.0.1020.gf2820cf01a

Re: [RFC/PATCH] refs: tone down the dwimmery in refname_match() for {heads,tags,remotes}/*

[Paolo Bonzini]

On 27/05/19 00:54, Ævar Arnfjörð Bjarmason wrote:
> This resulted in a case[1] where someone on LKML did:
> 
>     git push kvm +HEAD:tags/for-linus
> 
> Which would have created a new "tags/for-linus" branch in their "kvm"
> repository, except because they happened to have an existing
> "refs/tags/for-linus" reference we pushed there instead, and replaced
> an annotated tag with a lightweight tag.

Actually, I would not be surprised even if "git push foo
someref:tags/foo" _always_ created a lightweight tag (i.e. push to
refs/tags/foo).

In my opinion, the bug is that "git request-pull" should warn if the tag
is lightweight remotely but not locally, and possibly even vice versa.
Here is a simple testcase:

  # setup "local" repo
  mkdir -p testdir/a
  cd testdir/a
  git init
  echo a > test
  git add test
  git commit -minitial

  # setup "remote" repo
  git clone --bare . ../b

  # setup "local" tag
  echo b >> test
  git commit -msecond test
  git tag -mtag tag1

  # create remote lightweight tag and prepare a pull request
  git push ../b HEAD:refs/tags/tag1
  git request-pull HEAD^ ../b tags/tag1

Paolo

Re: [RFC/PATCH] refs: tone down the dwimmery in refname_match() for {heads,tags,remotes}/*

[Ævar Arnfjörð Bjarmason]

On Mon, May 27 2019, Paolo Bonzini wrote:

> On 27/05/19 00:54, Ævar Arnfjörð Bjarmason wrote:
>> This resulted in a case[1] where someone on LKML did:
>>
>>     git push kvm +HEAD:tags/for-linus
>>
>> Which would have created a new "tags/for-linus" branch in their "kvm"
>> repository, except because they happened to have an existing
>> "refs/tags/for-linus" reference we pushed there instead, and replaced
>> an annotated tag with a lightweight tag.
>
> Actually, I would not be surprised even if "git push foo
> someref:tags/foo" _always_ created a lightweight tag (i.e. push to
> refs/tags/foo).

That's not the intention (I think), and not what we document.

It mostly (and I believe always should) works by looking at whether
"someref" is a named ref, and e.g. looking at whether it's "master". We
then see that it lives in "refs/heads/master" locally, and thus
correspondingly add a "refs/heads/" to your <dst> "tags/foo", making it
"refs/heads/tags/foo".

*Or* we take e.g. <some random SHA-1>:master, the <some random...> is
ambiguous, but we see that "master" unambiguously refers to
"refs/heads/master" on the remote (so e.g. a refs/tags/master doesn't
exist). If you had both refs/{heads,tags}/master refs on the remote we'd
emit:

    error: dst refspec master matches more than one

(We should improve that error to note what conflicted, #leftoverbits)

So your HEAD:tags/for-linus resulted in pushing a HEAD that referred to
some refs/heads/* to refs/tags/for-linus. I believe that's an unintendedem
ergent effect in how we try to apply these two rules. We should apply
one, not both in combination.

And as an aside none of these rules have to do with whether the <src> is
a lightweight or annotated tag, and both types live in the refs/tags/*
namespace.

> In my opinion, the bug is that "git request-pull" should warn if the tag
> is lightweight remotely but not locally, and possibly even vice versa.
> Here is a simple testcase:
>
>   # setup "local" repo
>   mkdir -p testdir/a
>   cd testdir/a
>   git init
>   echo a > test
>   git add test
>   git commit -minitial
>
>   # setup "remote" repo
>   git clone --bare . ../b
>
>   # setup "local" tag
>   echo b >> test
>   git commit -msecond test
>   git tag -mtag tag1
>
>   # create remote lightweight tag and prepare a pull request
>   git push ../b HEAD:refs/tags/tag1
>   git request-pull HEAD^ ../b tags/tag1

Yeah, maybe. I don't use git-request-pull. So maybe this is a simple
mitigation for that tool since you supply a <remote> to it already.

I was more interested and surprised by HEAD being implicitly resolved to
refs/tags/* in a way that would be *different* than if you didn't have
an existing tag there, but of course if we errored on that you might
have just done "+HEAD:refs/tags/for-linus" and ended up with the same
thing.

As an aside, in *general* tags, unlike branches, don't have "remote
tracking". That's something we'd eventually want, but we're nowhere near
the refstore and porcelain supporting that.

Thus such a check is hard to support in general, we'd always need a
remote name and a network roundtrip. Otherwise we couldn't do anything
sensible if you have 10 remotes of fellow LKML developers, all of whom
have a "for-linus" tag, which I'm assuming is a common use-case.

But since git-request-pull gets the remote it can (and does) check on
that remote, but seems to satisfied to see that the ref exists somewhere
on that remote.

Re: [RFC/PATCH] refs: tone down the dwimmery in refname_match() for {heads,tags,remotes}/*

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> It mostly (and I believe always should) works by looking at whether
> "someref" is a named ref, and e.g. looking at whether it's "master". We
> then see that it lives in "refs/heads/master" locally, and thus
> correspondingly add a "refs/heads/" to your <dst> "tags/foo", making it
> "refs/heads/tags/foo".

Yes.

(I am still not up to speed, so pardon me if I sound nonsense)

> *Or* we take e.g. <some random SHA-1>:master, the <some random...> is
> ambiguous, but we see that "master" unambiguously refers to
> "refs/heads/master" on the remote (so e.g. a refs/tags/master doesn't
> exist). If you had both refs/{heads,tags}/master refs on the remote we'd
> emit:
>
>     error: dst refspec master matches more than one

OK, so you are saying "if the source is unique, try to qualify the
destination to the same hierarchy (i.e. the previous paragraph). If
the source is not a ref (this paragraph), try to find a unique match
with the destination to determine where it should go".  I think that
makes sense.

> (We should improve that error to note what conflicted, #leftoverbits)

OK.

> So your HEAD:tags/for-linus resulted in pushing a HEAD that
> referred to some refs/heads/* to refs/tags/for-linus.  I believe
> that's an unintendedem ergent effect in how we try to apply these
> two rules. We should apply one, not both in combination.

Are you saying that HEAD is locally dereferenced to a branch name
(if you are not detached when pushing), and "if the source is unique
ref" rule is applied first?  That is not how I recall we designed
this dwimmery.  As we know there is no refs/heads/HEAD, it should be
like pushing HEAD^0:tags/for-linus (i.e. it should behave the same
way as pushing "<some random SHA-1>:tags/for-linus"), without "where
is the source?  let's qualify the destination the same way" rule
kicking in.  And because the repeated "Linus, please pull from that
usual tag for this cycle" request is a norm, "does the destination
uniquely exist at the receiving end" should kick in.  IOW, I think
that is quite a deliberate behaviour that is desirable, or atleast
was considered to be desirable when the feature was designed.

>> In my opinion, the bug is that "git request-pull" should warn if the tag
>> is lightweight remotely but not locally, and possibly even vice versa.

Hmm (yes, I realize I am not commenting on what Ævar wrote)...

>>   # create remote lightweight tag and prepare a pull request
>>   git push ../b HEAD:refs/tags/tag1
>>   git request-pull HEAD^ ../b tags/tag1

I do not think lightweight vs annotated should be the issue.  The
tag that the requestor asks to be pulled (from repository ../b)
should be what the requestor has locally when writing the request
(in repository .).  Even if both tags at remote and local are
annotated, we should still warn if they are different objects, no?

Do we run ls-remote or something (or consult remote-trakcing branch)
to see if that is the case in request-pull?
?

Re: [RFC/PATCH] refs: tone down the dwimmery in refname_match() for {heads,tags,remotes}/*

[Paolo Bonzini]

On 27/05/19 17:39, Junio C Hamano wrote:
> I do not think lightweight vs annotated should be the issue.  The
> tag that the requestor asks to be pulled (from repository ../b)
> should be what the requestor has locally when writing the request
> (in repository .).  Even if both tags at remote and local are
> annotated, we should still warn if they are different objects, no?

Right, lightweight vs annotated then is the obvious special case where
one of the two is a commit and the other is a tag, hence they ought not
to have the same SHA1.  I'll take a look.

Paolo

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

[Ævar Arnfjörð Bjarmason]

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

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> 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.

I do not think that is a problematic behaviour in the context of
asking Linus to pull when every time a merge window opens.  One
would keep refs/tags/for-linus at the publishing site, and update it
(forcing as necessary) before request-pull.  If it went to a branch
with confusing name tags/for-linus, that would be a disaster.

> 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'.

"matched" in past tense means that your example scenario actually
has such a confusing branch?  Then I think the above is OK (or just
silently updating the branch is also fine, I think).  If there were
no such branch currently, the above woudl be a serious regression,
but as long as both exist, I think it is probably OK.  From my quick
scan of your new tests, I couldn't quite tell if that case (i.e. the
a tag "my-tag" exists but a bbranch "tags/my-tag"does not exist at
the receiving end when push happens, and the tag is updated without
touching the branch nor giving extra warnings and hints) is covered,
though.

Thanks.