{"thread":{"id":"35713","subject":"[RFC PATCH] Make 'git request-pull' more strict about matching local/remote branches","startedAt":"2014-01-22T21:08:54Z","lastAt":"2014-01-22T22:27:03Z","messageCount":7,"participants":["Linus Torvalds","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"233545","messageId":"alpine.LFD.2.11.1401221243090.18459@i7.linux-foundation.org","threadId":"35713","inReplyTo":null,"subject":"[RFC PATCH] Make 'git request-pull' more strict about matching local/remote branches","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2014-01-22T21:08:54Z","receivedAt":"2014-01-22T21:08:54Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nFrom: Linus Torvalds <torvalds@linux-foundation.org>\nDate: Wed, 22 Jan 2014 12:32:30 -0800\nSubject: [PATCH] Make 'git request-pull' more strict about matching\n local/remote branches\n\nThe current 'request-pull' will try to find matching commit on the given \nremote, and rewrite the \"please pull\" line to match that remote ref.\n\nThat may be very helpful if your local tree doesn't match the layout of \nthe remote branches, but for the common case it's been a recurring \ndisaster, when \"request-pull\" is done against a delayed remote update, and \nit rewrites the target branch randomly to some other branch name that \nhappens to have the same expected SHA1 (or more commonly, leaves it \nblank).\n\nTo avoid that recurring problem, this changes \"git request-pull\" so that \nit matches the ref name to be pulled against the *local* repository, and \nthen warns if the remote repository does not have that exact same branch \nor tag name and content.\n\nThis means that git request-pull will never rewrite the ref-name you gave \nit.  If the local branch name is \"xyzzy\", that is the only branch name \nthat request-pull will ask the other side to fetch.\n\nIf the remote has that branch under a different name, that's your problem \nand git request-pull will not try to fix it up (but git request-pull will \nwarn about the fact that no exact matching branch is found, and you can \nedit the end result to then have the remote name you want if it doesn't \nmatch your local one).\n\nThe new \"find local ref\" code will also complain loudly if you give an\nambiguous refname (eg you have both a tag and a branch with that same\nname, and you don't specify \"heads/name\" or \"tags/name\").\n\nSigned-off-by: Linus Torvalds <torvalds@linux-foundation.org>\n---\n\nThis should fix the problem we've had multiple times with kernel \nmaintainers, where \"git request-pull\" ends up leaving the target branch \nname blank, because people either forgot to push it, or (more commonly) \npeople pushed it just before doing the pull request, and it hadn't \nactually had time to mirror out to the public site.\n\nNow, git request-pull will *warn* about the fact that the matching ref \nisn't found on the remote (and the new matching code is stricter at that), \nbut it will never try to re-write the branch name that it asks the other \nend to pull.\n\nSo if the remote branch doesn't exist, you'll get a warning, but the pull \nrequest will still have the branch you specified.\n\nThe whole checking thing is both simplified (removing more lines than it \nadds) and made more strict.\n\nComments? It passes the tests I put it through locally, but I did *not* \nmake it pass the test-suite, since it very much does change the rules. \nSome of the test suite code literally tests for the old completely broken \ncase (at least t5150, subtests 4 and 5).\n\nThus the RFC part. Because the currect git request-pull behavior has been \nhorrible.\n\n git-request-pull.sh | 110 ++++++++++++++++++++--------------------------------\n 1 file changed, 43 insertions(+), 67 deletions(-)\n\ndiff --git a/git-request-pull.sh b/git-request-pull.sh\nindex fe21d5db631c..659a412155d8 100755\n--- a/git-request-pull.sh\n+++ b/git-request-pull.sh\n@@ -35,20 +35,7 @@ do\n \tshift\n done\n \n-base=$1 url=$2 head=${3-HEAD} status=0 branch_name=\n-\n-headref=$(git symbolic-ref -q \"$head\")\n-if git show-ref -q --verify \"$headref\"\n-then\n-\tbranch_name=${headref#refs/heads/}\n-\tif test \"z$branch_name\" = \"z$headref\" ||\n-\t\t! git config \"branch.$branch_name.description\" >/dev/null\n-\tthen\n-\t\tbranch_name=\n-\tfi\n-fi\n-\n-tag_name=$(git describe --exact \"$head^0\" 2>/dev/null)\n+base=$1 url=$2 status=0\n \n test -n \"$base\" && test -n \"$url\" || usage\n \n@@ -58,55 +45,68 @@ then\n     die \"fatal: Not a valid revision: $base\"\n fi\n \n+#\n+# $3 must be a symbolic ref, a unique ref, or\n+# a SHA object expression\n+#\n+head=$(git symbolic-ref -q \"${3-HEAD}\")\n+head=${head:-$(git show-ref \"${3-HEAD}\" | cut -d' ' -f2)}\n+head=${head:-$(git rev-parse --quiet --verify \"$3\")}\n+\n+# None of the above? Bad.\n+test -z \"$head\" && die \"fatal: Not a valid revision: $3\"\n+\n+# This also verifies that the resulting head is unique:\n+# \"git show-ref\" could have shown multiple matching refs..\n headrev=$(git rev-parse --verify --quiet \"$head\"^0)\n-if test -z \"$headrev\"\n+test -z \"$headrev\" && die \"fatal: Ambiguous revision: $3\"\n+\n+# Was it a branch with a description?\n+branch_name=${head#refs/heads/}\n+if test \"z$branch_name\" = \"z$headref\" ||\n+\t! git config \"branch.$branch_name.description\" >/dev/null\n then\n-    die \"fatal: Not a valid revision: $head\"\n+\tbranch_name=\n fi\n \n+prettyhead=${head#refs/}\n+prettyhead=${prettyhead#heads/}\n+\n merge_base=$(git merge-base $baserev $headrev) ||\n die \"fatal: No commits in common between $base and $head\"\n \n-# $head is the token given from the command line, and $tag_name, if\n-# exists, is the tag we are going to show the commit information for.\n-# If that tag exists at the remote and it points at the commit, use it.\n-# Otherwise, if a branch with the same name as $head exists at the remote\n-# and their values match, use that instead.\n+# $head is the refname from the command line.\n+# If a ref with the same name as $head exists at the remote\n+# and their values match, use that.\n #\n # Otherwise find a random ref that matches $headrev.\n find_matching_ref='\n-\tsub abbr {\n-\t\tmy $ref = shift;\n-\t\tif ($ref =~ s|^refs/heads/|| || $ref =~ s|^refs/tags/|tags/|) {\n-\t\t\treturn $ref;\n-\t\t} else {\n-\t\t\treturn $ref;\n-\t\t}\n-\t}\n-\n-\tmy ($tagged, $branch, $found);\n+\tmy ($exact,$found);\n \twhile (<STDIN>) {\n-\t\tmy ($sha1, $ref, $deref) = /^(\\S+)\\s+(\\S+?)(\\^\\{\\})?$/;\n+\t\tmy ($sha1, $ref, $deref) = /^(\\S+)\\s+([^^]+)(\\S*)$/;\n \t\tnext unless ($sha1 eq $ARGV[1]);\n-\t\t$found = abbr($ref);\n-\t\tif ($deref && $ref eq \"tags/$ARGV[2]\") {\n-\t\t\t$tagged = $found;\n-\t\t\tlast;\n+\t\tif ($ref eq $ARGV[0]) {\n+\t\t\t$exact = $ref;\n \t\t}\n-\t\tif ($ref =~ m|/\\Q$ARGV[0]\\E$|) {\n-\t\t\t$exact = $found;\n+\t\tif ($sha1 eq $ARGV[0]) {\n+\t\t\t$found = $sha1;\n \t\t}\n \t}\n-\tif ($tagged) {\n-\t\tprint \"$tagged\\n\";\n-\t} elsif ($exact) {\n+\tif ($exact) {\n \t\tprint \"$exact\\n\";\n \t} elsif ($found) {\n \t\tprint \"$found\\n\";\n \t}\n '\n \n-ref=$(git ls-remote \"$url\" | @@PERL@@ -e \"$find_matching_ref\" \"$head\" \"$headrev\" \"$tag_name\")\n+ref=$(git ls-remote \"$url\" | @@PERL@@ -e \"$find_matching_ref\" \"$head\" \"$headrev\")\n+\n+if test -z \"$ref\"\n+then\n+\techo \"warn: No match for $prettyhead found at $url\" >&2\n+\techo \"warn: Are you sure you pushed '$prettyhead' there?\" >&2\n+\tstatus=1\n+fi\n \n url=$(git ls-remote --get-url \"$url\")\n \n@@ -116,7 +116,7 @@ git show -s --format='The following changes since commit %H:\n \n are available in the git repository at:\n ' $merge_base &&\n-echo \"  $url${ref+ $ref}\" &&\n+echo \"  $url $prettyhead\" &&\n git show -s --format='\n for you to fetch changes up to %H:\n \n@@ -129,34 +129,10 @@ then\n \techo \"(from the branch description for $branch_name local branch)\"\n \techo\n \tgit config \"branch.$branch_name.description\"\n-fi &&\n-\n-if test -n \"$tag_name\"\n-then\n-\tif test -z \"$ref\" || test \"$ref\" != \"tags/$tag_name\"\n-\tthen\n-\t\techo >&2 \"warn: You locally have $tag_name but it does not (yet)\"\n-\t\techo >&2 \"warn: appear to be at $url\"\n-\t\techo >&2 \"warn: Do you want to push it there, perhaps?\"\n-\tfi\n-\tgit cat-file tag \"$tag_name\" |\n-\tsed -n -e '1,/^$/d' -e '/^-----BEGIN PGP /q' -e p\n-\techo\n-fi &&\n-\n-if test -n \"$branch_name\" || test -n \"$tag_name\"\n-then\n \techo \"----------------------------------------------------------------\"\n fi &&\n \n git shortlog ^$baserev $headrev &&\n git diff -M --stat --summary $patch $merge_base..$headrev || status=1\n \n-if test -z \"$ref\"\n-then\n-\techo \"warn: No branch of $url is at:\" >&2\n-\tgit show -s --format='warn:   %h: %s' $headrev >&2\n-\techo \"warn: Are you sure you pushed '$head' there?\" >&2\n-\tstatus=1\n-fi\n exit $status\n-- \n1.9.rc0.7.gd9bb4be.dirty\n"},{"id":"233549","messageId":"xmqq1tzzy9ip.fsf@gitster.dls.corp.google.com","threadId":"35713","inReplyTo":"alpine.LFD.2.11.1401221243090.18459@i7.linux-foundation.org","subject":"Re: [RFC PATCH] Make 'git request-pull' more strict about matching local/remote branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-22T21:46:54Z","receivedAt":"2014-01-22T21:46:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> This means that git request-pull will never rewrite the ref-name you gave \n> it.  If the local branch name is \"xyzzy\", that is the only branch name \n> that request-pull will ask the other side to fetch.\n>\n> If the remote has that branch under a different name, that's your problem \n> and git request-pull will not try to fix it up (but git request-pull will \n> warn about the fact that no exact matching branch is found, and you can \n> edit the end result to then have the remote name you want if it doesn't \n> match your local one).\n>\n> The new \"find local ref\" code will also complain loudly if you give an\n> ambiguous refname (eg you have both a tag and a branch with that same\n> name, and you don't specify \"heads/name\" or \"tags/name\").\n\nI agree with the basic direction, especially the part \"we will never\nrewrite\", is quite attractive.\n\nBut this part might be a bit problematic.  $3=master will almost\nalways have refs/heads/master and refs/remotes/origin/master listed\nbecause the call to \"show-ref\" comes before \"rev-parse --verify\",\nno?\n\n> +head=$(git symbolic-ref -q \"${3-HEAD}\")\n> +head=${head:-$(git show-ref \"${3-HEAD}\" | cut -d' ' -f2)}\n> +head=${head:-$(git rev-parse --quiet --verify \"$3\")}\n> +\n> +# None of the above? Bad.\n> +test -z \"$head\" && die \"fatal: Not a valid revision: $3\"\n> +\n> +# This also verifies that the resulting head is unique:\n> +# \"git show-ref\" could have shown multiple matching refs..\n>  headrev=$(git rev-parse --verify --quiet \"$head\"^0)\n> -if test -z \"$headrev\"\n> +test -z \"$headrev\" && die \"fatal: Ambiguous revision: $3\"\n\n... and it would die here.  $3=for-linus may be the most common case\non the kernel list, and remotes/origin/for-linus may be unlikely to\nappear in the real life (hmph, really?  perhaps people keep track of\nwhat they pushed out the last time with it, I dunno).\n"},{"id":"233550","messageId":"CA+55aFxSiKhM=4GtFaruWNZFNaUgCpVHsJOWJ9Qcd19nz6YYTw@mail.gmail.com","threadId":"35713","inReplyTo":"xmqq1tzzy9ip.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC PATCH] Make 'git request-pull' more strict about matching local/remote branches","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2014-01-22T22:03:47Z","receivedAt":"2014-01-22T22:03:47Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Wed, Jan 22, 2014 at 1:46 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> The new \"find local ref\" code will also complain loudly if you give an\n>> ambiguous refname (eg you have both a tag and a branch with that same\n>> name, and you don't specify \"heads/name\" or \"tags/name\").\n>\n> But this part might be a bit problematic.  $3=master will almost\n> always have refs/heads/master and refs/remotes/origin/master listed\n> because the call to \"show-ref\" comes before \"rev-parse --verify\",\n> no?\n\nHmm. Yes.\n\nIt's done that way very much on purpose, to avoid the branch/tag\nambiguity (which we have had problems with), but you're right, it also\nends up being ambiguous wrt remote branches, which wasn't the\nintention, and you're right, that is not acceptable.\n\nDamn. I very much want to get the full ref-name (ie \"master\" should\nbecome \"refs/heads/master\"), and I do want to avoid the branch/tag\nambiguity, but you're right, \"show-ref\" plus the subsequent \"rev-parse\n--verify\" comes close but not quite close enough.\n\nAny ideas? The hacky way is to do \"| head -1\" to take the first\nshow-ref output, and then check if you get a different result if you\nre-do it using \"show-ref --tags\". But that sounds really excessively\nhacky. Is there a good way to do it?\n\n                 Linus\n"},{"id":"233551","messageId":"CA+55aFwhrfbxVZDjf8mcaTOXqGEjjirTq_S-8N2H60VcVP7P6w@mail.gmail.com","threadId":"35713","inReplyTo":"CA+55aFxSiKhM=4GtFaruWNZFNaUgCpVHsJOWJ9Qcd19nz6YYTw@mail.gmail.com","subject":"Re: [RFC PATCH] Make 'git request-pull' more strict about matching local/remote branches","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2014-01-22T22:07:58Z","receivedAt":"2014-01-22T22:07:58Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Wed, Jan 22, 2014 at 2:03 PM, Linus Torvalds\n<torvalds@linux-foundation.org> wrote:\n>\n> Any ideas? The hacky way is to do \"| head -1\" to take the first\n> show-ref output, and then check if you get a different result if you\n> re-do it using \"show-ref --tags\". But that sounds really excessively\n> hacky. Is there a good way to do it?\n\nUsing \"git show-refs --tags --heads\" would work for the common case\n(since that ignores remote branches), but would then disallow remote\nbranches entirely.\n\nThat might be ok in practice, but it's definitely wrong too.\n\nI'm probably missing some obvious solution.\n\n                    Linus\n"},{"id":"233552","messageId":"xmqqwqhrwtoy.fsf@gitster.dls.corp.google.com","threadId":"35713","inReplyTo":"alpine.LFD.2.11.1401221243090.18459@i7.linux-foundation.org","subject":"Re: [RFC PATCH] Make 'git request-pull' more strict about matching local/remote branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-22T22:14:05Z","receivedAt":"2014-01-22T22:14:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> That may be very helpful if your local tree doesn't match the layout of \n> the remote branches, but for the common case it's been a recurring \n> disaster, when \"request-pull\" is done against a delayed remote update, and \n> it rewrites the target branch randomly to some other branch name that \n> happens to have the same expected SHA1 (or more commonly, leaves it \n> blank).\n\nThinking about this a bit more...\n\n> Comments? It passes the tests I put it through locally, but I did *not* \n> make it pass the test-suite, since it very much does change the rules. \n> Some of the test suite code literally tests for the old completely broken \n> case (at least t5150, subtests 4 and 5).\n\nI looked at 5150.4 and found that what it attempts to do is halfway\nsensible.  The contributor works on the local 'master' branch,\npublishes the result to 'for-linus' in its 'origin' repository, and\nasks his state to be pulled, with:\n\n\tgit push origin master:for-linus\n        git request-pull initial origin\n\nThe contributor could be more explicit in his request-pull and say \n\n        git request-pull initial origin master\n\nbut there is no 'master' on the publishing repository in this case\n(or even if there is, it does not match what is being pushed out),\nand there is no 'for-linus' branch locally, so there is no way for\nhim to say\n\n        git request-pull initial origin for-linus\n\nunless he creates it locally first.\n\nI am starting to wonder if it is a better fix to check potentially\nambiguous cases (e.g. the publishing repository does have 'master'\nthat does not point at the commit local 'master' points at, and\n'for-linus' that points at the same commit, and the user asks for\n'master' to be pulled) or clearly broken cases (e.g. the user gave\nsomething other than HEAD explicitly from the command line, but we\nended up computing blank) and die loudly, without breaking cases\nthis test tries to protect.\n\nOn the other hand, I tend to think what 5150.5 wants is convoluted\nand expects a broken behaviour.  Its publishing repository has\n'master' and 'for-upstream', and also has 'tags/full' that is an\nannotated tag that points at the commit, runs request-pull with its\nlocal 'master', and expects the resulting message to ask 'tags/full'\nto be pulled.  If the contributor wants such a non-commit to be\npulled, I think it should be made more explicit, i.e., not with\n\n\tgit request-pull initial $origin_url\n\nbut with\n\n\tgit request-pull initial $origin_url tags/full\n\nor something.\n"},{"id":"233553","messageId":"CA+55aFxdNv+Y+OzSazY=yaEGkG57+HsiDCi93tBhvNHarSFyjA@mail.gmail.com","threadId":"35713","inReplyTo":"xmqqwqhrwtoy.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC PATCH] Make 'git request-pull' more strict about matching local/remote branches","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2014-01-22T22:20:34Z","receivedAt":"2014-01-22T22:20:34Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Wed, Jan 22, 2014 at 2:14 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> I looked at 5150.4 and found that what it attempts to do is halfway\n> sensible.\n\nI agree that it is \"half-way sensible\". The important bit being the HALF part.\n\nThe half part is why we have the semantics we have. There's no\nquestion about that.\n\nThe problem is, the *other* half is pure and utter crap. The \"half-way\nsensible\" solution then generates pure and utter garbage in the\n\"totally sensible\" case.\n\nAnd that's why I think it needs to be fixed. Not because the existing\nbehavior can never make sense in some circumstances, but because the\nexisting behavior can screw up really really badly in other (arguably\nmore common, and definitely real) circumstances.\n\nFor the kernel, the broken \"missing branch name\" situation has come up\npretty regularly. This is definitely not a one-time event, it's more\nlike \"almost every merge window somebody gets screwed by this and I\nhave to guess what the branch name should have been\".\n\nI think that we could potentially do a \"local:remote\" syntax for that\nhalf-way sensible case, so that if you do\n\n   git push .. master:for-linus\n\nthen you have to do\n\n   git request-pull .. master:for-linus\n\nto match the fact that you renamed your local branch on the remote.\n\n              Linus\n"},{"id":"233554","messageId":"xmqqsisfwt3c.fsf@gitster.dls.corp.google.com","threadId":"35713","inReplyTo":"xmqqwqhrwtoy.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC PATCH] Make 'git request-pull' more strict about matching local/remote branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-22T22:27:03Z","receivedAt":"2014-01-22T22:27:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> ... there is no 'for-linus' branch locally, so there is no way for\n> him to say\n>\n>         git request-pull initial origin for-linus\n>\n> unless he creates it locally first.\n\nIn real life on the kernel list, for-linus may have to be a signed\ntag, and pushed out 1-to-1 name mapping, so in that sense, \"unless\nhe creates it locally first\" may not be a problem.\n\nI'd throw this into \"No, this is not the only way to do so and there\nare workarounds available if we suddenly tightened the rule and\nbroke those who relied on this behaviour. But this is not a less\ngood way compared to the alternative of creating the same-named ref\nfirst, so we _are_ breaking people deliberately---is that worth the\nsafety for always-push-one-to-one people?\" category.\n\nI'd throw the other one (i.e. 5150.5) into \"that is crazy enough\nthat a short apology in the Release Notes is sufficient before\nbreaking those who relied on that behaviour\" category, on the other\nhand ;-).\n"}]}