{"thread":{"id":"44419","subject":"Bug in git rev-parse @{push}?","startedAt":"2016-11-03T14:53:51Z","lastAt":"2016-11-03T16:12:16Z","messageCount":7,"participants":["Chris Purcell","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"305349","messageId":"CAJUoZVYF=ZCBUX6jztwhJX3BZdq4JEfrSRbi4cFPwn+7yA=VjA@mail.gmail.com","threadId":"44419","inReplyTo":null,"subject":"Bug in git rev-parse @{push}?","fromName":"Chris Purcell","fromEmail":"chris.purcell.39@gmail.com","sentAt":"2016-11-03T14:53:44Z","receivedAt":"2016-11-03T14:53:51Z","isPatch":false,"sender":{"key":"chris.purcell.39@gmail.com","avatar":"https://gravatar.com/avatar/1a23b613df823fbc5bb625a2a0bb6ba750017c1dac46edd4bff65b4e5b718eeb?d=mp&s=160"},"body":"Hi folks,\n\nI think I have discovered a bug in rev-parse's handling of @{push}:\n\n$ git push\nEverything up-to-date\n$ git rev-parse @{push}\nfatal: cannot resolve 'simple' push to a single destination\n\nThe documentation for rev-parse says that \"the suffix @{push} reports\nthe branch 'where we would push to' if git push were run while\nbranchname was checked out\", so I would not expect this to error\nunless git push does.\n\nThe relevant parts of my configuration are:\n\n[push]\n    default = simple\n[remote]\n    pushdefault = origin\n[branch \"foo\"]\n    remote = origin\n    merge = refs/heads/develop\n\nThe code in branch_get_push_1 (remote.c) in the PUSH_DEFAULT_SIMPLE\ncase is calling both branch_get_upstream and tracking_for_push_dest\nand erroring if they don't return the same result, which I assume is\nincorrect for a triangular workflow?\n\nPlease let me know if I've missed out important information by\nmistake. I'm happy to work on a patch if given guidance, but this is\ndefinitely outside my comfort zone for an unfamiliar codebase\notherwise! e.g. I can't find the test suite.\n\nCheers,\nChris\n"},{"id":"305350","messageId":"20161103151422.nhrgzivapy5tisr4@sigill.intra.peff.net","threadId":"44419","inReplyTo":"CAJUoZVYF=ZCBUX6jztwhJX3BZdq4JEfrSRbi4cFPwn+7yA=VjA@mail.gmail.com","subject":"Re: Bug in git rev-parse @{push}?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-03T15:14:22Z","receivedAt":"2016-11-03T15:14:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 03, 2016 at 02:53:44PM +0000, Chris Purcell wrote:\n\n> I think I have discovered a bug in rev-parse's handling of @{push}:\n> \n> $ git push\n> Everything up-to-date\n> $ git rev-parse @{push}\n> fatal: cannot resolve 'simple' push to a single destination\n> \n> The documentation for rev-parse says that \"the suffix @{push} reports\n> the branch 'where we would push to' if git push were run while\n> branchname was checked out\", so I would not expect this to error\n> unless git push does.\n\nI'm not too surprised if there's a bug there. IIRC, the way the code is\nstructured, some of the logic had to be reimplemented for @{push} rather\nthan re-used, so there may be corner cases where they do not agree.\n\n> The relevant parts of my configuration are:\n> \n> [push]\n>     default = simple\n> [remote]\n>     pushdefault = origin\n> [branch \"foo\"]\n>     remote = origin\n>     merge = refs/heads/develop\n> \n> The code in branch_get_push_1 (remote.c) in the PUSH_DEFAULT_SIMPLE\n> case is calling both branch_get_upstream and tracking_for_push_dest\n> and erroring if they don't return the same result, which I assume is\n> incorrect for a triangular workflow?\n\nI assume you have branch \"foo\" checked out?\n\nWith this config I don't see how \"git push\" would work. Because you're\nusing \"simple\", it should complain that \"develop\" and \"foo\" are not the\nsame name.\n\nCan you give a more full reproduction recipe? If I try:\n\n  git init tmp && cd tmp\n  git config push.default simple\n  git commit -m foo --allow-empty ;# just to have some commit to push\n\n  git init --bare dst.git\n  git remote add origin dst.git\n  git push origin master:refs/heads/develop\n\n  git checkout -b foo origin/develop\n\n  # pushdefault of \"origin\" is already the default. checkout will have\n  # set up branch.foo.* as you specified. So let's try our push.\n  git push\n\nThen I get:\n\n  fatal: The upstream branch of your current branch does not match\n  the name of your current branch.  To push to the upstream branch\n  on the remote, use\n\n      git push origin HEAD:develop\n\n  To push to the branch of the same name on the remote, use\n\n      git push origin foo\n\nwhich makes sense.\n\nIf you _don't_ get that same message with \"git push\", then my next\nquestion is: might you have any aliases or other systems like \"hub\" that\nare munging the arguments to \"git push\"? Running with \"GIT_TRACE=1\" in\nthe environment might be enlightening there.\n\n> Please let me know if I've missed out important information by\n> mistake. I'm happy to work on a patch if given guidance, but this is\n> definitely outside my comfort zone for an unfamiliar codebase\n> otherwise! e.g. I can't find the test suite.\n\nThe tests are in the \"t\" directory; see t/README for details. You can\nrun them all with \"make test\" from the top-level directory.\n\n-Peff\n"},{"id":"305356","messageId":"CAJUoZVaKFdwtPRi5PGodzxHp6_NOsR0P9ToRfQnuVMDLztPHgw@mail.gmail.com","threadId":"44419","inReplyTo":"CAJUoZVbnWzpjzWe_qGZTnX_6N2+OJVG39WYaBgY7OfQBNCg+rg@mail.gmail.com","subject":"Re: Bug in git rev-parse @{push}?","fromName":"Chris Purcell","fromEmail":"chris.purcell.39@gmail.com","sentAt":"2016-11-03T15:52:42Z","receivedAt":"2016-11-03T15:52:49Z","isPatch":false,"sender":{"key":"chris.purcell.39@gmail.com","avatar":"https://gravatar.com/avatar/1a23b613df823fbc5bb625a2a0bb6ba750017c1dac46edd4bff65b4e5b718eeb?d=mp&s=160"},"body":"Resending to mailing list because Inbox is fighting with vger...\n\nOn 3 November 2016 at 15:48, Chris Purcell <chris.purcell.39@gmail.com> wrote:\n> Thanks, Jeff! If I remove the explicit configuration of remote.pushdefault =\n> \"origin\", I get the same error message as you, so I suspect that's _not_ the\n> default.\n>\n> On Thu, 3 Nov 2016 at 15:14 Jeff King <peff@peff.net> wrote:\n>>\n>> On Thu, Nov 03, 2016 at 02:53:44PM +0000, Chris Purcell wrote:\n>>\n>> > I think I have discovered a bug in rev-parse's handling of @{push}:\n>> >\n>> > $ git push\n>> > Everything up-to-date\n>> > $ git rev-parse @{push}\n>> > fatal: cannot resolve 'simple' push to a single destination\n>> >\n>> > The documentation for rev-parse says that \"the suffix @{push} reports\n>> > the branch 'where we would push to' if git push were run while\n>> > branchname was checked out\", so I would not expect this to error\n>> > unless git push does.\n>>\n>> I'm not too surprised if there's a bug there. IIRC, the way the code is\n>> structured, some of the logic had to be reimplemented for @{push} rather\n>> than re-used, so there may be corner cases where they do not agree.\n>>\n>> > The relevant parts of my configuration are:\n>> >\n>> > [push]\n>> >     default = simple\n>> > [remote]\n>> >     pushdefault = origin\n>> > [branch \"foo\"]\n>> >     remote = origin\n>> >     merge = refs/heads/develop\n>> >\n>> > The code in branch_get_push_1 (remote.c) in the PUSH_DEFAULT_SIMPLE\n>> > case is calling both branch_get_upstream and tracking_for_push_dest\n>> > and erroring if they don't return the same result, which I assume is\n>> > incorrect for a triangular workflow?\n>>\n>> I assume you have branch \"foo\" checked out?\n>>\n>> With this config I don't see how \"git push\" would work. Because you're\n>> using \"simple\", it should complain that \"develop\" and \"foo\" are not the\n>> same name.\n>>\n>> Can you give a more full reproduction recipe? If I try:\n>>\n>>   git init tmp && cd tmp\n>>   git config push.default simple\n>>   git commit -m foo --allow-empty ;# just to have some commit to push\n>>\n>>   git init --bare dst.git\n>>   git remote add origin dst.git\n>>   git push origin master:refs/heads/develop\n>>\n>>   git checkout -b foo origin/develop\n>>\n>>   # pushdefault of \"origin\" is already the default. checkout will have\n>>   # set up branch.foo.* as you specified. So let's try our push.\n>>   git push\n>>\n>> Then I get:\n>>\n>>   fatal: The upstream branch of your current branch does not match\n>>   the name of your current branch.  To push to the upstream branch\n>>   on the remote, use\n>>\n>>       git push origin HEAD:develop\n>>\n>>   To push to the branch of the same name on the remote, use\n>>\n>>       git push origin foo\n>>\n>> which makes sense.\n>>\n>> If you _don't_ get that same message with \"git push\", then my next\n>> question is: might you have any aliases or other systems like \"hub\" that\n>> are munging the arguments to \"git push\"? Running with \"GIT_TRACE=1\" in\n>> the environment might be enlightening there.\n>>\n>> > Please let me know if I've missed out important information by\n>> > mistake. I'm happy to work on a patch if given guidance, but this is\n>> > definitely outside my comfort zone for an unfamiliar codebase\n>> > otherwise! e.g. I can't find the test suite.\n>>\n>> The tests are in the \"t\" directory; see t/README for details. You can\n>> run them all with \"make test\" from the top-level directory.\n>>\n>> -Peff\n"},{"id":"305357","messageId":"20161103155400.jqzopj5i2rrlj6km@sigill.intra.peff.net","threadId":"44419","inReplyTo":"CAJUoZVbnWzpjzWe_qGZTnX_6N2+OJVG39WYaBgY7OfQBNCg+rg@mail.gmail.com","subject":"Re: Bug in git rev-parse @{push}?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-03T15:54:00Z","receivedAt":"2016-11-03T15:54:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 03, 2016 at 03:48:50PM +0000, Chris Purcell wrote:\n\n> Thanks, Jeff! If I remove the explicit configuration of remote.pushdefault\n> = \"origin\", I get the same error message as you, so I suspect that's _not_\n> the default.\n\nThat's really bizarre, because I get the same behavior with or without\nit set. Not only that, but it shouldn't even come into play, as\nbranch.foo.remote should take precedence anyway.\n\nSo now I'm really puzzled.\n\n-Peff\n"},{"id":"305359","messageId":"CAJUoZVY0-OA7Tu=cHDPH+NwmQ8MUuWc37_5aqeGQbwKpNWVzqg@mail.gmail.com","threadId":"44419","inReplyTo":"20161103155400.jqzopj5i2rrlj6km@sigill.intra.peff.net","subject":"Re: Bug in git rev-parse @{push}?","fromName":"Chris Purcell","fromEmail":"chris.purcell.39@gmail.com","sentAt":"2016-11-03T15:59:13Z","receivedAt":"2016-11-03T15:59:20Z","isPatch":false,"sender":{"key":"chris.purcell.39@gmail.com","avatar":"https://gravatar.com/avatar/1a23b613df823fbc5bb625a2a0bb6ba750017c1dac46edd4bff65b4e5b718eeb?d=mp&s=160"},"body":">> Thanks, Jeff! If I remove the explicit configuration of remote.pushdefault\n>> = \"origin\", I get the same error message as you, so I suspect that's _not_\n>> the default.\n>\n> That's really bizarre, because I get the same behavior with or without\n> it set. Not only that, but it shouldn't even come into play, as\n> branch.foo.remote should take precedence anyway.\n>\n> So now I'm really puzzled.\n\nThat's because I lied. Sorry! Too many branches configured. I've actually got\n\n[branch \"foo\"]\n    remote = .\n\nIf I change that to 'origin' like I claimed it was, it starts erroring.\n"},{"id":"305361","messageId":"20161103160716.6ygtf5iosudo2w3g@sigill.intra.peff.net","threadId":"44419","inReplyTo":"CAJUoZVY0-OA7Tu=cHDPH+NwmQ8MUuWc37_5aqeGQbwKpNWVzqg@mail.gmail.com","subject":"Re: Bug in git rev-parse @{push}?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-03T16:07:16Z","receivedAt":"2016-11-03T16:07:23Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 03, 2016 at 03:59:13PM +0000, Chris Purcell wrote:\n\n> >> Thanks, Jeff! If I remove the explicit configuration of remote.pushdefault\n> >> = \"origin\", I get the same error message as you, so I suspect that's _not_\n> >> the default.\n> >\n> > That's really bizarre, because I get the same behavior with or without\n> > it set. Not only that, but it shouldn't even come into play, as\n> > branch.foo.remote should take precedence anyway.\n> >\n> > So now I'm really puzzled.\n> \n> That's because I lied. Sorry! Too many branches configured. I've actually got\n> \n> [branch \"foo\"]\n>     remote = .\n> \n> If I change that to 'origin' like I claimed it was, it starts erroring.\n\nOK, now I can reproduce. And changing remote.pushdefault _does_ matter\nbecause builtin/push.c:is_workflow_triangular() uses it as a key to \"we\nare in a triangular workflow\".\n\nThat was added by ed2b18292 (push: change `simple` to accommodate\ntriangular workflows, 2013-06-19).  TBH, I think the right solution is\n\"stop using 'simple' in a triangular setup\". But since that workflow\nexists for git-push, we probably need to support it via @{push}, too.\n\nI think the solution would be something like:\n\n  - move is_workflow_triangular() into remote.c so it is accessible in\n    both places\n\n  - when branch_get_push_1 sees that is_workflow_triangular() is true,\n    treat \"simple\" as \"current\"\n\n  - new tests should go into t/t1514-rev-parse-push.sh to cover this\n    case\n\nWant to take a stab at it?\n\n-Peff\n"},{"id":"305362","messageId":"CAJUoZVbejQ2JE8GOKoyVqEEcLcH=5Jn8vAK83MAQosgPP7NsBQ@mail.gmail.com","threadId":"44419","inReplyTo":"20161103160716.6ygtf5iosudo2w3g@sigill.intra.peff.net","subject":"Re: Bug in git rev-parse @{push}?","fromName":"Chris Purcell","fromEmail":"chris.purcell.39@gmail.com","sentAt":"2016-11-03T16:09:56Z","receivedAt":"2016-11-03T16:12:16Z","isPatch":false,"sender":{"key":"chris.purcell.39@gmail.com","avatar":"https://gravatar.com/avatar/1a23b613df823fbc5bb625a2a0bb6ba750017c1dac46edd4bff65b4e5b718eeb?d=mp&s=160"},"body":"Will do, thanks! Meanwhile, I'll work around locally by changing off\n'simple' in my config—if I can figure out how not to break everything\nin the process...\n\nOn 3 November 2016 at 16:07, Jeff King <peff@peff.net> wrote:\n> On Thu, Nov 03, 2016 at 03:59:13PM +0000, Chris Purcell wrote:\n>\n>> >> Thanks, Jeff! If I remove the explicit configuration of remote.pushdefault\n>> >> = \"origin\", I get the same error message as you, so I suspect that's _not_\n>> >> the default.\n>> >\n>> > That's really bizarre, because I get the same behavior with or without\n>> > it set. Not only that, but it shouldn't even come into play, as\n>> > branch.foo.remote should take precedence anyway.\n>> >\n>> > So now I'm really puzzled.\n>>\n>> That's because I lied. Sorry! Too many branches configured. I've actually got\n>>\n>> [branch \"foo\"]\n>>     remote = .\n>>\n>> If I change that to 'origin' like I claimed it was, it starts erroring.\n>\n> OK, now I can reproduce. And changing remote.pushdefault _does_ matter\n> because builtin/push.c:is_workflow_triangular() uses it as a key to \"we\n> are in a triangular workflow\".\n>\n> That was added by ed2b18292 (push: change `simple` to accommodate\n> triangular workflows, 2013-06-19).  TBH, I think the right solution is\n> \"stop using 'simple' in a triangular setup\". But since that workflow\n> exists for git-push, we probably need to support it via @{push}, too.\n>\n> I think the solution would be something like:\n>\n>   - move is_workflow_triangular() into remote.c so it is accessible in\n>     both places\n>\n>   - when branch_get_push_1 sees that is_workflow_triangular() is true,\n>     treat \"simple\" as \"current\"\n>\n>   - new tests should go into t/t1514-rev-parse-push.sh to cover this\n>     case\n>\n> Want to take a stab at it?\n>\n> -Peff\n"}]}