{"thread":{"id":"51179","subject":"Re: [GIT PULL] KVM changes for Linux 5.2-rc2","startedAt":"2019-05-26T20:49:43Z","lastAt":"2019-06-21T16:05:44Z","messageCount":8,"participants":["Linus Torvalds","Ævar Arnfjörð Bjarmason","Paolo Bonzini","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"376230","messageId":"CAHk-=wgzKzAwS=_ySikL1f=Gr62YXL_WXGh82wZKMOvzJ9+2VA@mail.gmail.com","threadId":"51179","inReplyTo":"2d55fd2a-afbf-1b7c-ca82-8bffaa18e0d0@redhat.com","subject":"Re: [GIT PULL] KVM changes for Linux 5.2-rc2","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2019-05-26T20:49:16Z","receivedAt":"2019-05-26T20:49:43Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Sun, May 26, 2019 at 10:53 AM Paolo Bonzini <pbonzini@redhat.com> wrote:\n>\n> The interesting thing is that not only git will treat lightweight tags\n> like, well, tags:\n\nYeah, that's very much by design - lightweight tags are very\ncomvenient for local temporary stuff where you don't want signing etc\n(think automated test infrastructure, or just local reminders).\n\n> In addition, because I _locally_ had a tag object that\n> pointed to the same commit and had the same name, git-request-pull\n> included my local tag's message in its output!  I wonder if this could\n> be considered a bug.\n\nYeah, I think git request-pull should at least *warn* about the tag\nnot being the same object locally as in the remote you're asking me to\npull.\n\nAre you sure you didn't get a warning, and just missed it? But adding\nJunio and the Git list just as a possible heads-up for this in case\ngit request-pull really only compares the object the tag points to,\nrather than the SHA1 of the tag itself.\n\n             Linus\n"},{"id":"376232","messageId":"20190526225445.21618-1-avarab@gmail.com","threadId":"51179","inReplyTo":"CAHk-=wgzKzAwS=_ySikL1f=Gr62YXL_WXGh82wZKMOvzJ9+2VA@mail.gmail.com","subject":"[RFC/PATCH] refs: tone down the dwimmery in refname_match() for {heads,tags,remotes}/*","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-05-26T22:54:45Z","receivedAt":"2019-05-26T22:55:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When a refspec like HEAD:tags/x is pushed where HEAD is a branch,\nwe'll push a *branch* that'll be located at \"refs/heads/tags/x\". This\nis part of the rather straightforward rules I documented in\n2219c09e23 (\"push doc: document the DWYM behavior pushing to\nunqualified <dst>\", 2018-11-13).\n\nHowever, if there exists a refs/tags/x on the remote the\ncount_refspec_match() logic will, as a result of calling\nrefname_match() match the detected branch type of the LHS of the\nrefspec to refs/tags/x, because it's in a loop where it tries to match\n\"tags/x\" to \"refs/tags/X', then \"refs/tags/tags/x\" etc.\n\nThis resulted in a case[1] where someone on LKML did:\n\n    git push kvm +HEAD:tags/for-linus\n\nWhich would have created a new \"tags/for-linus\" branch in their \"kvm\"\nrepository, except because they happened to have an existing\n\"refs/tags/for-linus\" reference we pushed there instead, and replaced\nan annotated tag with a lightweight tag.\n\nLet's tone this down a bit and not match the more general expansions\nif they'd overlap with later expansions.\n\nThis patch is a hack, and should not be applied. We probably want to\nfix this for \"push\", but we use refname_match() all over the place. We\nprobably want to start by undoing part of\n54457fe509 (\"refname_match(): always use the rules in\nref_rev_parse_rules\", 2014-01-14) and having special rules just for\npush.\n\nFurthermore ref_rev_parse_rules is used elsewhere, should we be doing\nthis in other places? I think not if we undo most of 54457fe509 and\ncan just have a custom matcher just for count_refspec_match(). That\none shouldn't need any sort of magic, because elsewhere in the remote\npush DWYM code we try to add implicit refs/{tags,heads}/ prefixes.\n\nAs the t/t5150-request-pull.sh change shows this results in a failing\ntest where a local \"full\" branch is being pushed to a remote\n\"refs/tags/full\". So maybe this is something LKML people actually want\nfor some reason.\n\n1. https://lore.kernel.org/lkml/2d55fd2a-afbf-1b7c-ca82-8bffaa18e0d0@redhat.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nOn Sun, May 26 2019, Linus Torvalds wrote:\n\n> On Sun, May 26, 2019 at 10:53 AM Paolo Bonzini <pbonzini@redhat.com> wrote:\n>>\n>> The interesting thing is that not only git will treat lightweight tags\n>> like, well, tags:\n>\n> Yeah, that's very much by design - lightweight tags are very\n> comvenient for local temporary stuff where you don't want signing etc\n> (think automated test infrastructure, or just local reminders).\n>\n>> In addition, because I _locally_ had a tag object that\n>> pointed to the same commit and had the same name, git-request-pull\n>> included my local tag's message in its output!  I wonder if this could\n>> be considered a bug.\n>\n> Yeah, I think git request-pull should at least *warn* about the tag\n> not being the same object locally as in the remote you're asking me to\n> pull.\n>\n> Are you sure you didn't get a warning, and just missed it? But adding\n> Junio and the Git list just as a possible heads-up for this in case\n> git request-pull really only compares the object the tag points to,\n> rather than the SHA1 of the tag itself.\n\nThis behavior looks like a bug to me. This RFC-quality patch is an\ninitial stab at fixing it, and is all I had time for today.\n\n refs.c                   |  8 +++++++-\n t/t5150-request-pull.sh  |  2 +-\n t/t5505-remote.sh        | 17 +++++++++++++++++\n t/t9101-git-svn-props.sh | 12 ++++++------\n 4 files changed, 31 insertions(+), 8 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 92d1f6dbdd..729b921328 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -514,9 +514,15 @@ int refname_match(const char *abbrev_name, const char *full_name)\n \tconst int abbrev_name_len = strlen(abbrev_name);\n \tconst int num_rules = NUM_REV_PARSE_RULES;\n \n-\tfor (p = ref_rev_parse_rules; *p; p++)\n+\tfor (p = ref_rev_parse_rules; *p; p++) {\n+\t\tif (!strcmp(*p, \"refs/%.*s\") &&\n+\t\t    (starts_with(abbrev_name, \"tags/\") ||\n+\t\t     starts_with(abbrev_name, \"heads/\") ||\n+\t\t     starts_with(abbrev_name, \"remotes/\")))\n+\t\t\tcontinue;\n \t\tif (!strcmp(full_name, mkpath(*p, abbrev_name_len, abbrev_name)))\n \t\t\treturn &ref_rev_parse_rules[num_rules] - p;\n+\t}\n \n \treturn 0;\n }\ndiff --git a/t/t5150-request-pull.sh b/t/t5150-request-pull.sh\nindex fca001eb9b..0265871cf4 100755\n--- a/t/t5150-request-pull.sh\n+++ b/t/t5150-request-pull.sh\n@@ -212,7 +212,7 @@ test_expect_success 'pull request format' '\n \t\tcd local &&\n \t\tgit checkout initial &&\n \t\tgit merge --ff-only master &&\n-\t\tgit push origin tags/full &&\n+\t\tgit push origin full:refs/tags/full &&\n \t\tgit request-pull initial \"$downstream_url\" tags/full >../request\n \t) &&\n \t<request sed -nf fuzz.sed >request.fuzzy &&\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 883b32efa0..52507b9e50 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -1277,4 +1277,21 @@ test_expect_success 'refs/remotes/* <src> refspec and unqualified <dst> DWIM and\n \t)\n '\n \n+test_expect_success 'HEAD:tags/A and HEAD:tags/B should not be different one of refs/tags/[AB] exists' '\n+\tgit clone \"file://$PWD/two\" tags-match &&\n+\t(\n+\t\tcd tags-match &&\n+\t\ttest_commit A &&\n+\t\tgit rev-parse HEAD >expected &&\n+\n+\t\tgit push origin HEAD:tags/my-not-a-tag &&\n+\t\tgit -C ../two rev-parse refs/heads/tags/my-not-a-tag >actual &&\n+\t\ttest_cmp expected actual &&\n+\n+\t\tgit push origin HEAD:tags/my-tag &&\n+\t\tgit -C ../two rev-parse refs/heads/tags/my-tag >actual &&\n+\t\ttest_cmp expected actual\n+\t)\n+'\n+\n test_done\ndiff --git a/t/t9101-git-svn-props.sh b/t/t9101-git-svn-props.sh\nindex c26c4b0927..f9e43f4e97 100755\n--- a/t/t9101-git-svn-props.sh\n+++ b/t/t9101-git-svn-props.sh\n@@ -73,11 +73,11 @@ test_expect_success 'fetch revisions from svn' 'git svn fetch'\n \n name='test svn:keywords ignoring'\n test_expect_success \"$name\" \\\n-\t'git checkout -b mybranch remotes/git-svn &&\n+\t'git checkout -b mybranch refs/remotes/git-svn &&\n \techo Hi again >> kw.c &&\n \tgit commit -a -m \"test keywords ignoring\" &&\n-\tgit svn set-tree remotes/git-svn..mybranch &&\n-\tgit pull . remotes/git-svn'\n+\tgit svn set-tree refs/remotes/git-svn..mybranch &&\n+\tgit pull . refs/remotes/git-svn'\n \n expect='/* $Id$ */'\n got=\"$(sed -ne 2p kw.c)\"\n@@ -95,7 +95,7 @@ test_expect_success \"propset CR on crlf files\" '\n \n test_expect_success 'fetch and pull latest from svn and checkout a new wc' \\\n \t'git svn fetch &&\n-\t git pull . remotes/git-svn &&\n+\t git pull . refs/remotes/git-svn &&\n \t svn_cmd co \"$svnrepo\" new_wc'\n \n for i in crlf ne_crlf lf ne_lf cr ne_cr empty_cr empty_lf empty empty_crlf\n@@ -117,7 +117,7 @@ cd test_wc\n \t svn_cmd commit -m \"propset CRLF on cr files\"'\n cd ..\n test_expect_success 'fetch and pull latest from svn' \\\n-\t'git svn fetch && git pull . remotes/git-svn'\n+\t'git svn fetch && git pull . refs/remotes/git-svn'\n \n b_cr=\"$(git hash-object cr)\"\n b_ne_cr=\"$(git hash-object ne_cr)\"\n@@ -168,7 +168,7 @@ cat >create-ignore-index.expect <<\\EOF\n EOF\n \n test_expect_success 'test create-ignore' \"\n-\tgit svn fetch && git pull . remotes/git-svn &&\n+\tgit svn fetch && git pull . refs/remotes/git-svn &&\n \tgit svn create-ignore &&\n \tcmp ./.gitignore create-ignore.expect &&\n \tcmp ./deeply/.gitignore create-ignore.expect &&\n-- \n2.21.0.1020.gf2820cf01a\n\n"},{"id":"376237","messageId":"5c9ce55c-2c3a-fce0-d6e3-dfe5f8fc9b01@redhat.com","threadId":"51179","inReplyTo":"20190526225445.21618-1-avarab@gmail.com","subject":"Re: [RFC/PATCH] refs: tone down the dwimmery in refname_match() for {heads,tags,remotes}/*","fromName":"Paolo Bonzini","fromEmail":"pbonzini@redhat.com","sentAt":"2019-05-27T12:33:10Z","receivedAt":"2019-05-27T12:33:19Z","isPatch":true,"sender":{"key":"pbonzini@redhat.com","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"On 27/05/19 00:54, Ævar Arnfjörð Bjarmason wrote:\n> This resulted in a case[1] where someone on LKML did:\n> \n>     git push kvm +HEAD:tags/for-linus\n> \n> Which would have created a new \"tags/for-linus\" branch in their \"kvm\"\n> repository, except because they happened to have an existing\n> \"refs/tags/for-linus\" reference we pushed there instead, and replaced\n> an annotated tag with a lightweight tag.\n\nActually, I would not be surprised even if \"git push foo\nsomeref:tags/foo\" _always_ created a lightweight tag (i.e. push to\nrefs/tags/foo).\n\nIn my opinion, the bug is that \"git request-pull\" should warn if the tag\nis lightweight remotely but not locally, and possibly even vice versa.\nHere is a simple testcase:\n\n  # setup \"local\" repo\n  mkdir -p testdir/a\n  cd testdir/a\n  git init\n  echo a > test\n  git add test\n  git commit -minitial\n\n  # setup \"remote\" repo\n  git clone --bare . ../b\n\n  # setup \"local\" tag\n  echo b >> test\n  git commit -msecond test\n  git tag -mtag tag1\n\n  # create remote lightweight tag and prepare a pull request\n  git push ../b HEAD:refs/tags/tag1\n  git request-pull HEAD^ ../b tags/tag1\n\nPaolo\n"},{"id":"376238","messageId":"874l5gezsn.fsf@evledraar.gmail.com","threadId":"51179","inReplyTo":"5c9ce55c-2c3a-fce0-d6e3-dfe5f8fc9b01@redhat.com","subject":"Re: [RFC/PATCH] refs: tone down the dwimmery in refname_match() for {heads,tags,remotes}/*","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-05-27T14:29:28Z","receivedAt":"2019-05-27T14:29:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, May 27 2019, Paolo Bonzini wrote:\n\n> On 27/05/19 00:54, Ævar Arnfjörð Bjarmason wrote:\n>> This resulted in a case[1] where someone on LKML did:\n>>\n>>     git push kvm +HEAD:tags/for-linus\n>>\n>> Which would have created a new \"tags/for-linus\" branch in their \"kvm\"\n>> repository, except because they happened to have an existing\n>> \"refs/tags/for-linus\" reference we pushed there instead, and replaced\n>> an annotated tag with a lightweight tag.\n>\n> Actually, I would not be surprised even if \"git push foo\n> someref:tags/foo\" _always_ created a lightweight tag (i.e. push to\n> refs/tags/foo).\n\nThat's not the intention (I think), and not what we document.\n\nIt mostly (and I believe always should) works by looking at whether\n\"someref\" is a named ref, and e.g. looking at whether it's \"master\". We\nthen see that it lives in \"refs/heads/master\" locally, and thus\ncorrespondingly add a \"refs/heads/\" to your <dst> \"tags/foo\", making it\n\"refs/heads/tags/foo\".\n\n*Or* we take e.g. <some random SHA-1>:master, the <some random...> is\nambiguous, but we see that \"master\" unambiguously refers to\n\"refs/heads/master\" on the remote (so e.g. a refs/tags/master doesn't\nexist). If you had both refs/{heads,tags}/master refs on the remote we'd\nemit:\n\n    error: dst refspec master matches more than one\n\n(We should improve that error to note what conflicted, #leftoverbits)\n\nSo your HEAD:tags/for-linus resulted in pushing a HEAD that referred to\nsome refs/heads/* to refs/tags/for-linus. I believe that's an unintendedem\nergent effect in how we try to apply these two rules. We should apply\none, not both in combination.\n\nAnd as an aside none of these rules have to do with whether the <src> is\na lightweight or annotated tag, and both types live in the refs/tags/*\nnamespace.\n\n> In my opinion, the bug is that \"git request-pull\" should warn if the tag\n> is lightweight remotely but not locally, and possibly even vice versa.\n> Here is a simple testcase:\n>\n>   # setup \"local\" repo\n>   mkdir -p testdir/a\n>   cd testdir/a\n>   git init\n>   echo a > test\n>   git add test\n>   git commit -minitial\n>\n>   # setup \"remote\" repo\n>   git clone --bare . ../b\n>\n>   # setup \"local\" tag\n>   echo b >> test\n>   git commit -msecond test\n>   git tag -mtag tag1\n>\n>   # create remote lightweight tag and prepare a pull request\n>   git push ../b HEAD:refs/tags/tag1\n>   git request-pull HEAD^ ../b tags/tag1\n\nYeah, maybe. I don't use git-request-pull. So maybe this is a simple\nmitigation for that tool since you supply a <remote> to it already.\n\nI was more interested and surprised by HEAD being implicitly resolved to\nrefs/tags/* in a way that would be *different* than if you didn't have\nan existing tag there, but of course if we errored on that you might\nhave just done \"+HEAD:refs/tags/for-linus\" and ended up with the same\nthing.\n\nAs an aside, in *general* tags, unlike branches, don't have \"remote\ntracking\". That's something we'd eventually want, but we're nowhere near\nthe refstore and porcelain supporting that.\n\nThus such a check is hard to support in general, we'd always need a\nremote name and a network roundtrip. Otherwise we couldn't do anything\nsensible if you have 10 remotes of fellow LKML developers, all of whom\nhave a \"for-linus\" tag, which I'm assuming is a common use-case.\n\nBut since git-request-pull gets the remote it can (and does) check on\nthat remote, but seems to satisfied to see that the ref exists somewhere\non that remote.\n"},{"id":"376240","messageId":"xmqqef4jewj6.fsf@gitster-ct.c.googlers.com","threadId":"51179","inReplyTo":"874l5gezsn.fsf@evledraar.gmail.com","subject":"Re: [RFC/PATCH] refs: tone down the dwimmery in refname_match() for {heads,tags,remotes}/*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-27T15:39:57Z","receivedAt":"2019-05-27T15:40:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> It mostly (and I believe always should) works by looking at whether\n> \"someref\" is a named ref, and e.g. looking at whether it's \"master\". We\n> then see that it lives in \"refs/heads/master\" locally, and thus\n> correspondingly add a \"refs/heads/\" to your <dst> \"tags/foo\", making it\n> \"refs/heads/tags/foo\".\n\nYes.\n\n(I am still not up to speed, so pardon me if I sound nonsense)\n\n> *Or* we take e.g. <some random SHA-1>:master, the <some random...> is\n> ambiguous, but we see that \"master\" unambiguously refers to\n> \"refs/heads/master\" on the remote (so e.g. a refs/tags/master doesn't\n> exist). If you had both refs/{heads,tags}/master refs on the remote we'd\n> emit:\n>\n>     error: dst refspec master matches more than one\n\nOK, so you are saying \"if the source is unique, try to qualify the\ndestination to the same hierarchy (i.e. the previous paragraph). If\nthe source is not a ref (this paragraph), try to find a unique match\nwith the destination to determine where it should go\".  I think that\nmakes sense.\n\n> (We should improve that error to note what conflicted, #leftoverbits)\n\nOK.\n\n> So your HEAD:tags/for-linus resulted in pushing a HEAD that\n> referred to some refs/heads/* to refs/tags/for-linus.  I believe\n> that's an unintendedem ergent effect in how we try to apply these\n> two rules. We should apply one, not both in combination.\n\nAre you saying that HEAD is locally dereferenced to a branch name\n(if you are not detached when pushing), and \"if the source is unique\nref\" rule is applied first?  That is not how I recall we designed\nthis dwimmery.  As we know there is no refs/heads/HEAD, it should be\nlike pushing HEAD^0:tags/for-linus (i.e. it should behave the same\nway as pushing \"<some random SHA-1>:tags/for-linus\"), without \"where\nis the source?  let's qualify the destination the same way\" rule\nkicking in.  And because the repeated \"Linus, please pull from that\nusual tag for this cycle\" request is a norm, \"does the destination\nuniquely exist at the receiving end\" should kick in.  IOW, I think\nthat is quite a deliberate behaviour that is desirable, or atleast\nwas considered to be desirable when the feature was designed.\n\n>> In my opinion, the bug is that \"git request-pull\" should warn if the tag\n>> is lightweight remotely but not locally, and possibly even vice versa.\n\nHmm (yes, I realize I am not commenting on what Ævar wrote)...\n\n>>   # create remote lightweight tag and prepare a pull request\n>>   git push ../b HEAD:refs/tags/tag1\n>>   git request-pull HEAD^ ../b tags/tag1\n\nI do not think lightweight vs annotated should be the issue.  The\ntag that the requestor asks to be pulled (from repository ../b)\nshould be what the requestor has locally when writing the request\n(in repository .).  Even if both tags at remote and local are\nannotated, we should still warn if they are different objects, no?\n\nDo we run ls-remote or something (or consult remote-trakcing branch)\nto see if that is the case in request-pull?\n?\n"},{"id":"376241","messageId":"a40c5388-4274-6bfa-4213-6013601c8fae@redhat.com","threadId":"51179","inReplyTo":"xmqqef4jewj6.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC/PATCH] refs: tone down the dwimmery in refname_match() for {heads,tags,remotes}/*","fromName":"Paolo Bonzini","fromEmail":"pbonzini@redhat.com","sentAt":"2019-05-27T15:44:07Z","receivedAt":"2019-05-27T15:44:14Z","isPatch":true,"sender":{"key":"pbonzini@redhat.com","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"On 27/05/19 17:39, Junio C Hamano wrote:\n> I do not think lightweight vs annotated should be the issue.  The\n> tag that the requestor asks to be pulled (from repository ../b)\n> should be what the requestor has locally when writing the request\n> (in repository .).  Even if both tags at remote and local are\n> annotated, we should still warn if they are different objects, no?\n\nRight, lightweight vs annotated then is the obvious special case where\none of the two is a commit and the other is a tag, hence they ought not\nto have the same SHA1.  I'll take a look.\n\nPaolo\n"},{"id":"377746","messageId":"20190621144447.21769-1-avarab@gmail.com","threadId":"51179","inReplyTo":"20190526225445.21618-1-avarab@gmail.com","subject":"[PATCH] push: make \"HEAD:tags/my-tag\" consistently push to a branch","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-06-21T14:44:47Z","receivedAt":"2019-06-21T14:45:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When a refspec like \"HEAD:tags/my-tag\" is pushed where \"HEAD\" is a\nbranch, we'll push a *branch* that'll be located at\n\"refs/heads/tags/my-tag\". This is part of the rather straightforward\nrules I documented in 2219c09e23 (\"push doc: document the DWYM\nbehavior pushing to unqualified <dst>\", 2018-11-13).\n\nHowever, if there exists a \"refs/tags/my-tag\" on the remote the\ncount_refspec_match() logic will, as a result of calling\nrefname_match(), match partially-qualified RHS of the refspec\n\"refs/tags/my-tag\", because it's in a loop where it tries to match\n\"tags/my-tag\" to \"refs/tags/my-tag', then \"refs/tags/tags/my-tag\" etc.\n\nThis resulted in a case[1] where someone on LKML did:\n\n    git push kvm +HEAD:tags/for-linus\n\nWhich would have created a new \"refs/heads/tags/for-linus\" branch in\ntheir \"kvm\" repository. But since they happened to have an existing\n\"refs/tags/for-linus\" reference we pushed there instead, and replaced\nan annotated tag with a lightweight tag.\n\nWe do want a RHS ref like \"master\" to match \"refs/heads/master\", but\nit's confusing and dangerous that the DWYM behavior for matching\npartial RHS refspecs acts differently when the start of the RHS\nhappens to be a second-level namespace under \"refs/\" namespace like\n\"tags\".\n\nNow we'll print out the following advice when this happens, and act\ndifferently as described therein:\n\n    hint: The <dst> part of the refspec matched both of:\n    hint:\n    hint:   1. tags/my-tag -> refs/tags/my-tag\n    hint:   2. tags/my-tag -> refs/heads/tags/my-tag\n    hint:\n    hint: Earlier versions of git would have picked (1) as the RHS starts\n    hint: with a second-level ref prefix which could be fully-qualified by\n    hint: adding 'refs/' in front of it. We now pick (2) which uses the prefix\n    hint: inferred from the <src> part of the refspec.\n    hint:\n    hint: See the \"<refspec>...\" rules  discussed in 'git help push'.\n\nAn earlier version of this patch[2] used the much more heavy-handed\napproach of changing this logic in refname_match(). As shown from the\ntests that patch needed to modify that results in changes that are\noverzealous for fixing this push-specific issue.\n\nThe right place to fix this is in match_explicit(). There we can see\nif we have both a DWYM match and a match based on the prefix of the\nLHS of the refspec, in those cases the match based on the LHS's ref\nprefix should win.\n\n1. https://lore.kernel.org/lkml/2d55fd2a-afbf-1b7c-ca82-8bffaa18e0d0@redhat.com/\n2. https://public-inbox.org/git/20190526225445.21618-1-avarab@gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nNow that the 2.22.0 release is out I cleaned this up into a more\nsensible patch.\n\n Documentation/config/advice.txt |  7 +++++++\n Documentation/git-push.txt      | 13 +++++++++++++\n advice.c                        |  2 ++\n advice.h                        |  1 +\n remote.c                        | 23 ++++++++++++++++++++++-\n t/t5505-remote.sh               | 18 ++++++++++++++++++\n 6 files changed, 63 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt\nindex ec4f6ae658..36cb3db63a 100644\n--- a/Documentation/config/advice.txt\n+++ b/Documentation/config/advice.txt\n@@ -37,6 +37,13 @@ advice.*::\n \t\twe can still suggest that the user push to either\n \t\trefs/heads/* or refs/tags/* based on the type of the\n \t\tsource object.\n+\tpushPartialAmbigiousName::\n+\t\tShown when linkgit:git-push[1] is given a refspec\n+\t\twhere the <src> in earlier versions of Git would have\n+\t\tmatched a <dst> on the remote based on its existence\n+\t\tover appending a prefix based on the type of the\n+\t\t<src>. See the \"<refspec>...\" documentation in\n+\t\tlinkgit:git-push[1] for details.\n \tstatusHints::\n \t\tShow directions on how to proceed from the current\n \t\tstate in the output of linkgit:git-status[1], in\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex 6a8a0d958b..5c46ef5e59 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -84,6 +84,19 @@ is ambiguous.\n \n * If <src> resolves to a ref starting with refs/heads/ or refs/tags/,\n   then prepend that to <dst>.\n++\n+Versions of Git before 2.23.0 would override this rule and match\n+e.g. `HEAD:tags/mark` to either `refs/tags/mark` or `refs/tags/mark`\n+depending on, respectively, if `refs/tags/mark` existed or not on the\n+remote.\n++\n+We'll now consistently pick `refs/heads/tags/mark` based on this rule\n+and so that we're not so eager in guessing the <dst> on the remote\n+that we'll pick a different <dst> based on what refs exist there\n+already than we otherwise would have. This exception guards for cases\n+where the match would be different due to a subsequent\n+\"refs/{tags,heads,remotes}/\" matching rule\". than a plain \"refs/\"\n+prefix match.\n \n * Other ambiguity resolutions might be added in the future, but for\n   now any other cases will error out with an error indicating what we\ndiff --git a/advice.c b/advice.c\nindex ce5f374ecd..c9217834e2 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -10,6 +10,7 @@ int advice_push_already_exists = 1;\n int advice_push_fetch_first = 1;\n int advice_push_needs_force = 1;\n int advice_push_unqualified_ref_name = 1;\n+int advice_partial_ambiguous_ref_name = 1;\n int advice_status_hints = 1;\n int advice_status_u_option = 1;\n int advice_commit_before_merge = 1;\n@@ -66,6 +67,7 @@ static struct {\n \t{ \"pushFetchFirst\", &advice_push_fetch_first },\n \t{ \"pushNeedsForce\", &advice_push_needs_force },\n \t{ \"pushUnqualifiedRefName\", &advice_push_unqualified_ref_name },\n+\t{ \"pushPartialAmbigiousName\", &advice_partial_ambiguous_ref_name },\n \t{ \"statusHints\", &advice_status_hints },\n \t{ \"statusUoption\", &advice_status_u_option },\n \t{ \"commitBeforeMerge\", &advice_commit_before_merge },\ndiff --git a/advice.h b/advice.h\nindex e50f02cdfe..027ec396cf 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -10,6 +10,7 @@ extern int advice_push_already_exists;\n extern int advice_push_fetch_first;\n extern int advice_push_needs_force;\n extern int advice_push_unqualified_ref_name;\n+extern int advice_partial_ambiguous_ref_name;\n extern int advice_status_hints;\n extern int advice_status_u_option;\n extern int advice_commit_before_merge;\ndiff --git a/remote.c b/remote.c\nindex e50f7602ed..be226fac11 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1066,7 +1066,7 @@ static int match_explicit(struct ref *src, struct ref *dst,\n \t\t\t  struct ref ***dst_tail,\n \t\t\t  struct refspec_item *rs)\n {\n-\tstruct ref *matched_src, *matched_dst;\n+\tstruct ref *matched_src, *matched_dst, *tmp;\n \tint allocated_src;\n \n \tconst char *dst_value = rs->dst;\n@@ -1094,6 +1094,27 @@ static int match_explicit(struct ref *src, struct ref *dst,\n \n \tswitch (count_refspec_match(dst_value, dst, &matched_dst)) {\n \tcase 1:\n+\t\tif ((starts_with(dst_value, \"tags/\") ||\n+\t\t     starts_with(dst_value, \"heads/\") ||\n+\t\t     starts_with(dst_value, \"remotes/\")) &&\n+\t\t    (dst_guess = guess_ref(dst_value, matched_src))) {\n+\t\t\ttmp = make_linked_ref(dst_guess, dst_tail);\n+\t\t\tif (advice_partial_ambiguous_ref_name)\n+\t\t\t\tadvise(_(\"The <dst> part of the refspec matched both of:\\n\"\n+\t\t\t\t\t \"\\n\"\n+\t\t\t\t\t \"\t1. %1$s -> %2$s\\n\"\n+\t\t\t\t\t \"\t2. %1$s -> %3$s\\n\"\n+\t\t\t\t\t \"\\n\"\n+\t\t\t\t\t \"Earlier versions of git would have picked (1) as the RHS starts\\n\"\n+\t\t\t\t\t \"with a second-level ref prefix which could be fully-qualified by\\n\"\n+\t\t\t\t\t \"adding 'refs/' in front of it. We now pick (2) which uses the prefix\\n\"\n+\t\t\t\t\t \"inferred from the <src> part of the refspec.\\n\"\n+\t\t\t\t\t \"\\n\"\n+\t\t\t\t\t \"See the \\\"<refspec>...\\\" rules  discussed in 'git help push'.\\n\"),\n+\t\t\t\t\tdst_value, matched_dst->name, tmp->name);\n+\t\t\tmatched_dst = tmp;\n+\t\t}\n+\n \t\tbreak;\n \tcase 0:\n \t\tif (starts_with(dst_value, \"refs/\")) {\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 883b32efa0..4d54f90ae3 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -1277,4 +1277,22 @@ test_expect_success 'refs/remotes/* <src> refspec and unqualified <dst> DWIM and\n \t)\n '\n \n+test_expect_success 'HEAD:tags/A and HEAD:tags/B should not be different depending on if refs/tags/[AB] exists or not' '\n+\tgit clone \"file://$PWD/two\" tags-match &&\n+\t(\n+\t\tcd tags-match &&\n+\t\ttest_commit A &&\n+\t\tgit rev-parse HEAD >expected &&\n+\n+\t\tgit push origin HEAD:tags/my-not-a-tag &&\n+\t\tgit -C ../two rev-parse refs/heads/tags/my-not-a-tag >actual &&\n+\t\ttest_cmp expected actual &&\n+\n+\t\tgit push origin HEAD:tags/my-tag 2>advice &&\n+\t\ttest_i18ngrep \"hint: The <dst> part of the refspec matched both of\" advice &&\n+\t\tgit -C ../two rev-parse refs/heads/tags/my-tag >actual &&\n+\t\ttest_cmp expected actual\n+\t)\n+'\n+\n test_done\n-- \n2.22.0.455.g172b71a6c5\n\n"},{"id":"377751","messageId":"xmqq4l4ily9l.fsf@gitster-ct.c.googlers.com","threadId":"51179","inReplyTo":"20190621144447.21769-1-avarab@gmail.com","subject":"Re: [PATCH] push: make \"HEAD:tags/my-tag\" consistently push to a branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-21T16:05:26Z","receivedAt":"2019-06-21T16:05:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> This resulted in a case[1] where someone on LKML did:\n>\n>     git push kvm +HEAD:tags/for-linus\n>\n> Which would have created a new \"refs/heads/tags/for-linus\" branch in\n> their \"kvm\" repository. But since they happened to have an existing\n> \"refs/tags/for-linus\" reference we pushed there instead, and replaced\n> an annotated tag with a lightweight tag.\n\nI do not think that is a problematic behaviour in the context of\nasking Linus to pull when every time a merge window opens.  One\nwould keep refs/tags/for-linus at the publishing site, and update it\n(forcing as necessary) before request-pull.  If it went to a branch\nwith confusing name tags/for-linus, that would be a disaster.\n\n> Now we'll print out the following advice when this happens, and act\n> differently as described therein:\n>\n>     hint: The <dst> part of the refspec matched both of:\n>     hint:\n>     hint:   1. tags/my-tag -> refs/tags/my-tag\n>     hint:   2. tags/my-tag -> refs/heads/tags/my-tag\n>     hint:\n>     hint: Earlier versions of git would have picked (1) as the RHS starts\n>     hint: with a second-level ref prefix which could be fully-qualified by\n>     hint: adding 'refs/' in front of it. We now pick (2) which uses the prefix\n>     hint: inferred from the <src> part of the refspec.\n>     hint:\n>     hint: See the \"<refspec>...\" rules  discussed in 'git help push'.\n\n\"matched\" in past tense means that your example scenario actually\nhas such a confusing branch?  Then I think the above is OK (or just\nsilently updating the branch is also fine, I think).  If there were\nno such branch currently, the above woudl be a serious regression,\nbut as long as both exist, I think it is probably OK.  From my quick\nscan of your new tests, I couldn't quite tell if that case (i.e. the\na tag \"my-tag\" exists but a bbranch \"tags/my-tag\"does not exist at\nthe receiving end when push happens, and the tag is updated without\ntouching the branch nor giving extra warnings and hints) is covered,\nthough.\n\nThanks.\n\n"}]}