mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <>
Cc: "Linus Torvalds" <>,
	"Junio C Hamano" <>,
	"Paolo Bonzini" <>,
	"Linux List Kernel Mailing" <>,
	"Radim Krčmář" <>,
	"KVM list" <>,
	"Michael Haggerty" <>,
	"Ævar Arnfjörð Bjarmason" <>
Subject: [RFC/PATCH] refs: tone down the dwimmery in refname_match() for {heads,tags,remotes}/*
Date: Mon, 27 May 2019 00:54:45 +0200	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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

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


Signed-off-by: Ævar Arnfjörð Bjarmason <>

On Sun, May 26 2019, Linus Torvalds wrote:

> On Sun, May 26, 2019 at 10:53 AM Paolo Bonzini <> 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/  |  2 +-
 t/        | 17 +++++++++++++++++
 t/ | 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/ b/t/
index fca001eb9b..0265871cf4 100755
--- a/t/
+++ b/t/
@@ -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/ b/t/
index 883b32efa0..52507b9e50 100755
--- a/t/
+++ b/t/
@@ -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
+	)
diff --git a/t/ b/t/
index c26c4b0927..f9e43f4e97 100755
--- a/t/
+++ b/t/
@@ -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
 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 &&

  reply	other threads:[~2019-05-26 22:55 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <>
     [not found] ` <>
     [not found]   ` <>
2019-05-26 20:49     ` [GIT PULL] KVM changes for Linux 5.2-rc2 Linus Torvalds
2019-05-26 22:54       ` Ævar Arnfjörð Bjarmason [this message]
2019-05-27 12:33         ` [RFC/PATCH] refs: tone down the dwimmery in refname_match() for {heads,tags,remotes}/* Paolo Bonzini
2019-05-27 14:29           ` Ævar Arnfjörð Bjarmason
2019-05-27 15:39             ` Junio C Hamano
2019-05-27 15:44               ` Paolo Bonzini
2019-06-21 14:44         ` [PATCH] push: make "HEAD:tags/my-tag" consistently push to a branch Ævar Arnfjörð Bjarmason
2019-06-21 16:05           ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).