{"thread":{"id":"35715","subject":"[RFC PATCH 2/1] Make request-pull able to take a refspec of form local:remote","startedAt":"2014-01-23T00:06:32Z","lastAt":"2014-03-13T21:22:00Z","messageCount":13,"participants":["Linus Torvalds","Junio C Hamano","brian m. carlson","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"233571","messageId":"alpine.LFD.2.11.1401221535140.18207@i7.linux-foundation.org","threadId":"35715","inReplyTo":null,"subject":"[RFC PATCH 2/1] Make request-pull able to take a refspec of form local:remote","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2014-01-23T00:06:32Z","receivedAt":"2014-01-23T00:06:32Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nFrom: Linus Torvalds \nDate: Wed, 22 Jan 2014 15:23:48 -0800\nSubject: [PATCH] Make request-pull able to take a refspec of form local:remote\n\nThis allows a user to say that a local branch has a different name on\nthe remote server, using the same syntax that \"git push\" uses to create\nthat situation.\n\nSigned-off-by: Linus Torvalds \n---\n\nSo this relaxes the remote matching, and allows using the \"local:remote\" \nsyntax to say that the local branch is differently named from the remote \none.\n\nIt is probably worth folding it into the previous patch if you think this \nwhole approach is workable.\n\n git-request-pull.sh | 50 +++++++++++++++++++++++++++++---------------------\n 1 file changed, 29 insertions(+), 21 deletions(-)\n\ndiff --git a/git-request-pull.sh b/git-request-pull.sh\nindex 659a412155d8..c8ab0e912011 100755\n--- a/git-request-pull.sh\n+++ b/git-request-pull.sh\n@@ -47,19 +47,23 @@ fi\n \n #\n # $3 must be a symbolic ref, a unique ref, or\n-# a SHA object expression\n+# a SHA object expression. It can also be of\n+# the format 'local-name:remote-name'.\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+local=${3%:*}\n+local=${local:-HEAD}\n+remote=${3#*:}\n+head=$(git symbolic-ref -q \"$local\")\n+head=${head:-$(git show-ref --heads --tags \"$local\" | cut -d' ' -f2)}\n+head=${head:-$(git rev-parse --quiet --verify \"$local\")}\n \n # None of the above? Bad.\n-test -z \"$head\" && die \"fatal: Not a valid revision: $3\"\n+test -z \"$head\" && die \"fatal: Not a valid revision: $local\"\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-test -z \"$headrev\" && die \"fatal: Ambiguous revision: $3\"\n+test -z \"$headrev\" && die \"fatal: Ambiguous revision: $local\"\n \n # Was it a branch with a description?\n branch_name=${head#refs/heads/}\n@@ -69,9 +73,6 @@ then\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@@ -81,30 +82,37 @@ die \"fatal: No commits in common between $base and $head\"\n #\n # Otherwise find a random ref that matches $headrev.\n find_matching_ref='\n-\tmy ($exact,$found);\n+\tmy ($head,$headrev) = (@ARGV);\n+\tmy ($found);\n+\n \twhile () {\n+\t\tchomp;\n \t\tmy ($sha1, $ref, $deref) = /^(\\S+)\\s+([^^]+)(\\S*)$/;\n-\t\tnext unless ($sha1 eq $ARGV[1]);\n-\t\tif ($ref eq $ARGV[0]) {\n-\t\t\t$exact = $ref;\n+\t\tmy ($pattern);\n+\t\tnext unless ($sha1 eq $headrev);\n+\n+\t\t$pattern=\"/$head\\$\";\n+\t\tif ($ref eq $head) {\n+\t\t\t$found = $ref;\n+\t\t}\n+\t\tif ($ref =~ /$pattern/) {\n+\t\t\t$found = $ref;\n \t\t}\n-\t\tif ($sha1 eq $ARGV[0]) {\n+\t\tif ($sha1 eq $head) {\n \t\t\t$found = $sha1;\n \t\t}\n \t}\n-\tif ($exact) {\n-\t\tprint \"$exact\\n\";\n-\t} elsif ($found) {\n+\tif ($found) {\n \t\tprint \"$found\\n\";\n \t}\n '\n \n-ref=$(git ls-remote \"$url\" | @@PERL@@ -e \"$find_matching_ref\" \"$head\" \"$headrev\")\n+ref=$(git ls-remote \"$url\" | @@PERL@@ -e \"$find_matching_ref\" \"${remote:-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+\techo \"warn: No match for commit $headrev found at $url\" >&2\n+\techo \"warn: Are you sure you pushed '${remote:-HEAD}' there?\" >&2\n \tstatus=1\n fi\n \n@@ -116,7 +124,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 $prettyhead\" &&\n+echo \" $url $remote\" &&\n git show -s --format='\n for you to fetch changes up to %H:\n \n-- \n1.9.rc0.10.gf0799f9.dirty\n"},{"id":"233612","messageId":"xmqqlhy6trfp.fsf@gitster.dls.corp.google.com","threadId":"35715","inReplyTo":"alpine.LFD.2.11.1401221535140.18207@i7.linux-foundation.org","subject":"Re: [RFC PATCH 2/1] Make request-pull able to take a refspec of form local:remote","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-23T19:43:22Z","receivedAt":"2014-01-23T19:43:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> So this relaxes the remote matching, and allows using the \"local:remote\" \n> syntax to say that the local branch is differently named from the remote \n> one.\n>\n> It is probably worth folding it into the previous patch if you think this \n> whole approach is workable.\n\nHaven't thought enough to decide on that part, yet.\n\n> # $3 must be a symbolic ref, a unique ref, or\n> -# a SHA object expression\n> +# a SHA object expression. It can also be of\n> +# the format 'local-name:remote-name'.\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> +local=${3%:*}\n> +local=${local:-HEAD}\n> +remote=${3#*:}\n> +head=$(git symbolic-ref -q \"$local\")\n> +head=${head:-$(git show-ref --heads --tags \"$local\" | cut -d' ' -f2)}\n> +head=${head:-$(git rev-parse --quiet --verify \"$local\")}\n> \n> # None of the above? Bad.\n> -test -z \"$head\" && die \"fatal: Not a valid revision: $3\"\n> +test -z \"$head\" && die \"fatal: Not a valid revision: $local\"\n> \n> # This also verifies that the resulting head is unique:\n\nI am not sure if it is a good idea to hand-craft \"resulting head is\nunique\" constraint here. We already have disambiguation rules (and\nwarning mechanism) we use in other places---this part should use the\nsame rule, I think.\n\nA short experiment tells me that we are almost there:\n\n $ git init && git commit --allow-empty -m initial\n $ git branch other && git tag master\n $ git -c core.warnambiguousrefs=true \\\n rev-parse --symbolic-full-name other\n refs/heads/other\n $ git -c core.warnambiguousrefs=true \\\n rev-parse --symbolic-full-name master; echo $?\n warning: refname 'master' is ambiguous.\n error: refname 'master' is ambiguous\n 0\n\nWe say \"error\" but we do not actually error out, but that shouldn't\nbe too hard to fix.\n\n> # \"git show-ref\" could have shown multiple matching refs..\n> headrev=$(git rev-parse --verify --quiet \"$head\"^0)\n> -test -z \"$headrev\" && die \"fatal: Ambiguous revision: $3\"\n> +test -z \"$headrev\" && die \"fatal: Ambiguous revision: $local\"\n> \n> # Was it a branch with a description?\n> branch_name=${head#refs/heads/}\n> @@ -69,9 +73,6 @@ then\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> @@ -81,30 +82,37 @@ die \"fatal: No commits in common between $base and $head\"\n> #\n> # Otherwise find a random ref that matches $headrev.\n> find_matching_ref='\n> +\tmy ($head,$headrev) = (@ARGV);\n> +\tmy ($found);\n> +\n> \twhile () {\n> +\t\tchomp;\n> \t\tmy ($sha1, $ref, $deref) = /^(\\S+)\\s+([^^]+)(\\S*)$/;\n> +\t\tmy ($pattern);\n> +\t\tnext unless ($sha1 eq $headrev);\n> +\n> +\t\t$pattern=\"/$head\\$\";\n\nI think $head is constant inside the loop, so lift it outside?\n\n> +\t\tif ($ref eq $head) {\n> +\t\t\t$found = $ref;\n> +\t\t}\n> +\t\tif ($ref =~ /$pattern/) {\n> +\t\t\t$found = $ref;\n> \t\t}\n> +\t\tif ($sha1 eq $head) {\n\nI think this is $headrev ($head may be $remote or HEAD), but then\nanything that does not point at $headrev has already been rejected\nat the beginning of this loop, so...?\n\n> \t\t\t$found = $sha1;\n> \t\t}\n> \t}\n> +\tif ($found) {\n> \t\tprint \"$found\\n\";\n> \t}\n> '\n\nI somehow feel that this is inadequate to catch the \"delayed\npropagation\" error in the opposite direction. The publish\nrepository may have an unrelated ref pointing at the $headrev and we\nmay guess that is the ref to be fetched by the integrator based on\nthat, but by the time the integrator fetches from the repository,\nthe ref may have been updated to its new value that does not match\n$headrev. But I do not think of a way to solve that one.\n\nIn any case, shouldn't we be catching duplicate matches here, if the\nreal objective is to make it less likely for the users to make\nmistakes? Otherwise, if there are two 'master' over there\n(e.g. refs/heads/master and refs/remotes/origin/master), it seems to\nme that $ref =~ /$pattern/ will trigger twice in your loop and ends\nup reporting the last match.\n\nIn other words,\n\n\tmy (@found) = ();\n my (@guessed) = ();\n\twhile () {\n\t\tnext unless ($sha1 eq $headrev);\n \t...\n if ($ref =~ /$pattern/) {\n \tpush @found, $ref;\n\t\t} else {\n \tpush @guessed, $ref;\n\t\t}\n\t}\n if (@found == 1) {\n \tprint \"$found[0]\\n\";\n\t} else if (@guessed == 1) {\n \tprint \"$guessed[0]\\n\";\n }\n\nor something like that?\n\n> -ref=$(git ls-remote \"$url\" | @@PERL@@ -e \"$find_matching_ref\" \"$head\" \"$headrev\")\n> +ref=$(git ls-remote \"$url\" | @@PERL@@ -e \"$find_matching_ref\" \"${remote:-HEAD}\" \"$headrev\")\n\nThere are three cases as far as ${remote:-HEAD} aka $head in the\nscript is concerned:\n\n 1. The user said $3=local:remote; we do not want to guess, and we\n pass it thru, e.g. 'master:for-linus' will give 'for-linus',\n likely to mean 'refs/heads/for-linus' or 'refs/tags/for-linus'.\n The loop must find it if it is there and is a unique match.\n\n 2. The user said $3=branch.\n\n a) It may have been pushed to the branch of the same name over\n there. The loop must find it if it is there and is a unique\n match.\n\n b) It may have been pushed out as 'for-linus' branch or tag over\n there. The loop must find it based on $headrev.\n\n 3. The user said $3=\"\", implying HEAD.\n\n Passing HEAD does not sound like a good way to handle this\n case, as the loop in find_matching_ref script would try to use\n it literally.\n\n With \"git symbolic-ref HEAD | sed -e 's|^refs/heads/||\" or an\n equivalent of it is used instead, we can reuse the logic in the\n second case fully, no?\n\nIt is too early to include in the discussion, but Peff's B@{publish}\nnotation may play a role in the future to convert 'local' to 'remote'\nwithout the user having to say 'local:remote'.\n\nThanks.\n"},{"id":"233619","messageId":"CA+55aFyGaaMOL5pBhZ1BHMr07oDi2MuS-fPu4nnxhjoy+F0AQw@mail.gmail.com","threadId":"35715","inReplyTo":"xmqqlhy6trfp.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC PATCH 2/1] Make request-pull able to take a refspec of form local:remote","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2014-01-23T19:57:30Z","receivedAt":"2014-01-23T19:57:30Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Thu, Jan 23, 2014 at 11:43 AM, Junio C Hamano wrote:\n>\n> I am not sure if it is a good idea to hand-craft \"resulting head is\n> unique\" constraint here. We already have disambiguation rules (and\n> warning mechanism) we use in other places---this part should use the\n> same rule, I think.\n\nIf you can fix that, then yes, that would be lovely. As it is, I\ncouldn't find any easily scriptable way to do that.\n\n>> #\n>> # Otherwise find a random ref that matches $headrev.\n>> find_matching_ref='\n>> + my ($head,$headrev) = (@ARGV);\n>> + my ($found);\n>> +\n>> while () {\n>> + chomp;\n>> my ($sha1, $ref, $deref) = /^(\\S+)\\s+([^^]+)(\\S*)$/;\n>> + my ($pattern);\n>> + next unless ($sha1 eq $headrev);\n>> +\n>> + $pattern=\"/$head\\$\";\n>\n> I think $head is constant inside the loop, so lift it outside?\n\nYes. I'm not really a perl person, and this came from me trying to\nmake the code more readable (and it used to do that magic quoting\nthing inside the loop, I just used a helper pattern variable).\n\n>> + if ($sha1 eq $head) {\n>\n> I think this is $headrev ($head may be $remote or HEAD), but then\n> anything that does not point at $headrev has already been rejected\n> at the beginning of this loop, so...?\n\nNo, this is for when \"head\" ends up not being a ref, but a SHA1 expression.\n\nIOW, for when you do something odd like\n\n git request-pull HEAD^^ origin HEAD^\n\nwhen hacking things together. It doesn't actually generate the right\nrequest-pull message (because there's no valid branch name), but it\n*works* in the sense that you can get the diffstat etc and edit things\nmanually.\n\nIt's not a big deal - it has never really \"worked\", and I actually\nbroke that when I then used \"$remote\" that doesn't actually have the\nSHA1 any more.\n\n>> + if ($found) {\n>> print \"$found\\n\";\n>> }\n>> '\n>\n> I somehow feel that this is inadequate to catch the \"delayed\n> propagation\" error in the opposite direction. The publish\n> repository may have an unrelated ref pointing at the $headrev and we\n> may guess that is the ref to be fetched by the integrator based on\n> that, but by the time the integrator fetches from the repository,\n> the ref may have been updated to its new value that does not match\n> $headrev. But I do not think of a way to solve that one.\n\nYes, so you'll get a warning (or, if you get a partial match, maybe\nnot even that), but the important part about all these changes is that\nit DOESN'T MATTER.\n\nWhy? Because it no longer re-writes the target branch name based on\nthat match or non-match. So the pull request will be fine.\n\nIn other words, the really fundamental change here i that the \"oops, I\ncouldn't find things on the remote\" no longer affects the output. It\nonly affects the warning. And I think that's important.\n\nIt used to be that the remote matching actually changed the output of\nthe request-pull, and *THAT* was the fundamental problem.\n\n> In any case, shouldn't we be catching duplicate matches here, if the\n> real objective is to make it less likely for the users to make\n> mistakes?\n\nIt would be good, yes. But my perl-fu is weak, and I really didn't\nwant to worry about it. Also, as above: my primary issue was to not\nscrew up the output, so the remote matching actually has become much\nless important, and now the warning about it is purely about being\nhelpful, it no longer fundamentally alters any semantics.\n\nSo I agree that there is room for improvement, but that's kind of\nseparate from the immediate problem I was trying to solve.\n\n Linus\n"},{"id":"233653","messageId":"xmqqsises3u0.fsf@gitster.dls.corp.google.com","threadId":"35715","inReplyTo":"CA+55aFyGaaMOL5pBhZ1BHMr07oDi2MuS-fPu4nnxhjoy+F0AQw@mail.gmail.com","subject":"Re: [RFC PATCH 2/1] Make request-pull able to take a refspec of form local:remote","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-23T22:58:31Z","receivedAt":"2014-01-23T22:58:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> Yes, so you'll get a warning (or, if you get a partial match, maybe\n> not even that), but the important part about all these changes is that\n> it DOESN'T MATTER.\n>\n> Why? Because it no longer re-writes the target branch name based on\n> that match or non-match. So the pull request will be fine.\n\nWill be fine, provided if they always use local:remote syntax, I'd\nagree.\n\n> In other words, the really fundamental change here i that the \"oops, I\n> couldn't find things on the remote\" no longer affects the output. It\n> only affects the warning. And I think that's important.\n>\n> It used to be that the remote matching actually changed the output of\n> the request-pull, and *THAT* was the fundamental problem.\n\nThe fingers of users can be retrained to use the local:remote syntax\nafter we apologize for this change in the Release Notes, I see only\none issue (we seem to lose the message from the annotated/signed tag\nwhen asking to pull it) remaining, after looking at what updates are\nneeded for t5150.\n\nThanks.\n\n-- >8 --\nSubject: [PATCH] pull-request: test updates\n\nThis illustrates behaviour changes that result from the recent\nchange by Linus. Most shows good changes, but there may be\nusability regressions:\n\n - The command continues to fail when the user forgot to push out\n before running the command, but the wording of the message has\n been slightly changed.\n\n - The command no longer guesses when asked to request the commit at\n the HEAD be pulled after pushing it to a branch 'for-upstream',\n even when that branch points at the correct commit. The user\n must ask the command with the new \"master:for-upstream\" syntax.\n\n - The command no longer favours a tag that peels to the requested\n commit over a branch that points at the same commit. This needs\n to be asked explicitly by specifying the tag object, not the\n commit. But somehow this does not see to work (yet); somewhere\n the \"tag-ness\" of the requested ref seems to be lost.\n\nThe new behaviour needs to be documented in any case, but we need to\nagree what the new behaviour should be before doing so, so...\n\nSigned-off-by: Junio C Hamano \n---\n t/t5150-request-pull.sh | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t5150-request-pull.sh b/t/t5150-request-pull.sh\nindex 1afa0d5..412ee4f 100755\n--- a/t/t5150-request-pull.sh\n+++ b/t/t5150-request-pull.sh\n@@ -86,7 +86,7 @@ test_expect_success 'setup: two scripts for reading pull requests' '\n \ts/[-0-9]\\{10\\} [:0-9]\\{8\\} [-+][0-9]\\{4\\}/DATE/g\n \ts/ [^ ].*/ SUBJECT/g\n \ts/ [^ ].* (DATE)/ SUBJECT (DATE)/g\n-\ts/for-upstream/BRANCH/g\n+\ts|tags/full|BRANCH|g\n \ts/mnemonic.txt/FILENAME/g\n \ts/^version [0-9]/VERSION/\n \t/^ FILENAME | *[0-9]* [-+]*\\$/ b diffstat\n@@ -127,7 +127,7 @@ test_expect_success 'pull request when forgot to push' '\n \t\ttest_must_fail git request-pull initial \"$downstream_url\" \\\n \t\t\t2>../err\n \t) &&\n-\tgrep \"No branch of.*is at:\\$\" err &&\n+\tgrep \"No match for commit .*\" err &&\n \tgrep \"Are you sure you pushed\" err\n \n '\n@@ -141,7 +141,7 @@ test_expect_success 'pull request after push' '\n \t\tgit checkout initial &&\n \t\tgit merge --ff-only master &&\n \t\tgit push origin master:for-upstream &&\n-\t\tgit request-pull initial origin >../request\n+\t\tgit request-pull initial origin master:for-upstream >../request\n \t) &&\n \tsed -nf read-request.sed digest &&\n \tcat digest &&\n@@ -160,7 +160,7 @@ test_expect_success 'pull request after push' '\n \n '\n \n-test_expect_success 'request names an appropriate branch' '\n+test_expect_success 'request asks HEAD to be pulled' '\n \n \trm -fr downstream.git &&\n \tgit init --bare downstream.git &&\n@@ -179,11 +179,11 @@ test_expect_success 'request names an appropriate branch' '\n \t\tread repository &&\n \t\tread branch\n \t} ../request\n+\t\tgit push origin tags/full &&\n+\t\tgit request-pull initial \"$downstream_url\" tags/full >../request\n \t) &&\n \trequest.fuzzy &&\n \ttest_i18ncmp expect request.fuzzy\n@@ -229,7 +229,7 @@ test_expect_success 'request-pull ignores OPTIONS_KEEPDASHDASH poison' '\n \t\tgit checkout initial &&\n \t\tgit merge --ff-only master &&\n \t\tgit push origin master:for-upstream &&\n-\t\tgit request-pull -- initial \"$downstream_url\" >../request\n+\t\tgit request-pull -- initial \"$downstream_url\" master:for-upstream >../request\n \t)\n \n '\n-- \n1.9-rc0-250-ge2d8c96\n"},{"id":"233661","messageId":"CA+55aFyN7WWAF6pGfP+0j29nf6ETao0J5sUu+5UDaXUYC9_Geg@mail.gmail.com","threadId":"35715","inReplyTo":"xmqqsises3u0.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC PATCH 2/1] Make request-pull able to take a refspec of form local:remote","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2014-01-23T23:56:14Z","receivedAt":"2014-01-23T23:56:14Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Thu, Jan 23, 2014 at 2:58 PM, Junio C Hamano wrote:\n>\n> Will be fine, provided if they always use local:remote syntax, I'd\n> agree.\n\nWhy? No sane user should actually need to use the local:remote syntax.\n\nThe normal situation should be that you create the correctly named\nbranch or tag locally, and then push it out under that name.\n\nSo I don't actually think anybody should need to be retrained, or\n\"always use the local:remote\" syntax. The local:remote syntax exists\nonly for that special insane case where you used (the same)\nlocal:remote syntax to push out a branch under a different name.\n\n[ And yeah, maybe that behavior is more common than I think, but even\nif it is, such behavior would always be among people who are *very*\naware of the whole \"local branch vs remote branch name is different\"\nsituation. ]\n\n Linus\n"},{"id":"233715","messageId":"xmqqvbx9qgo7.fsf@gitster.dls.corp.google.com","threadId":"35715","inReplyTo":"CA+55aFyN7WWAF6pGfP+0j29nf6ETao0J5sUu+5UDaXUYC9_Geg@mail.gmail.com","subject":"Re: [RFC PATCH 2/1] Make request-pull able to take a refspec of form local:remote","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-24T20:16:24Z","receivedAt":"2014-01-24T20:16:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> So I don't actually think anybody should need to be retrained, or\n> \"always use the local:remote\" syntax. The local:remote syntax exists\n> only for that special insane case where you used (the same)\n> local:remote syntax to push out a branch under a different name.\n>\n> [ And yeah, maybe that behavior is more common than I think, but even\n> if it is, such behavior would always be among people who are *very*\n> aware of the whole \"local branch vs remote branch name is different\"\n> situation. ]\n\nAs the new default for \"git push\" would push to the same name, I\nagree that people who are now forced to use local:remote syntax\nwould be the ones who know what they are doing [*1*].\n\nSo there are two remaining items, I think.\n\n - After creating a tags/for-linus signed tag and pushing it to\n tags/for-linus, asking request-pull to request that tag to be\n pulled seems to lose the tag message from the output.\n\n - Docs.\n\n\n[Footnote]\n\n*1* Not that it is always acceptable to break the existing users as\n long as they are clueful ones and they are given an escape hatch.\n But this time I know I won't be in the middle of firestorm like\n the one we had immediately after 1.6.0, as long as I keep the\n URL of the message I am responding to in the list archive ;-)\n"},{"id":"233910","messageId":"xmqqfvo6icqf.fsf_-_@gitster.dls.corp.google.com","threadId":"35715","inReplyTo":"xmqqvbx9qgo7.fsf@gitster.dls.corp.google.com","subject":"Re* [RFC PATCH 2/1] Make request-pull able to take a refspec of form local:remote","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-29T23:34:32Z","receivedAt":"2014-01-29T23:34:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> So there are two remaining items, I think.\n>\n> - After creating a tags/for-linus signed tag and pushing it to\n> tags/for-linus, asking request-pull to request that tag to be\n> pulled seems to lose the tag message from the output.\n>\n> - Docs.\n>\n> [Footnote]\n>\n> *1* Not that it is always acceptable to break the existing users as\n> long as they are clueful ones and they are given an escape hatch.\n> But this time I know I won't be in the middle of firestorm like\n> the one we had immediately after 1.6.0, as long as I keep the\n> URL of the message I am responding to in the list archive ;-)\n\nI am not yet doing the docs, but here is a minimal (and I think is\nthe most sensible) fix to the \"If I asked a tag to be pulled, I used\nto get the message from the tag in the output---the updated code no\nlonger does so\" problem.\n\nWith this fix, the updates to t5150 I queued on top of the two\npatches can lose a \"test_expect_failure\".\n\nI would not be surprised if there are other regressions, though\n[*1*]. I am worried about regressions when the user explicitly asks\na ref to be pulled---e.g the command refuses to produce output and\ninstead fails (perhaps because the ambiguity check is overly\nstricter than it should be), or the command produces output that is\ndifferent from what we used to produce (this patch is a fix to the\nproblem in that latter category, but there may be other differences\nthe existing tests are not covering).\n\n[Footnote]\n\n*1* No, I do not count \"I used to be able to ask 'master' (or\nimplicitly 'HEAD' that I happen to be sitting on) to be pulled and\nrely on that the command figures out that I have that commit on\n'for-linus' in my publish repository, but that feature was removed\"\nas a regression. Removing that cleverness is the point of this\nseries.\n\n-- >8 --\nSubject: [PATCH] request-pull: pick up tag message as before\n\nThe previous two steps were meant to stop promoting the explicit\nrefname the user gave to the command to a different ref that points\nat it. Most notably, we no longer substitute a branch name the user\nused with a name of the tqag that points at the commit at the tip of\nthe branch.\n\nHowever, they also lost the code that included the message in a\ntag when the user _did_ ask the tag to be pulled. Resurrect it.\n\nSigned-off-by: Junio C Hamano \n---\n git-request-pull.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/git-request-pull.sh b/git-request-pull.sh\nindex c8ab0e9..93b4135 100755\n--- a/git-request-pull.sh\n+++ b/git-request-pull.sh\n@@ -132,6 +132,14 @@ for you to fetch changes up to %H:\n \n ----------------------------------------------------------------' $headrev &&\n \n+if test $(git cat-file -t \"$head\") = tag\n+then\n+\tgit cat-file tag \"$head\" |\n+\tsed -n -e '1,/^$/d' -e '/^-----BEGIN PGP /q' -e p\n+\techo\n+\techo \"----------------------------------------------------------------\"\n+fi &&\n+\n if test -n \"$branch_name\"\n then\n \techo \"(from the branch description for $branch_name local branch)\"\n-- \n1.9-rc1-183-g614c158\n"},{"id":"233912","messageId":"20140130001630.GD91854@vauxhall.crustytoothpaste.net","threadId":"35715","inReplyTo":"xmqqfvo6icqf.fsf_-_@gitster.dls.corp.google.com","subject":"Re: Re* [RFC PATCH 2/1] Make request-pull able to take a refspec of form local:remote","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2014-01-30T00:16:30Z","receivedAt":"2014-01-30T00:16:30Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Jan 29, 2014 at 03:34:32PM -0800, Junio C Hamano wrote:\n> The previous two steps were meant to stop promoting the explicit\n> refname the user gave to the command to a different ref that points\n> at it. Most notably, we no longer substitute a branch name the user\n> used with a name of the tqag that points at the commit at the tip of\n\ns/tqag/tag/\n\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"233913","messageId":"CA+55aFyWM+VDa4mdqseNy3tSnp23x=2sPtHf5KpxsfROeHbVGw@mail.gmail.com","threadId":"35715","inReplyTo":"xmqqfvo6icqf.fsf_-_@gitster.dls.corp.google.com","subject":"Re: Re* [RFC PATCH 2/1] Make request-pull able to take a refspec of form local:remote","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2014-01-30T00:40:37Z","receivedAt":"2014-01-30T00:40:37Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Wed, Jan 29, 2014 at 3:34 PM, Junio C Hamano wrote:\n>\n> I am not yet doing the docs, but here is a minimal (and I think is\n> the most sensible) fix to the \"If I asked a tag to be pulled, I used\n> to get the message from the tag in the output---the updated code no\n> longer does so\" problem.\n\nThat was a complete oversight/bug on my part, due to just removing the\ntag_name special cases, not thinking about the tag message.\n\nThinking some more about the tag_name issue, I realize that the other\npatch (\"Make request-pull able to take a refspec of form\nlocal:remote\") broke another thing.\n\nThe first patch pretty-printed the local branch-name, removing \"refs/\"\nand possibly \"heads/\" from the local refname. So for a branch, it\nwould ask people to just pull from the branch-name, and for a tag it\nwould ask people to pull from \"tags/name\", which is good policy. So if\nyou had a tag called \"for-linus\", it would say so (using\n\"tags/for-linus\").\n\nBut the local:remote syntax thing ends up breaking that nice feature.\nThe old find_matching_refs would actually cause us to show the \"tags\"\npart if it existed on the remote, but that had become pointless and\ncounter-productive with the first patch. But with the second patch,\nmaybe we should reinstate that logic..\n\n Linus\n"},{"id":"235337","messageId":"xmqqob1u6f5d.fsf@gitster.dls.corp.google.com","threadId":"35715","inReplyTo":"CA+55aFyWM+VDa4mdqseNy3tSnp23x=2sPtHf5KpxsfROeHbVGw@mail.gmail.com","subject":"Re: Re* [RFC PATCH 2/1] Make request-pull able to take a refspec of form local:remote","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-25T21:44:46Z","receivedAt":"2014-02-25T21:44:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> Thinking some more about the tag_name issue, I realize that the other\n> patch (\"Make request-pull able to take a refspec of form\n> local:remote\") broke another thing.\n>\n> The first patch pretty-printed the local branch-name, removing \"refs/\"\n> and possibly \"heads/\" from the local refname. So for a branch, it\n> would ask people to just pull from the branch-name, and for a tag it\n> would ask people to pull from \"tags/name\", which is good policy. So if\n> you had a tag called \"for-linus\", it would say so (using\n> \"tags/for-linus\").\n>\n> But the local:remote syntax thing ends up breaking that nice feature.\n> The old find_matching_refs would actually cause us to show the \"tags\"\n> part if it existed on the remote, but that had become pointless and\n> counter-productive with the first patch. But with the second patch,\n> maybe we should reinstate that logic..\n\nSorry for back-burnering this topic so long.\n\nI think the following does what you suggested in the message I am\nresponding to.\n\nNow, hopefully the only thing we need is a documentation update and\nthe series should be ready to go.\n\n-- >8 --\nSubject: request-pull: resurrect \"pretty refname\" feature\n\nWhen asking to fetch/pull a branch whose name is B or a tag whose\nname is T, we used to show the command to run as:\n\n\tgit pull $URL B\n git pull $URL tags/T\n\t\neven when B and T were spelled in a more qualified way in order to\ndisambiguate, e.g. heads/B or refs/tags/T, but the recent update\nlost this feature. Resurrect it.\n\nSigned-off-by: Junio C Hamano \n---\n git-request-pull.sh | 4 +++-\n t/t5150-request-pull.sh | 8 +++++++-\n 2 files changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/git-request-pull.sh b/git-request-pull.sh\nindex 93b4135..b67513a 100755\n--- a/git-request-pull.sh\n+++ b/git-request-pull.sh\n@@ -53,6 +53,8 @@ fi\n local=${3%:*}\n local=${local:-HEAD}\n remote=${3#*:}\n+pretty_remote=${remote#refs/}\n+pretty_remote=${pretty_remote#heads/}\n head=$(git symbolic-ref -q \"$local\")\n head=${head:-$(git show-ref --heads --tags \"$local\" | cut -d' ' -f2)}\n head=${head:-$(git rev-parse --quiet --verify \"$local\")}\n@@ -124,7 +126,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 $remote\" &&\n+echo \" $url $pretty_remote\" &&\n git show -s --format='\n for you to fetch changes up to %H:\n \ndiff --git a/t/t5150-request-pull.sh b/t/t5150-request-pull.sh\nindex 2622057..75d6b38 100755\n--- a/t/t5150-request-pull.sh\n+++ b/t/t5150-request-pull.sh\n@@ -216,8 +216,14 @@ test_expect_success 'pull request format' '\n \t\tgit request-pull initial \"$downstream_url\" tags/full >../request\n \t) &&\n \trequest.fuzzy &&\n-\ttest_i18ncmp expect request.fuzzy\n+\ttest_i18ncmp expect request.fuzzy &&\n \n+\t(\n+\t\tcd local &&\n+\t\tgit request-pull initial \"$downstream_url\" tags/full:refs/tags/full\n+\t) >request &&\n+\tsed -nf fuzz.sed request.fuzzy &&\n+\ttest_i18ncmp expect request.fuzzy\n '\n \n test_expect_success 'request-pull ignores OPTIONS_KEEPDASHDASH poison' '\n"},{"id":"236590","messageId":"xmqqvbvjmgys.fsf@gitster.dls.corp.google.com","threadId":"35715","inReplyTo":"xmqqob1u6f5d.fsf@gitster.dls.corp.google.com","subject":"Re: Re* [RFC PATCH 2/1] Make request-pull able to take a refspec of form local:remote","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-12T18:04:11Z","receivedAt":"2014-03-12T18:04:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Sorry for back-burnering this topic so long.\n>\n> I think the following does what you suggested in the message I am\n> responding to.\n>\n> Now, hopefully the only thing we need is a documentation update and\n> the series should be ready to go.\n\n... and here it is, to be sanity checked before I queue the whole\nthing in 'next'.\n\n-- >8 --\nSubject: [PATCH] request-pull: documentation updates\n\nThe original description talked only about what it does. Instead,\nstart it with the purpose of the command, i.e. what it is used for,\nand then mention what it does to achieve that goal.\n\nClarify what , and means in the context of the\noverall purpose of the command.\n\nDescribe the extended syntax of parameter that is used when\nthe local branch name is different from the branch name at the\nrepository the changes are published.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/git-request-pull.txt | 55 +++++++++++++++++++++++++++++++++-----\n 1 file changed, 49 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-request-pull.txt b/Documentation/git-request-pull.txt\nindex b99681c..57bc1f5 100644\n--- a/Documentation/git-request-pull.txt\n+++ b/Documentation/git-request-pull.txt\n@@ -13,22 +13,65 @@ SYNOPSIS\n DESCRIPTION\n -----------\n \n-Summarizes the changes between two commits to the standard output, and includes\n-the given URL in the generated summary.\n+Prepare a request to your upstream project to pull your changes to\n+their tree to the standard output, by summarizing your changes and\n+showing where your changes can be pulled from.\n+\n+The upstream project is expected to have the commit named by\n+`` and the output asks it to integrate the changes you made\n+since that commit, up to the commit named by ``, by visiting\n+the repository named by ``.\n+\n \n OPTIONS\n -------\n -p::\n-\tShow patch text\n+\tInclude patch text in the output.\n \n ::\n-\tCommit to start at.\n+\tCommit to start at. This names a commit that is already in\n+\tthe upstream history.\n \n ::\n-\tURL to include in the summary.\n+\tThe repository URL to be pulled from.\n \n ::\n-\tCommit to end at; defaults to HEAD.\n+\tCommit to end at (defaults to HEAD). This names the commit\n+\tat the tip of the history you are asking to be pulled.\n++\n+When the repository named by `` has the commit at a tip of a\n+ref that is different from the ref you have it locally, you can use\n+the `:` syntax, to have its local name, a colon `:`,\n+and its remote name.\n+\n+\n+EXAMPLE\n+-------\n+\n+Imagine that you built your work on your `master` branch on top of\n+the `v1.0` release, and want it to be integrated to the project.\n+First you push that change to your public repository for others to\n+see:\n+\n+\tgit push https://git.ko.xz/project master\n+\n+Then, you run this command:\n+\n+\tgit request-pull v1.0 https://git.ko.xz/project master\n+\n+which will produce a request to the upstream, summarizing the\n+changes between the `v1.0` release and your `master`, to pull it\n+from your public repository.\n+\n+If you pushed your change to a branch whose name is different from\n+the one you have locally, e.g.\n+\n+\tgit push https://git.ko.xz/project master:for-linus\n+\n+then you can ask that to be pulled with\n+\n+\tgit request-pull v1.0 https://git.ko.xz/project master:for-linus\n+\n \n GIT\n ---\n-- \n1.9.0-293-gd838d6f\n"},{"id":"236647","messageId":"CAPig+cQTRFgbbyXsWraNTbL+nOY2y09KEJPfGc3wynFA+RWswQ@mail.gmail.com","threadId":"35715","inReplyTo":"xmqqvbvjmgys.fsf@gitster.dls.corp.google.com","subject":"Re: Re* [RFC PATCH 2/1] Make request-pull able to take a refspec of form local:remote","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-12T23:18:44Z","receivedAt":"2014-03-12T23:18:44Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 12, 2014 at 2:04 PM, Junio C Hamano wrote:\n> Subject: [PATCH] request-pull: documentation updates\n>\n> The original description talked only about what it does. Instead,\n> start it with the purpose of the command, i.e. what it is used for,\n> and then mention what it does to achieve that goal.\n>\n> Clarify what , and means in the context of the\n> overall purpose of the command.\n>\n> Describe the extended syntax of parameter that is used when\n> the local branch name is different from the branch name at the\n> repository the changes are published.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n> Documentation/git-request-pull.txt | 55 +++++++++++++++++++++++++++++++++-----\n> 1 file changed, 49 insertions(+), 6 deletions(-)\n>\n> diff --git a/Documentation/git-request-pull.txt b/Documentation/git-request-pull.txt\n> index b99681c..57bc1f5 100644\n> --- a/Documentation/git-request-pull.txt\n> +++ b/Documentation/git-request-pull.txt\n> @@ -13,22 +13,65 @@ SYNOPSIS\n> DESCRIPTION\n> -----------\n>\n> -Summarizes the changes between two commits to the standard output, and includes\n> -the given URL in the generated summary.\n> +Prepare a request to your upstream project to pull your changes to\n> +their tree to the standard output, by summarizing your changes and\n> +showing where your changes can be pulled from.\n\nPerhaps splitting this into two sentence (and using fewer to's) would\nmake it a bit easier to grok? Something like:\n\n Generate a request asking your upstream project to pull changes\n into their tree. The request, printed to standard output,\n summarizes the changes and indicates from where they can be\n pulled.\n\n> +The upstream project is expected to have the commit named by\n> +`` and the output asks it to integrate the changes you made\n> +since that commit, up to the commit named by ``, by visiting\n> +the repository named by ``.\n> +\n>\n> OPTIONS\n> -------\n> -p::\n> - Show patch text\n> + Include patch text in the output.\n>\n> ::\n> - Commit to start at.\n> + Commit to start at. This names a commit that is already in\n> + the upstream history.\n>\n> ::\n> - URL to include in the summary.\n> + The repository URL to be pulled from.\n>\n> ::\n> - Commit to end at; defaults to HEAD.\n> + Commit to end at (defaults to HEAD). This names the commit\n> + at the tip of the history you are asking to be pulled.\n> ++\n> +When the repository named by `` has the commit at a tip of a\n> +ref that is different from the ref you have it locally, you can use\n\nDid you want to drop \"it\" from this sentence? Or did you mean to say\n\"the ref as you have it locally\"?\n\n> +the `:` syntax, to have its local name, a colon `:`,\n> +and its remote name.\n> +\n> +\n> +EXAMPLE\n> +-------\n> +\n> +Imagine that you built your work on your `master` branch on top of\n> +the `v1.0` release, and want it to be integrated to the project.\n> +First you push that change to your public repository for others to\n> +see:\n> +\n> + git push https://git.ko.xz/project master\n> +\n> +Then, you run this command:\n> +\n> + git request-pull v1.0 https://git.ko.xz/project master\n> +\n> +which will produce a request to the upstream, summarizing the\n> +changes between the `v1.0` release and your `master`, to pull it\n> +from your public repository.\n> +\n> +If you pushed your change to a branch whose name is different from\n> +the one you have locally, e.g.\n> +\n> + git push https://git.ko.xz/project master:for-linus\n> +\n> +then you can ask that to be pulled with\n> +\n> + git request-pull v1.0 https://git.ko.xz/project master:for-linus\n> +\n>\n> GIT\n> ---\n> --\n> 1.9.0-293-gd838d6f\n"},{"id":"236688","messageId":"xmqqvbvhhk07.fsf@gitster.dls.corp.google.com","threadId":"35715","inReplyTo":"CAPig+cQTRFgbbyXsWraNTbL+nOY2y09KEJPfGc3wynFA+RWswQ@mail.gmail.com","subject":"Re: Re* [RFC PATCH 2/1] Make request-pull able to take a refspec of form local:remote","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-13T21:22:00Z","receivedAt":"2014-03-13T21:22:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> +Prepare a request to your upstream project to pull your changes to\n>> +their tree to the standard output, by summarizing your changes and\n>> +showing where your changes can be pulled from.\n>\n> Perhaps splitting this into two sentence (and using fewer to's) would\n> make it a bit easier to grok? Something like:\n>\n> Generate a request asking your upstream project to pull changes\n> into their tree. The request, printed to standard output,\n> summarizes the changes and indicates from where they can be\n> pulled.\n\nThanks.\n\n>> +When the repository named by `` has the commit at a tip of a\n>> +ref that is different from the ref you have it locally, you can use\n>\n> Did you want to drop \"it\" from this sentence? Or did you mean to say\n> \"the ref as you have it locally\"?\n\nThanks for your careful reading. Will drop \"it\".\n"}]}