{"thread":{"id":"21456","subject":"BUG: git rebase -i -p silently looses commits","startedAt":"2009-11-02T16:18:07Z","lastAt":"2009-11-13T09:07:31Z","messageCount":9,"participants":["Constantine Plotnikov","demerphq","Sverre Rabbelier","Johannes Schindelin","Greg Price"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"126600","messageId":"85647ef50911020818p61d0c975kd5655fa58993e07b@mail.gmail.com","threadId":"21456","inReplyTo":null,"subject":"BUG: git rebase -i -p silently looses commits","fromName":"Constantine Plotnikov","fromEmail":"constantine.plotnikov@gmail.com","sentAt":"2009-11-02T16:18:07Z","receivedAt":"2009-11-02T16:18:07Z","isPatch":false,"sender":{"key":"constantine.plotnikov@gmail.com","avatar":null},"body":"I have encountered what looks like critical bugs in the git rebase -i\n-p (it can be reproduced on mingw and cygwin, I have not tried other\nplatforms).\n\nLet's create a git repository with\n\ngit init\n# the next line is for mingw\ngit config core.autocrlf input\necho a >a.txt\necho b >b.txt\ngit add a.txt b.txt\ngit commit -m \"init commit\"\necho aa >a.txt\ngit add a.txt\ngit commit -m \"aa commit\"\necho bb >b.txt\ngit add b.txt\ngit commit -m \"bb commit\"\necho aaa >a.txt\ngit add a.txt\ngit commit -m \"aaa commit\"\n\nNow let's use the following rebase command:\n\ngit rebase -i -p HEAD~3\n\nWhen the editor will appear, just move the commit \"bb commit\" to the\nend of the list. The rebase process will complete successfully, but\ncommit \"aaa commit\" will be missing from the history and working tree\nwill not be affected by that commit.\n\nOther bug is that if we move \"bb commit\" to the top of the list in the\neditor, the rebase process will apply \"bb commit\", but instead of\napplying \"aa commit\" and than \"aaa commit\", the rebase process fails\nwith a merge conflict.\n\nThis can be reproduced with git 1.6.5.1 (msys) and 1.6.1.2 (cygwin). I\nconsider these to be a critical bugs that make \"-p\" option extremely\ndangerous for interactive rebase. It might even make sense to disable\nit for interactive rebase until the bug is fixed.\n\nRegards,\nConstantine\n"},{"id":"126601","messageId":"9b18b3110911020833y56be8fbdoaf10b6e6259f57c8@mail.gmail.com","threadId":"21456","inReplyTo":"85647ef50911020818p61d0c975kd5655fa58993e07b@mail.gmail.com","subject":"Re: BUG: git rebase -i -p silently looses commits","fromName":"demerphq","fromEmail":"demerphq@gmail.com","sentAt":"2009-11-02T16:33:25Z","receivedAt":"2009-11-02T16:33:25Z","isPatch":false,"sender":{"key":"demerphq@gmail.com","avatar":null},"body":"2009/11/2 Constantine Plotnikov :\n> I have encountered what looks like critical bugs in the git rebase -i\n> -p (it can be reproduced on mingw and cygwin, I have not tried other\n> platforms).\n>\n> Let's create a git repository with\n>\n> git init\n> # the next line is for mingw\n> git config core.autocrlf input\n> echo a >a.txt\n> echo b >b.txt\n> git add a.txt b.txt\n> git commit -m \"init commit\"\n> echo aa >a.txt\n> git add a.txt\n> git commit -m \"aa commit\"\n> echo bb >b.txt\n> git add b.txt\n> git commit -m \"bb commit\"\n> echo aaa >a.txt\n> git add a.txt\n> git commit -m \"aaa commit\"\n>\n> Now let's use the following rebase command:\n>\n> git rebase -i -p HEAD~3\n>\n> When the editor will appear, just move the commit \"bb commit\" to the\n> end of the list. The rebase process will complete successfully, but\n> commit \"aaa commit\" will be missing from the history and working tree\n> will not be affected by that commit.\n>\n> Other bug is that if we move \"bb commit\" to the top of the list in the\n> editor, the rebase process will apply \"bb commit\", but instead of\n> applying \"aa commit\" and than \"aaa commit\", the rebase process fails\n> with a merge conflict.\n>\n> This can be reproduced with git 1.6.5.1 (msys) and 1.6.1.2 (cygwin). I\n> consider these to be a critical bugs that make \"-p\" option extremely\n> dangerous for interactive rebase. It might even make sense to disable\n> it for interactive rebase until the bug is fixed.\n\nDoesnt -p ONLY work for interactive rebase?\n\n -p, --preserve-merges\n Instead of ignoring merges, try to recreate them. This\noption only works in interactive mode.\n\nYves\n\n\n-- \nperl -Mre=debug -e \"/just|another|perl|hacker/\"\n"},{"id":"126604","messageId":"fabb9a1e0911020856o1ff79bfbn50704b7440bfbe0f@mail.gmail.com","threadId":"21456","inReplyTo":"85647ef50911020818p61d0c975kd5655fa58993e07b@mail.gmail.com","subject":"Re: BUG: git rebase -i -p silently looses commits","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2009-11-02T16:56:58Z","receivedAt":"2009-11-02T16:56:58Z","isPatch":false,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Mon, Nov 2, 2009 at 17:18, Constantine Plotnikov\n wrote:\n> I have encountered what looks like critical bugs in the git rebase -i\n> -p (it can be reproduced on mingw and cygwin, I have not tried other\n> platforms).\n\nJohannes has been working off and on on fixing git rebase -i -p to\nDTRT, perhaps this is related?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"126605","messageId":"85647ef50911020859l1d76d03emf1302aafab642438@mail.gmail.com","threadId":"21456","inReplyTo":"9b18b3110911020833y56be8fbdoaf10b6e6259f57c8@mail.gmail.com","subject":"Re: BUG: git rebase -i -p silently looses commits","fromName":"Constantine Plotnikov","fromEmail":"constantine.plotnikov@gmail.com","sentAt":"2009-11-02T16:59:27Z","receivedAt":"2009-11-02T16:59:27Z","isPatch":false,"sender":{"key":"constantine.plotnikov@gmail.com","avatar":null},"body":"On Mon, Nov 2, 2009 at 7:33 PM, demerphq wrote:\n> 2009/11/2 Constantine Plotnikov :\n\n> Doesnt -p ONLY work for interactive rebase?\n>\n>       -p, --preserve-merges\n>           Instead of ignoring merges, try to recreate them. This\n> option only works in interactive mode.\n>\nYep, I forgot about it. But it does not seem to work correctly for\ninteractive rebase either.\n\nConstantine\n"},{"id":"126608","messageId":"alpine.DEB.1.00.0911021832530.2479@felix-maschine","threadId":"21456","inReplyTo":"85647ef50911020818p61d0c975kd5655fa58993e07b@mail.gmail.com","subject":"Re: BUG: git rebase -i -p silently looses commits","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-11-02T17:34:15Z","receivedAt":"2009-11-02T17:34:15Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 2 Nov 2009, Constantine Plotnikov wrote:\n\n> I have encountered what looks like critical bugs in the git rebase -i\n> -p (it can be reproduced on mingw and cygwin, I have not tried other\n> platforms).\n\nrebase -i -p was never intended to reorder commits. In fact, the \"-i\" of \nit was only for convenience: I was more familiar with the code base of \nrebase -i than that of rebase.\n\nHaving said that, I worked for some time on fixing this issue, and I \nactually run a version of rebase -i -p here that allows reordering \ncommits, but it is far from stable (and due to GSoC and day-job \nobligations, I had no time to work on it in months).\n\nCiao,\nDscho\n"},{"id":"126835","messageId":"20091104214611.GL9139@dr-wily.mit.edu","threadId":"21456","inReplyTo":"alpine.DEB.1.00.0911021832530.2479@felix-maschine","subject":"Re: BUG: git rebase -i -p silently looses commits","fromName":"Greg Price","fromEmail":"price@ksplice.com","sentAt":"2009-11-04T21:46:12Z","receivedAt":"2009-11-04T21:46:12Z","isPatch":false,"sender":{"key":"price@mit.edu","avatar":"https://avatars.githubusercontent.com/u/28173?v=4"},"body":"On Mon, 2 Nov 2009, Johannes Schindelin wrote:\n> Having said that, I worked for some time on fixing this issue, and I \n> actually run a version of rebase -i -p here that allows reordering \n> commits, but it is far from stable (and due to GSoC and day-job \n> obligations, I had no time to work on it in months).\n\nI'm interested in this topic too. Some weeks ago I took your\nrebase-i-p branch from January and rebased it onto the latest release;\nit's at\n git://repo.or.cz/git/price.git rebase-i-p\nand now based on v1.6.5.2. I fixed a few bugs and added a feature,\nand it's the version I run day to day.\n\nConstantine and others interested in reordering commits with -p,\nyou're welcome to pull and build this version and try it out. It\nmostly solves my problems, and maybe it will solve yours. Be warned\nit does have bugs, and also be warned that I may rewind and rebase\nthat branch. I'd be glad to hear about bugs you see, though.\n\nDscho, do you have a TODO written somewhere of what work you're aware\nthe topic still needs? I plan to continue spending a little time\nworking on it, and I have my own list but it'd be good to compare\nit with yours.\n\nCheers,\nGreg\n\n\nPS - I'm open to suggestions on the workflow for how to develop a\ntopic branch like this. Some rewinding seems necessary as half-baked\npatches get finished, etc, but if Dscho or someone else finds time to\nwork on it too, then the rewinding gets in the way of pull/push\ncollaboration.\n\n\nPPS - For those just scanning along, the shortlog so far:\n\nGreg Price (6):\n rebase -i -p: honor -s\n rebase -i -p: get full message from original merge commit\n rebase -i -p: always merge --no-ff\n rebase -i: Add the \"ref\" command\n rebase -i -p: Preserve author information on merges.\n [broken] rebase -i: implement pause\n\nJohannes Schindelin (19):\n Some of Dscho's tools\n debug settings in Makefile\n Make CFLAGS more strict\n rebase -i --root: simplify code\n rebase -i: make pick_one() safer\n rebase -i -p: add helper parse_commit() to find rewritten commits\n rebase -i: add the \"goto\" command\n rebase -i -p: add a helper to add mappings for rewritten commits\n rebase -i -p: Add the \"merge\" command\n rebase -i: make sure that the commands record the rewritten commits\n rebase -i: move the code to write the rebase script into generate_script()\n rebase -i: let generate_script output to stdout\n rebase -i -p: refactor the preparation for -p into its own function\n rebase -i -p: use patch-id directly to determine the dropped commits\n rebase tests' fake-editor.sh: allow debugging with DEBUG_EDIT\n rebase's fake-editor: prepare for \"goto\" and \"merge\" commands\n rebase -i -p: generate a script using \"goto\" and \"merge\"\n TODO\n Make some tests in t3412 a little bit stricter\n"},{"id":"127355","messageId":"alpine.DEB.1.00.0911111804520.19111@intel-tinevez-2-302","threadId":"21456","inReplyTo":"20091104214611.GL9139@dr-wily.mit.edu","subject":"Re: BUG: git rebase -i -p silently looses commits","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-11-11T17:32:42Z","receivedAt":"2009-11-11T17:32:42Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 4 Nov 2009, Greg Price wrote:\n\n> On Mon, 2 Nov 2009, Johannes Schindelin wrote:\n> > Having said that, I worked for some time on fixing this issue, and I \n> > actually run a version of rebase -i -p here that allows reordering \n> > commits, but it is far from stable (and due to GSoC and day-job \n> > obligations, I had no time to work on it in months).\n> \n> I'm interested in this topic too. Some weeks ago I took your\n> rebase-i-p branch from January and rebased it onto the latest release;\n> it's at\n> git://repo.or.cz/git/price.git rebase-i-p\n> and now based on v1.6.5.2. I fixed a few bugs and added a feature,\n> and it's the version I run day to day.\n\nThat is very interesting!\n\nHowever, for rebase-i-p to have a chance to be accepted, I think a few \nthings are necessary still (this is all from memory, so please take \neverything with a grain of salt):\n\n- reorder the series to have the -i fixes first, the new commands next, \n and then the changes to the actual -p mode\n\n- rework the mark stuff so that 'todo' works properly, and then change the \n system to use ':' style bookmarks.\n\n- fix that nasty bug which makes one revision not pass the tests (I forgot \n which one, but it should be in the TODOs)\n\n- add proper handling for the case when a patch has been applied in \n upstream already, but was not correctly identified as that by \n --cherry-pick (well, this TODO is actually not really related to rebase \n -i -p, but something I deeply care about)\n\nUnfortunately, I am getting more and more deprived of Git time budget \nthese days, so that I cannot seem to find a few hours to at least restart \nmy efforts.\n\nCiao,\nDscho\n"},{"id":"127464","messageId":"1ac2d430911120957gecb6a27k4166016ef8498eab@mail.gmail.com","threadId":"21456","inReplyTo":"alpine.DEB.1.00.0911111804520.19111@intel-tinevez-2-302","subject":"Re: BUG: git rebase -i -p silently looses commits","fromName":"Greg Price","fromEmail":"price@ksplice.com","sentAt":"2009-11-12T17:57:09Z","receivedAt":"2009-11-12T17:57:09Z","isPatch":false,"sender":{"key":"price@mit.edu","avatar":"https://avatars.githubusercontent.com/u/28173?v=4"},"body":"On Wed, Nov 11, 2009 at 12:32 PM, Johannes Schindelin\n wrote:\n> That is very interesting!\n>\n> However, for rebase-i-p to have a chance to be accepted, I think a few\n> things are necessary still (this is all from memory, so please take\n> everything with a grain of salt):\n\nGreat, this is helpful, and it overlaps with my existing to-do list.\nI have a couple of questions.\n\n\n> - reorder the series to have the -i fixes first, the new commands next,\n>  and then the changes to the actual -p mode\n\nThis one will be easy when everything else is ready, I think.\n\n\n> - rework the mark stuff so that 'todo' works properly, and then change the\n>  system to use ':' style bookmarks.\n\nThis is the biggest change I was going to suggest! Glad we're on the\nsame page. To be clear, what I want to do here is\n - add a 'mark' command\n - emit 'mark' commands in the TODO generation for the target of each\n'goto', and use them.\nIs that also what you had in mind?\n\n\n> - fix that nasty bug which makes one revision not pass the tests (I forgot\n>  which one, but it should be in the TODOs)\n\nHmm. I see one TODO comment in your patches, and it doesn't sound\nlike this. Is there a TODO somewhere else that I'm missing?\nAlternatively, I can always end up just running the tests on all the\nrevisions and find out.\n\n\n> - add proper handling for the case when a patch has been applied in\n>  upstream already, but was not correctly identified as that by\n>  --cherry-pick (well, this TODO is actually not really related to rebase\n>  -i -p, but something I deeply care about)\n\nHmm. I'll have to think about what the behavior could be here.\nUnless you've already worked out a behavior you would like to see?\nFor context, I think the issue you're referring to is that sometimes\nthe patch-id changed, so that --cherry-pick doesn't identify the\npatch; and then some later upstream patch has touched the same code\nagain, so that there's a conflict when we try to apply the older\npatch. I would also like to see this fixed, but I don't see offhand\nwhat the right behavior would be.\n\nThe \"read my mind\" behavior might be something like, somewhere between\nthe merge-base and the upstream there is a commit after which this one\nwould apply as no changes, so let's say that commit already applied\nthis patch. But that could be the wrong thing if e.g. a patch was\napplied and later reverted. And I don't know offhand how to implement\nit efficiently.\n\nAnyway, I think you're right that this improvement is orthogonal to\nrebase -i -p.\n\n\n> Unfortunately, I am getting more and more deprived of Git time budget\n> these days, so that I cannot seem to find a few hours to at least restart\n> my efforts.\n\nUnderstood. I may have some time to work on this soon, we'll see. I\nthink the priorities will be to\n - add \"mark\" as you say\n - add the \"pause\" command, to make it possible to amend a merge\n - write tests\n - fix a couple of bugs, track down the one you mentioned\n - write documentation\n\nAt that point, and with the reordering you suggested, I think it will\nbe ready to submit for inclusion.\n\nFurther comments, and bug reports from anyone else using the\ndevelopment version, are welcome.\n\nThanks,\nGreg\n"},{"id":"127499","messageId":"alpine.DEB.1.00.0911130958190.4985@pacific.mpi-cbg.de","threadId":"21456","inReplyTo":"1ac2d430911120957gecb6a27k4166016ef8498eab@mail.gmail.com","subject":"Re: BUG: git rebase -i -p silently looses commits","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-11-13T09:07:31Z","receivedAt":"2009-11-13T09:07:31Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Greg,\n\nOn Thu, 12 Nov 2009, Greg Price wrote:\n\n> On Wed, Nov 11, 2009 at 12:32 PM, Johannes Schindelin\n> wrote:\n> > That is very interesting!\n> >\n> > However, for rebase-i-p to have a chance to be accepted, I think a few \n> > things are necessary still (this is all from memory, so please take \n> > everything with a grain of salt):\n> \n> Great, this is helpful, and it overlaps with my existing to-do list. I \n> have a couple of questions.\n> \n> > - rework the mark stuff so that 'todo' works properly, and then change \n> > the  system to use ':' style bookmarks.\n> \n> This is the biggest change I was going to suggest! Glad we're on the\n> same page. To be clear, what I want to do here is\n> - add a 'mark' command\n> - emit 'mark' commands in the TODO generation for the target of each\n> 'goto', and use them.\n> Is that also what you had in mind?\n\nAlmost. I called it \"bookmark\" so that the abbreviated command does not \nclash with \"merge\". And there are possible goto targets you have never \nbeen at:\n\n- A - B - C\n \\ /\n D\n\nIf C is your HEAD, and you \"rebase -i -p B\", before cherry-picking D, you \nhave to \"goto A\".\n\nSo I strongly advise against trying to give all goto targets a name, just \nthe obvious one: onto (you do not need upstream, as all the commits which \nhave upstream as parent are supposed to be applied on top of onto anyway).\n \n> > - fix that nasty bug which makes one revision not pass the tests (I \n> > forgot  which one, but it should be in the TODOs)\n> \n> Hmm. I see one TODO comment in your patches, and it doesn't sound like \n> this. Is there a TODO somewhere else that I'm missing? Alternatively, I \n> can always end up just running the tests on all the revisions and find \n> out.\n\nIt should be in one commit message (something like WIP...).\n\n> > - add proper handling for the case when a patch has been applied in \n> >  upstream already, but was not correctly identified as that by \n> >  --cherry-pick (well, this TODO is actually not really related to \n> > rebase  -i -p, but something I deeply care about)\n> \n> Hmm. I'll have to think about what the behavior could be here.\n\nAt the moment, it gives you the status (which is multiple pages long here, \ndue to untracked files that I am unwilling to move elsewhere) and then \n\"nothing to commit\". You do not even see which commit was to be applied.\n\nMy plan was to detect that condition in the error case and _not_ output \nwhat the cherry-pick printed, but a much more helpful message along the \nlines\n\n\tIt appears \"Bla bli blu\" was already applied\n\nFor the exact message, I am sure all kinds of painters will want to help \nyou.\n\n> For context, I think the issue you're referring to is that sometimes\n> the patch-id changed, so that --cherry-pick doesn't identify the\n> patch;\n\nCorrect.\n\n> and then some later upstream patch has touched the same code again, so \n> that there's a conflict when we try to apply the older patch.\n\nNo, that is not what I mean. I mean when more than one context line \nchanges. Then patch-ids will differ, but a 3-way merge will still succeed \nin finding that there was actually no change.\n\n> > Unfortunately, I am getting more and more deprived of Git time budget \n> > these days, so that I cannot seem to find a few hours to at least \n> > restart my efforts.\n> \n> Understood. I may have some time to work on this soon, we'll see. I\n> think the priorities will be to\n> - add \"mark\" as you say\n> - add the \"pause\" command, to make it possible to amend a merge\n> - write tests\n> - fix a couple of bugs, track down the one you mentioned\n> - write documentation\n> \n> At that point, and with the reordering you suggested, I think it will\n> be ready to submit for inclusion.\n> \n> Further comments, and bug reports from anyone else using the\n> development version, are welcome.\n\nThanks,\nDscho\n\nP.S.: I am mostly off-line until Monday."}]}