{"thread":{"id":"36997","subject":"BUG: git request-pull broken for plain branches","startedAt":"2014-06-25T09:55:35Z","lastAt":"2014-06-26T07:06:12Z","messageCount":5,"participants":["Uwe Kleine-König","Linus Torvalds","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"244962","messageId":"20140625095535.GA27365@pengutronix.de","threadId":"36997","inReplyTo":null,"subject":"BUG: git request-pull broken for plain branches","fromName":"Uwe Kleine-König","fromEmail":"u.kleine-koenig@pengutronix.de","sentAt":"2014-06-25T09:55:35Z","receivedAt":"2014-06-25T09:55:35Z","isPatch":false,"sender":{"key":"u.kleine-koenig@pengutronix.de","avatar":"https://gravatar.com/avatar/354b5e3ceb2806a2f1e1e382ac29ddbdad18288654da62b61eb13583a857eee7?d=mp&s=160"},"body":"Hello,\n\nI have git from Debian's 2.0.0-2 package:\n\n\t$ git version\n\tgit version 2.0.0\n\ngit request-pull is broken for me:\n\n\t$ git rev-parse HEAD\n\t9e065e4a5a58308f1a0da4bb80b830929dfa90b3\n\t$ git ls-remote origin | grep 9e065e4a5a58308f1a0da4bb80b830929dfa90b3\n\t9e065e4a5a58308f1a0da4bb80b830929dfa90b3\trefs/heads/ukl/for-mainline\n\t$ git request-pull origin/master origin HEAD > /dev/null\n\twarn: No match for commit 9e065e4a5a58308f1a0da4bb80b830929dfa90b3 found at origin\n\twarn: Are you sure you pushed 'HEAD' there?\n\nThe same happens on 2.0.0.421.g786a89d.\n\nThe problem is in git-request-pull.sh's find_matching_ref. This code has\nmore than one problem (looking on 2.0.0.421.g786a89d):\n\n\t- find_matching_ref doesn't assign to $found if none of the if\n\t conditions in the loop match (this results in my problem);\n\t- find_matching_ref happily overwrites $found even if the\n\t previous ref was better according to the metric specified\n\t above the definition of find_matching_ref; and\n\t- the output generated uses $pretty_remote without asserting\n\t that it matches $ref. In my case this results in a branch\n\t specification of \"HEAD\" even if I fix find_matching_ref to\n\t return refs/heads/ukl/for-mainline.\n\nI tried to add this case to t/t5150-request-pull.sh, but didn't\nunderstand how after starring at it for half an hour. :-(\n\nBisection points on 024d34cb0813 (request-pull: more strictly match\nlocal/remote branches) as first bad commit. Apart from introducing the\nwarning, it also changes the branch spec from \"ukl/for-mainline\" (which\nis correct) to the name of the current branch (which is bogus). Also\n024d34cb0813 makes 5 out of 7 tests in t/t5150-request-pull.sh fail.\n\nBest regards\nUwe\n\n-- \nPengutronix e.K. | Uwe Kleine-König |\nIndustrial Linux Solutions | http://www.pengutronix.de/ |\n"},{"id":"244965","messageId":"CA+55aFwZb7tT7xb7y2XJpzcmrxB=Rf--bjz5XbBcxC-7MMm9eg@mail.gmail.com","threadId":"36997","inReplyTo":"20140625095535.GA27365@pengutronix.de","subject":"Re: BUG: git request-pull broken for plain branches","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2014-06-25T12:05:51Z","receivedAt":"2014-06-25T12:05:51Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Wed, Jun 25, 2014 at 2:55 AM, Uwe Kleine-König\n wrote:\n>\n> $ git rev-parse HEAD\n> 9e065e4a5a58308f1a0da4bb80b830929dfa90b3\n> $ git ls-remote origin | grep 9e065e4a5a58308f1a0da4bb80b830929dfa90b3\n> 9e065e4a5a58308f1a0da4bb80b830929dfa90b3 refs/heads/ukl/for-mainline\n> $ git request-pull origin/master origin HEAD > /dev/null\n> warn: No match for commit 9e065e4a5a58308f1a0da4bb80b830929dfa90b3 found at origin\n> warn: Are you sure you pushed 'HEAD' there?\n\nNotice how \"HEAD\" does not match.\n\nThe error message is perhaps misleading. It's not enough to match the\ncommit. You need to match the branch name too. git used to guess the\nbranch name (from the commit), and it often guessed wrongly. So now\nthey need to match.\n\nSo you should do\n\n git request-pull origin/master origin ukl/for-mainline\n\nto let request-pull know that you're requesting a pull for \"ukl/for-mainline\".\n\nIf you have another name for that branch locally (ie you did something\nlike \"git push origin local:remote\"), then you can say\n\n git request-pull origin/master origin local-name:remote-name\n\nto specify what the branch to be pulled is called locally vs remotely.\n\nIn other words, what used to be \"pick some branch randomly\" is now\n\"you need to _specify_ the branch\".\n\n Linus\n"},{"id":"244967","messageId":"20140625132131.GO14781@pengutronix.de","threadId":"36997","inReplyTo":"CA+55aFwZb7tT7xb7y2XJpzcmrxB=Rf--bjz5XbBcxC-7MMm9eg@mail.gmail.com","subject":"Re: BUG: git request-pull broken for plain branches","fromName":"Uwe Kleine-König","fromEmail":"u.kleine-koenig@pengutronix.de","sentAt":"2014-06-25T13:21:31Z","receivedAt":"2014-06-25T13:21:31Z","isPatch":false,"sender":{"key":"u.kleine-koenig@pengutronix.de","avatar":"https://gravatar.com/avatar/354b5e3ceb2806a2f1e1e382ac29ddbdad18288654da62b61eb13583a857eee7?d=mp&s=160"},"body":"Hello Linus,\n\nOn Wed, Jun 25, 2014 at 05:05:51AM -0700, Linus Torvalds wrote:\n> On Wed, Jun 25, 2014 at 2:55 AM, Uwe Kleine-König\n> wrote:\n> >\n> > $ git rev-parse HEAD\n> > 9e065e4a5a58308f1a0da4bb80b830929dfa90b3\n> > $ git ls-remote origin | grep 9e065e4a5a58308f1a0da4bb80b830929dfa90b3\n> > 9e065e4a5a58308f1a0da4bb80b830929dfa90b3 refs/heads/ukl/for-mainline\n> > $ git request-pull origin/master origin HEAD > /dev/null\n> > warn: No match for commit 9e065e4a5a58308f1a0da4bb80b830929dfa90b3 found at origin\n> > warn: Are you sure you pushed 'HEAD' there?\n> \n> Notice how \"HEAD\" does not match.\n> \n> The error message is perhaps misleading. It's not enough to match the\n> commit. You need to match the branch name too. git used to guess the\n> branch name (from the commit), and it often guessed wrongly. So now\n> they need to match.\n> \n> So you should do\n> \n> git request-pull origin/master origin ukl/for-mainline\n> \n> to let request-pull know that you're requesting a pull for \"ukl/for-mainline\".\n> \n> If you have another name for that branch locally (ie you did something\n> like \"git push origin local:remote\"), then you can say\n> \n> git request-pull origin/master origin local-name:remote-name\n> \n> to specify what the branch to be pulled is called locally vs remotely.\n> \n> In other words, what used to be \"pick some branch randomly\" is now\n> \"you need to _specify_ the branch\".\nah, got it. Still some of my concerns stay valid and I also have some\nnew ones:\n\n - if there is a branch and a tag on the remote side that match what I\n specified the outcome depends on the order of git-ls-remote. (minor\n nit.)\n - if I have to specify the remote name now, why do I have to also\n specify my local ref? Isn't the respective $sha1 of the remote side\n enough to do what is needed?\n - Isn't $found = $sha1; silly because I cannot pull a rev, only a ref?\n (side note:\n\n \tgit pull linus d91d66e88ea95b6dd21958834414009614385153\n\n gives no error message, only returns 1 and does nothing else.)\n - Is the result of\n\n \tgit request-pull $somecommit origin\n\n what is intended? For me it does\n\n \t...\n\tare available in the git repository at:\n\n\t $repository\n\n\tfor you to fetch changes ...\n\n if the remote HEAD matches the local one. I'd prefer to have an\n explicit branch name there, or at least HEAD.\n\nI liked git guessing the branch name, maybe we can teach it to guess a\nbit better than it did before 2.0? Something like:\n\n - if there is a unique match on the remote side, use it.\n - if there are >= 1 match on the remote side and exactly one matches\n what I specified as , use it.\n - if there are >= 1 match on the remote side and exactly one of them is\n a tag, use the tag\n - if there are two matches on the remote side, and one is \"HEAD\",\n pick the other one.\n\nBest regards\nUwe\n\n-- \nPengutronix e.K. | Uwe Kleine-König |\nIndustrial Linux Solutions | http://www.pengutronix.de/ |\n"},{"id":"244980","messageId":"xmqqoaxg90y4.fsf@gitster.dls.corp.google.com","threadId":"36997","inReplyTo":"20140625132131.GO14781@pengutronix.de","subject":"Re: BUG: git request-pull broken for plain branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-25T20:41:23Z","receivedAt":"2014-06-25T20:41:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Uwe Kleine-König writes:\n\n> Hello Linus,\n>\n> On Wed, Jun 25, 2014 at 05:05:51AM -0700, Linus Torvalds wrote:\n>> On Wed, Jun 25, 2014 at 2:55 AM, Uwe Kleine-König\n>> wrote:\n>> >\n>> > $ git rev-parse HEAD\n>> > 9e065e4a5a58308f1a0da4bb80b830929dfa90b3\n>> > $ git ls-remote origin | grep 9e065e4a5a58308f1a0da4bb80b830929dfa90b3\n>> > 9e065e4a5a58308f1a0da4bb80b830929dfa90b3 refs/heads/ukl/for-mainline\n>> > $ git request-pull origin/master origin HEAD > /dev/null\n>> > warn: No match for commit 9e065e4a5a58308f1a0da4bb80b830929dfa90b3 found at origin\n>> > warn: Are you sure you pushed 'HEAD' there?\n>> \n>> Notice how \"HEAD\" does not match.\n>> \n>> The error message is perhaps misleading. It's not enough to match the\n>> commit. You need to match the branch name too. git used to guess the\n>> branch name (from the commit), and it often guessed wrongly. So now\n>> they need to match.\n>> \n>> So you should do\n>> \n>> git request-pull origin/master origin ukl/for-mainline\n>> \n>> to let request-pull know that you're requesting a pull for \"ukl/for-mainline\".\n\nOr\n\n\tgit request-pull origin/master origin HEAD:ukl/for-mainline\n\n\nI am not Linus, and do not speak for him, but FWIW here are some\ncomments on the ideas.\n\n> I liked git guessing the branch name, maybe we can teach it to guess a\n> bit better than it did before 2.0? Something like:\n>\n> - if there is a unique match on the remote side, use it.\n\nI am on the fence but slightly leaning to the negative side on this\none. The branch/ref the object was took from when \"git pull\" is run\ndoes matter, because the name is recorded in the merge summary, so\nwe cannot say \"There are refs that happen to point at the object you\nwanted to be pulled, so we'll pick one of them let the integrator\npull from that ref we randomly chose\" is not something we would\nwant. \"If there is a unique one, that must be the one the user has\nmeant; there is nothing else possible\" feels like a strong argument,\nand I was actually contemplating about doing an enhancement on top\nof Linus's original myself along that line, back when we queued that\npatch exactly for that reason.\n\nBut a counter-argument, in the context of Linus's change in question\nbeing primarily to avoid end-user mistakes resulting in a bogus\nrequest, is that the ref on the remote that happens to match the\nobject but is different from what the user named may be a totally\nunrelated branch before the user actually has pushed the set of\nchanges the request is going to ask to be pulled. The mistake that\nthis extra strictness tries to avoid is to compose request-pull\nbefore pushing what to be pulled and then forgetting to push.\n\n> - if there are >= 1 match on the remote side and exactly one matches\n> what I specified as , use it.\n\nThe original change by Linus being about avoiding mistakes by\nrequiring the user to name what to be pulled, i.e. , this point\nof \"other refs also happen to point at the same object\" is made\nirrelevant---if does have the object the user named to be\npulled, that should be used regardless of where other refs point at.\n\n> - if there are >= 1 match on the remote side and exactly one of them is\n> a tag, use the tag\n>\n> - if there are two matches on the remote side, and one is \"HEAD\",\n> pick the other one.\n\nAssuming that does not match the object in these two cases\n(otherwise your second condition would have caught it), they share\nthe same potential objection as the first one.\n\nI dunno.\n"},{"id":"245006","messageId":"20140626070612.GR14781@pengutronix.de","threadId":"36997","inReplyTo":"xmqqoaxg90y4.fsf@gitster.dls.corp.google.com","subject":"Re: BUG: git request-pull broken for plain branches","fromName":"Uwe Kleine-König","fromEmail":"u.kleine-koenig@pengutronix.de","sentAt":"2014-06-26T07:06:12Z","receivedAt":"2014-06-26T07:06:12Z","isPatch":false,"sender":{"key":"u.kleine-koenig@pengutronix.de","avatar":"https://gravatar.com/avatar/354b5e3ceb2806a2f1e1e382ac29ddbdad18288654da62b61eb13583a857eee7?d=mp&s=160"},"body":"Hi Junio,\n\nOn Wed, Jun 25, 2014 at 01:41:23PM -0700, Junio C Hamano wrote:\n> Uwe Kleine-König writes:\n> > On Wed, Jun 25, 2014 at 05:05:51AM -0700, Linus Torvalds wrote:\n> >> On Wed, Jun 25, 2014 at 2:55 AM, Uwe Kleine-König\n> >> wrote:\n> >> >\n> >> > $ git rev-parse HEAD\n> >> > 9e065e4a5a58308f1a0da4bb80b830929dfa90b3\n> >> > $ git ls-remote origin | grep 9e065e4a5a58308f1a0da4bb80b830929dfa90b3\n> >> > 9e065e4a5a58308f1a0da4bb80b830929dfa90b3 refs/heads/ukl/for-mainline\n> >> > $ git request-pull origin/master origin HEAD > /dev/null\n> >> > warn: No match for commit 9e065e4a5a58308f1a0da4bb80b830929dfa90b3 found at origin\n> >> > warn: Are you sure you pushed 'HEAD' there?\n> >> \n> >> Notice how \"HEAD\" does not match.\n> >> \n> >> The error message is perhaps misleading. It's not enough to match the\n> >> commit. You need to match the branch name too. git used to guess the\n> >> branch name (from the commit), and it often guessed wrongly. So now\n> >> they need to match.\n> >> \n> >> So you should do\n> >> \n> >> git request-pull origin/master origin ukl/for-mainline\n> >> \n> >> to let request-pull know that you're requesting a pull for \"ukl/for-mainline\".\n> \n> Or\n> \n> \tgit request-pull origin/master origin HEAD:ukl/for-mainline\n> \n> I am not Linus, and do not speak for him, but FWIW here are some\n> comments on the ideas.\n> \n> > I liked git guessing the branch name, maybe we can teach it to guess a\n> > bit better than it did before 2.0? Something like:\n> >\n> > - if there is a unique match on the remote side, use it.\n> \n> I am on the fence but slightly leaning to the negative side on this\n> one. The branch/ref the object was took from when \"git pull\" is run\n> does matter, because the name is recorded in the merge summary, so\n> we cannot say \"There are refs that happen to point at the object you\n> wanted to be pulled, so we'll pick one of them let the integrator\n> pull from that ref we randomly chose\" is not something we would\n> want. \"If there is a unique one, that must be the one the user has\n> meant; there is nothing else possible\" feels like a strong argument,\n> and I was actually contemplating about doing an enhancement on top\n> of Linus's original myself along that line, back when we queued that\n> patch exactly for that reason.\n> \n> But a counter-argument, in the context of Linus's change in question\n> being primarily to avoid end-user mistakes resulting in a bogus\n> request, is that the ref on the remote that happens to match the\n> object but is different from what the user named may be a totally\n> unrelated branch before the user actually has pushed the set of\n> changes the request is going to ask to be pulled. The mistake that\n> this extra strictness tries to avoid is to compose request-pull\n> before pushing what to be pulled and then forgetting to push.\nSounds sensible. Then the enhancements that come to my mind are:\n\nChange git-request-pull to explicitly take a remote ref as end. This\nmakes sure that it is actually there and the right remote name is\npicked. Don't require to specify a local ref even if there is no\nlocal matching ref, just use the remote sha1 to generate the diffstat\nand summary.\n\nAnother thing I'd like to have is to make git-request-pull not generate\nthe usual output if there is no match. Optionally introduce a -f to get\nback the (maybe bogus) output; in this case a local rev would be needed.\n\nBest regards\nUwe\n\n-- \nPengutronix e.K. | Uwe Kleine-König |\nIndustrial Linux Solutions | http://www.pengutronix.de/ |\n"}]}