{"thread":{"id":"53853","subject":"[RFC] should `git rebase --keep-base` imply `--reapply-cherry-picks` ?","startedAt":"2020-07-14T02:44:12Z","lastAt":"2020-07-15T08:53:25Z","messageCount":8,"participants":["Philippe Blain","Denton Liu","Jonathan Tan","Phillip Wood","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"401528","messageId":"0EA8C067-5805-40A7-857A-55C2633B8570@gmail.com","threadId":"53853","inReplyTo":null,"subject":"[RFC] should `git rebase --keep-base` imply `--reapply-cherry-picks` ?","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2020-07-14T02:44:06Z","receivedAt":"2020-07-14T02:44:12Z","isPatch":false,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hello,\n\nI learned today that doing `git rebase --keep-base master` \nwill drop commits that were cherry-picked from master to the current branch. \nI was simply doing a code clean up on my feature branch (the full command was\n`git rebase -i --keep-base master`), and this kind of confused me for a moment.\n\nIs this a sane default ? I understand that it is a good default when we are rebasing \n*on top* of master, but here I'm just doing some squashing and fixup's and I did not\nwant the commit I had cherry-picked from master to disappear (yet). In fact, because it\nwas dropped, it created a modify/delete conflict because in a subsequent commit \nin my feature branch I'm modifying files that are added in the commit I cherry-picked.\n\nHow would a change that made '--reapply-cherry-picks' be the default when using 'keep-base'\nbe received ?\n\nTangential question: in any case, would it make sense to still add the \"dropped because \nalready upstream\" commits to the todo list, in the case of an interactive rebase ? \n(maybe commented out, or listed as 'drop' with some kind of comment saying those \nare dropped because they appear textually upstream?)\n\nCheers,\nPhilippe.\nP.S. I CC'd those who were involved with the 'keep-base' patch or the 'reapply-cherry-picks' patch."},{"id":"401529","messageId":"20200714031017.GA15143@generichostname","threadId":"53853","inReplyTo":"0EA8C067-5805-40A7-857A-55C2633B8570@gmail.com","subject":"Re: [RFC] should `git rebase --keep-base` imply `--reapply-cherry-picks` ?","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-07-14T03:10:17Z","receivedAt":"2020-07-14T03:10:22Z","isPatch":false,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Philippe,\n\nOn Mon, Jul 13, 2020 at 10:44:06PM -0400, Philippe Blain wrote:\n> Hello,\n> \n> I learned today that doing `git rebase --keep-base master` \n> will drop commits that were cherry-picked from master to the current branch. \n> I was simply doing a code clean up on my feature branch (the full command was\n> `git rebase -i --keep-base master`), and this kind of confused me for a moment.\n\nGlad I'm not the only one using this feature :)\n\n> Is this a sane default ? I understand that it is a good default when we are rebasing \n> *on top* of master, but here I'm just doing some squashing and fixup's and I did not\n> want the commit I had cherry-picked from master to disappear (yet). In fact, because it\n> was dropped, it created a modify/delete conflict because in a subsequent commit \n> in my feature branch I'm modifying files that are added in the commit I cherry-picked.\n\nSo if I'm not mistaken, if we have the following graph\n\n\tA - B - C - D (master)\n\t     \\\n\t       - C' - D (feature)\n\nand we do `git rebase --keep-base master` from feature, C' will be\ndropped? Indeed, I am surprised by how this interacts with the\ndefault setting of --reapply-cherry-picks.\n\n> How would a change that made '--reapply-cherry-picks' be the default when using 'keep-base'\n> be received ?\n\nI'm somewhat surprised that --no-reapply-cherry-picks is the default. I\nwould argue that it _shouldn't_ be the default at all. It's an\noptimisation for when no --onto or --keep-base are specified but it\ndefinitely can cause problems otherwise, as we've seen.\n\nI think I would argue for the following in decreasing order of\npreference:\n\n\t1. Make --no-reapply-cherry-picks the default in all cases.\n\t   (Those who need the optimisation can enable it manually and\n\t   we can add a configuration option for it.)\n\n\t2. Make --no-reapply-cherry-picks only active if no --onto or\n\t   --keep-base are given (--keep-base is a special case of --onto\n\t   so we only have to handle it in one place).\n\n> Tangential question: in any case, would it make sense to still add the \"dropped because \n> already upstream\" commits to the todo list, in the case of an interactive rebase ? \n> (maybe commented out, or listed as 'drop' with some kind of comment saying those \n> are dropped because they appear textually upstream?)\n\nThat would make sense to me. I don't have a preference between either.\n\nThanks,\n\nDenton\n\n> Cheers,\n> Philippe.\n> P.S. I CC'd those who were involved with the 'keep-base' patch or the 'reapply-cherry-picks' patch.\n"},{"id":"401530","messageId":"20200714035104.1465772-1-jonathantanmy@google.com","threadId":"53853","inReplyTo":"20200714031017.GA15143@generichostname","subject":"Re: [RFC] should `git rebase --keep-base` imply `--reapply-cherry-picks` ?","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-07-14T03:51:04Z","receivedAt":"2020-07-14T03:51:12Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > Is this a sane default ? I understand that it is a good default when we are rebasing \n> > *on top* of master, but here I'm just doing some squashing and fixup's and I did not\n> > want the commit I had cherry-picked from master to disappear (yet). In fact, because it\n> > was dropped, it created a modify/delete conflict because in a subsequent commit \n> > in my feature branch I'm modifying files that are added in the commit I cherry-picked.\n> \n> So if I'm not mistaken, if we have the following graph\n> \n> \tA - B - C - D (master)\n> \t     \\\n> \t       - C' - D (feature)\n> \n> and we do `git rebase --keep-base master` from feature, C' will be\n> dropped? Indeed, I am surprised by how this interacts with the\n> default setting of --reapply-cherry-picks.\n\nJust to clarify, the default setting is --no-reapply-cherry-picks. (I\nunderstand that the naming is confusing - the name was discussed quite a\nbit [1].)\n\n[1] https://lore.kernel.org/git/20200309205523.121319-1-jonathantanmy@google.com/\n\n> > How would a change that made '--reapply-cherry-picks' be the default when using 'keep-base'\n> > be received ?\n> \n> I'm somewhat surprised that --no-reapply-cherry-picks is the default. I\n> would argue that it _shouldn't_ be the default at all. It's an\n> optimisation for when no --onto or --keep-base are specified but it\n> definitely can cause problems otherwise, as we've seen.\n\nWhen I encountered this feature, it was a surprise to me too, but this\nhas been documented as a feature for a long time (e.g. see the man page\nfor 2.1.4 from 2014 [2] - search for \"RECOVERING FROM UPSTREAM REBASE\").\n\n[2] https://git-scm.com/docs/git-rebase/2.1.4\n\n> I think I would argue for the following in decreasing order of\n> preference:\n> \n> \t1. Make --no-reapply-cherry-picks the default in all cases.\n> \t   (Those who need the optimisation can enable it manually and\n> \t   we can add a configuration option for it.)\n\nI assume you mean \"make --reapply-cherry-picks\" the default (since\nearlier you said that you were surprised that --no-reapply-cherry-picks\nis the default)? Also what do you mean by \"optimization\"?\n--reapply-cherry-picks is faster because it does not need to read the\nupstream commits (optimization for speed) but --no-reapply-cherry-picks\nfits some people's workflow better (optimization for workflow).\n\n> \t2. Make --no-reapply-cherry-picks only active if no --onto or\n> \t   --keep-base are given (--keep-base is a special case of --onto\n> \t   so we only have to handle it in one place).\n\nThis might be confusing - I wouldn't expect \"git rebase master\" and \"git\nrebase master --onto master\" to have different behavior.\n\n> > Tangential question: in any case, would it make sense to still add the \"dropped because \n> > already upstream\" commits to the todo list, in the case of an interactive rebase ? \n> > (maybe commented out, or listed as 'drop' with some kind of comment saying those \n> > are dropped because they appear textually upstream?)\n> \n> That would make sense to me. I don't have a preference between either.\n\nThis makes sense to me too.\n"},{"id":"401543","messageId":"9c6dff59-b204-1ace-e0aa-0885dd502214@gmail.com","threadId":"53853","inReplyTo":"20200714031017.GA15143@generichostname","subject":"Re: [RFC] should `git rebase --keep-base` imply `--reapply-cherry-picks` ?","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-07-14T09:52:05Z","receivedAt":"2020-07-14T09:52:11Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Denton\n\nOn 14/07/2020 04:10, Denton Liu wrote:\n> Hi Philippe,\n> \n> On Mon, Jul 13, 2020 at 10:44:06PM -0400, Philippe Blain wrote:\n>> Hello,\n>>\n>> I learned today that doing `git rebase --keep-base master`\n>> will drop commits that were cherry-picked from master to the current branch.\n>> I was simply doing a code clean up on my feature branch (the full command was\n>> `git rebase -i --keep-base master`), and this kind of confused me for a moment.\n> \n> Glad I'm not the only one using this feature :)\n> \n>> Is this a sane default ? I understand that it is a good default when we are rebasing\n>> *on top* of master, but here I'm just doing some squashing and fixup's and I did not\n>> want the commit I had cherry-picked from master to disappear (yet). In fact, because it\n>> was dropped, it created a modify/delete conflict because in a subsequent commit\n>> in my feature branch I'm modifying files that are added in the commit I cherry-picked.\n> \n> So if I'm not mistaken, if we have the following graph\n> \n> \tA - B - C - D (master)\n> \t     \\\n> \t       - C' - D (feature)\n> \n> and we do `git rebase --keep-base master` from feature, C' will be\n> dropped? Indeed, I am surprised by how this interacts with the\n> default setting of --reapply-cherry-picks.\n\nTo me the question is why are we looking at the upstream commits at all \nwith `--keep-base`? I had expected `rebase --keep-base` to be the same \nas `rebase $(git merge-base [--fork-point] @{upstream} HEAD)` but \nlooking at the code it seems to be `rebase --onto $(git merge-base \n@{upstream} HEAD) @{upstream}`. I didn't really follow the development \nof this feature - is there a reason we don't just use the merge-base as \nthe upstream commit?\n\nBest Wishes\n\nPhillip\n\n> \n>> How would a change that made '--reapply-cherry-picks' be the default when using 'keep-base'\n>> be received ?\n> \n> I'm somewhat surprised that --no-reapply-cherry-picks is the default. I\n> would argue that it _shouldn't_ be the default at all. It's an\n> optimisation for when no --onto or --keep-base are specified but it\n> definitely can cause problems otherwise, as we've seen.\n> \n> I think I would argue for the following in decreasing order of\n> preference:\n> \n> \t1. Make --no-reapply-cherry-picks the default in all cases.\n> \t   (Those who need the optimisation can enable it manually and\n> \t   we can add a configuration option for it.)\n> \n> \t2. Make --no-reapply-cherry-picks only active if no --onto or\n> \t   --keep-base are given (--keep-base is a special case of --onto\n> \t   so we only have to handle it in one place).\n> \n>> Tangential question: in any case, would it make sense to still add the \"dropped because\n>> already upstream\" commits to the todo list, in the case of an interactive rebase ?\n>> (maybe commented out, or listed as 'drop' with some kind of comment saying those\n>> are dropped because they appear textually upstream?)\n> \n> That would make sense to me. I don't have a preference between either.\n> \n> Thanks,\n> \n> Denton\n> \n>> Cheers,\n>> Philippe.\n>> P.S. I CC'd those who were involved with the 'keep-base' patch or the 'reapply-cherry-picks' patch.\n"},{"id":"401563","messageId":"nycvar.QRO.7.76.6.2007142236060.52@tvgsbejvaqbjf.bet","threadId":"53853","inReplyTo":"9c6dff59-b204-1ace-e0aa-0885dd502214@gmail.com","subject":"Re: [RFC] should `git rebase --keep-base` imply `--reapply-cherry-picks` ?","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-07-14T20:38:23Z","receivedAt":"2020-07-14T20:38:31Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 14 Jul 2020, Phillip Wood wrote:\n\n> On 14/07/2020 04:10, Denton Liu wrote:\n> >\n> > On Mon, Jul 13, 2020 at 10:44:06PM -0400, Philippe Blain wrote:\n> > >\n> > > I learned today that doing `git rebase --keep-base master` will drop\n> > > commits that were cherry-picked from master to the current branch. I\n> > > was simply doing a code clean up on my feature branch (the full\n> > > command was `git rebase -i --keep-base master`), and this kind of\n> > > confused me for a moment.\n> >\n> > Glad I'm not the only one using this feature :)\n> >\n> > > Is this a sane default ? I understand that it is a good default when\n> > > we are rebasing *on top* of master, but here I'm just doing some\n> > > squashing and fixup's and I did not want the commit I had\n> > > cherry-picked from master to disappear (yet). In fact, because it\n> > > was dropped, it created a modify/delete conflict because in a\n> > > subsequent commit in my feature branch I'm modifying files that are\n> > > added in the commit I cherry-picked.\n> >\n> > So if I'm not mistaken, if we have the following graph\n> >\n> >  A - B - C - D (master)\n> >       \\\n> >         - C' - D (feature)\n> >\n> > and we do `git rebase --keep-base master` from feature, C' will be\n> > dropped? Indeed, I am surprised by how this interacts with the\n> > default setting of --reapply-cherry-picks.\n>\n> To me the question is why are we looking at the upstream commits at all\n> with `--keep-base`? I had expected `rebase --keep-base` to be the same\n> as `rebase $(git merge-base [--fork-point] @{upstream} HEAD)` but\n> looking at the code it seems to be `rebase --onto $(git merge-base\n> @{upstream} HEAD) @{upstream}`. I didn't really follow the development\n> of this feature - is there a reason we don't just use the merge-base as\n> the upstream commit?\n\nThose are interesting questions, indeed.\n\nAnd I dare to suspect that the answer is indeed: `--keep-base` really\nshould not only substitute `onto` but also `upstream` with the merge base.\n\nCiao,\nDscho\n"},{"id":"401572","messageId":"20200715032014.GA10818@generichostname","threadId":"53853","inReplyTo":"nycvar.QRO.7.76.6.2007142236060.52@tvgsbejvaqbjf.bet","subject":"Re: [RFC] should `git rebase --keep-base` imply `--reapply-cherry-picks` ?","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-07-15T03:20:14Z","receivedAt":"2020-07-15T03:20:21Z","isPatch":false,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi all,\n\nOn Tue, Jul 14, 2020 at 10:38:23PM +0200, Johannes Schindelin wrote:\n> > To me the question is why are we looking at the upstream commits at all\n> > with `--keep-base`? I had expected `rebase --keep-base` to be the same\n> > as `rebase $(git merge-base [--fork-point] @{upstream} HEAD)` but\n> > looking at the code it seems to be `rebase --onto $(git merge-base\n> > @{upstream} HEAD) @{upstream}`. I didn't really follow the development\n> > of this feature - is there a reason we don't just use the merge-base as\n> > the upstream commit?\n\nIt behaves this way mostly for unimportant reasons. The first is that my\nworkflow before implementing this feature invoked running\n`git rebase --onto master... master` and I wanted to replicate that.\n\nMore importantly, one feature of using the upstream I considered is\ndocumented in t3431. Essentially, if we have the following graph,\n\n\tA---B---D---E    (master)\n\t     \\\n\t      C*---F---G (side)\n\t\n\tC was formerly part of master but master was rewound to remove C\n\nrunning `git rebase --keep-base --fork-point master` would drop C.\n\n> Those are interesting questions, indeed.\n> \n> And I dare to suspect that the answer is indeed: `--keep-base` really\n> should not only substitute `onto` but also `upstream` with the merge base.\n\nI would be open to changing the behaviour since the commit dropping\nisn't really a feature that I use very often. However, I am worried\nabout pulling the rug out from other people if they use it since this is\na documented feature in git-rebase.txt.\n\nThanks,\nDenton\n\n> Ciao,\n> Dscho\n"},{"id":"401573","messageId":"20200715033216.GA11770@generichostname","threadId":"53853","inReplyTo":"20200714035104.1465772-1-jonathantanmy@google.com","subject":"Re: [RFC] should `git rebase --keep-base` imply `--reapply-cherry-picks` ?","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-07-15T03:32:16Z","receivedAt":"2020-07-15T03:32:21Z","isPatch":false,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Mon, Jul 13, 2020 at 08:51:04PM -0700, Jonathan Tan wrote:\n> > > How would a change that made '--reapply-cherry-picks' be the default when using 'keep-base'\n> > > be received ?\n> > \n> > I'm somewhat surprised that --no-reapply-cherry-picks is the default. I\n> > would argue that it _shouldn't_ be the default at all. It's an\n> > optimisation for when no --onto or --keep-base are specified but it\n> > definitely can cause problems otherwise, as we've seen.\n> \n> When I encountered this feature, it was a surprise to me too, but this\n> has been documented as a feature for a long time (e.g. see the man page\n> for 2.1.4 from 2014 [2] - search for \"RECOVERING FROM UPSTREAM REBASE\").\n> \n> [2] https://git-scm.com/docs/git-rebase/2.1.4\n\nAh, I was mistaken. Thanks for the correction.\n"},{"id":"401578","messageId":"87889c7b-b83a-8969-e40a-9bb19b7c11ae@gmail.com","threadId":"53853","inReplyTo":"20200715032014.GA10818@generichostname","subject":"Re: [RFC] should `git rebase --keep-base` imply `--reapply-cherry-picks` ?","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-07-15T08:53:20Z","receivedAt":"2020-07-15T08:53:25Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi all\n\nOn 15/07/2020 04:20, Denton Liu wrote:\n> Hi all,\n> \n> On Tue, Jul 14, 2020 at 10:38:23PM +0200, Johannes Schindelin wrote:\n>>> To me the question is why are we looking at the upstream commits at all\n>>> with `--keep-base`? I had expected `rebase --keep-base` to be the same\n>>> as `rebase $(git merge-base [--fork-point] @{upstream} HEAD)` but\n>>> looking at the code it seems to be `rebase --onto $(git merge-base\n>>> @{upstream} HEAD) @{upstream}`. I didn't really follow the development\n>>> of this feature - is there a reason we don't just use the merge-base as\n>>> the upstream commit?\n> \n> It behaves this way mostly for unimportant reasons. The first is that my\n> workflow before implementing this feature invoked running\n> `git rebase --onto master... master` and I wanted to replicate that.\n> \n> More importantly, one feature of using the upstream I considered is\n> documented in t3431. Essentially, if we have the following graph,\n> \n> \tA---B---D---E    (master)\n> \t     \\\n> \t      C*---F---G (side)\n> \t\n> \tC was formerly part of master but master was rewound to remove C\n> \n> running `git rebase --keep-base --fork-point master` would drop C.\n> \n>> Those are interesting questions, indeed.\n>>\n>> And I dare to suspect that the answer is indeed: `--keep-base` really\n>> should not only substitute `onto` but also `upstream` with the merge base.\n> \n> I would be open to changing the behaviour since the commit dropping\n> isn't really a feature that I use very often. However, I am worried\n> about pulling the rug out from other people if they use it since this is\n> a documented feature in git-rebase.txt.\n\nI think changing it to behave like\n\n   git rebase --onto $(git merge-base @{upstream} HEAD) \\\n                     $(git merge-base --fork-point @{upstream} HEAD)\n\nwhen --fork-point is given would keep the existing behavior of dropping \ncommits when @{upstream} has been rewound without dropping cherry-picks. \n--fork-point seems less useful when combined with --keep-base than when \nused with a normal rebase as if @{upstream} has rewritten the commit \nthat the branch is based on rather than just dropping it we end up \ndropping the original commit without getting the new version from \nupstream as we would with a normal rebase. This could be surprising to \nusers if we keep --fork-point on by default with --keep-base.\n\nBest Wishes\n\nPhillip\n\n> \n> Thanks,\n> Denton\n> \n>> Ciao,\n>> Dscho\n"}]}