{"thread":{"id":"46685","subject":"Commit dropped when swapping commits with rebase -i -p","startedAt":"2017-08-30T10:12:06Z","lastAt":"2017-09-17T13:31:13Z","messageCount":12,"participants":["Sebastian Schuberth","Martin Ågren","Johannes Schindelin","Jonathan Nieder","Junio C Hamano","Andreas Heiduk","Phillip Wood"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"327394","messageId":"oo62vr$pvq$1@blaine.gmane.org","threadId":"46685","inReplyTo":null,"subject":"Commit dropped when swapping commits with rebase -i -p","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2017-08-30T10:11:14Z","receivedAt":"2017-08-30T10:12:06Z","isPatch":false,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"Hi,\n\nI believe stumbled upon a nasty bug in Git: It looks like a commits gets dropped during interactive rebase when asking to preserve merges. Steps:\n\n$ git clone -b git-bug --single-branch https://github.com/heremaps/scancode-toolkit.git\n$ git rebase -i -p HEAD~2\n# In the editor, swap the order of the two (non-merge) commits.\n$ git diff origin/git-bug\n\nThe last command will show a non-empty diff which looks as if the \"Do not shadow the os package with a variable name\" commit has been dropped, and indeed \"git log\" confirms this. The commit should not get dropped, and it does not if just using \"git rebase -i -p HEAD~2\" (without \"-p\").\n\nI'm observing this with Git 2.14.1 on Linux.\n\nRegards,\nSebastian\n\n\n\n"},{"id":"327447","messageId":"CAN0heSqGfxrFTwuaxgppZTx+3U=g_Qs4PyaCBF6ddV_PbvdpTQ@mail.gmail.com","threadId":"46685","inReplyTo":"oo62vr$pvq$1@blaine.gmane.org","subject":"Re: Commit dropped when swapping commits with rebase -i -p","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-30T18:07:39Z","receivedAt":"2017-08-30T18:07:45Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 30 August 2017 at 12:11, Sebastian Schuberth wrote:\n> Hi,\n>\n> I believe stumbled upon a nasty bug in Git: It looks like a commits gets dropped during interactive rebase when asking to preserve merges. Steps:\n>\n> $ git clone -b git-bug --single-branch https://github.com/heremaps/scancode-toolkit.git\n> $ git rebase -i -p HEAD~2\n> # In the editor, swap the order of the two (non-merge) commits.\n> $ git diff origin/git-bug\n>\n> The last command will show a non-empty diff which looks as if the \"Do not shadow the os package with a variable name\" commit has been dropped, and indeed \"git log\" confirms this. The commit should not get dropped, and it does not if just using \"git rebase -i -p HEAD~2\" (without \"-p\").\n>\n> I'm observing this with Git 2.14.1 on Linux.\n\nThe man-page for git rebase says that combining -p with -i is \"generally\nnot a good idea unless you know what you are doing (see BUGS below)\".\n\nUnder BUGS, it says\n\n\"The todo list presented by --preserve-merges --interactive does not\nrepresent the topology of the revision graph. Editing commits and\nrewording their commit messages should work fine, but attempts to\nreorder commits tend to produce counterintuitive results.\"\n\nSo if you agree that a \"dropped commit\" is a \"counterintuitive result\",\nthis is known and documented. Maybe the warning could be harsher, but it\ndoes say \"unless you know what you are doing\".\n\nMartin\n"},{"id":"327458","messageId":"CAHGBnuMC_10krsdZe2KiQ4jjiL43kogn--dWjPgca_p2xgmQMA@mail.gmail.com","threadId":"46685","inReplyTo":"CAN0heSqGfxrFTwuaxgppZTx+3U=g_Qs4PyaCBF6ddV_PbvdpTQ@mail.gmail.com","subject":"Re: Commit dropped when swapping commits with rebase -i -p","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2017-08-30T18:59:34Z","receivedAt":"2017-08-30T18:59:40Z","isPatch":false,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Wed, Aug 30, 2017 at 8:07 PM, Martin Ågren wrote:\n\n> The man-page for git rebase says that combining -p with -i is \"generally\n> not a good idea unless you know what you are doing (see BUGS below)\".\n\nThanks for pointing this out again. I remember to have read this some\ntime ago, but as I general consider myself to know what I'm doing, I\nforgot about it :-)\n\nAnyway, this should really more explicitly say *what* you need to know\nabout, that is, reordering commits does not work.\n\n> So if you agree that a \"dropped commit\" is a \"counterintuitive result\",\n> this is known and documented. Maybe the warning could be harsher, but it\n> does say \"unless you know what you are doing\".\n\nI'd say it's worse than counterintuitive, as counterintuitive might\nstill be correct, while in my case it clearly is not. So yes, the\nwarning must be harsher in my opinion. Maybe we should even abort\nrebase -i-p if reordering of commits is detected.\n\n-- \nSebastian Schuberth\n"},{"id":"327466","messageId":"alpine.DEB.2.21.1.1708302223510.7424@virtualbox","threadId":"46685","inReplyTo":"oo62vr$pvq$1@blaine.gmane.org","subject":"Re: Commit dropped when swapping commits with rebase -i -p","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-08-30T20:28:19Z","receivedAt":"2017-08-30T20:28:26Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Sebastian,\n\nOn Wed, 30 Aug 2017, Sebastian Schuberth wrote:\n\n> I believe stumbled upon a nasty bug in Git: It looks like a commits gets\n> dropped during interactive rebase when asking to preserve merges.\n\nPlease see 'exchange two commits with -p' in t3404. This is a known\nbreakage, and due to the fact that -p and -i are fundamentally\nincompatible with one another (even if -p's implementation was based on\n-i's). I never had in mind for -p to be allowed together with -i, and was\nagainst allowing it because of the design.\n\nShort version: do not use -p with -i.\n\nCiao,\nJohannes\n"},{"id":"327468","messageId":"CAHGBnuO0dviVr0zD+KqANc6Ju8-cZh2KLbLz6NH3h+jprRzbaw@mail.gmail.com","threadId":"46685","inReplyTo":"alpine.DEB.2.21.1.1708302223510.7424@virtualbox","subject":"Re: Commit dropped when swapping commits with rebase -i -p","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2017-08-30T20:48:31Z","receivedAt":"2017-08-30T20:48:37Z","isPatch":false,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Wed, Aug 30, 2017 at 10:28 PM, Johannes Schindelin\n wrote:\n\n> Please see 'exchange two commits with -p' in t3404. This is a known\n\nThank for pointing out the test.\n\n> breakage, and due to the fact that -p and -i are fundamentally\n> incompatible with one another (even if -p's implementation was based on\n> -i's). I never had in mind for -p to be allowed together with -i, and was\n> against allowing it because of the design.\n\nIn any case, I wouldn't have expected *that* kind of side effect for\nsuch a simple case (that does not involve any merge commits).\n\nIf these options are fundamentally incompatible as you say, would you\nagree that it makes sense to disallow their usage together (instead of\njust documenting that you should know what you're doing)?\n\n-- \nSebastian Schuberth\n"},{"id":"327508","messageId":"alpine.DEB.2.21.1.1709012116060.4132@virtualbox","threadId":"46685","inReplyTo":"CAHGBnuO0dviVr0zD+KqANc6Ju8-cZh2KLbLz6NH3h+jprRzbaw@mail.gmail.com","subject":"Re: Commit dropped when swapping commits with rebase -i -p","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-09-01T19:16:28Z","receivedAt":"2017-09-01T19:16:40Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Sebastian,\n\nOn Wed, 30 Aug 2017, Sebastian Schuberth wrote:\n\n> On Wed, Aug 30, 2017 at 10:28 PM, Johannes Schindelin\n> wrote:\n> \n> > Please see 'exchange two commits with -p' in t3404. This is a known\n> \n> Thank for pointing out the test.\n> \n> > breakage, and due to the fact that -p and -i are fundamentally\n> > incompatible with one another (even if -p's implementation was based on\n> > -i's). I never had in mind for -p to be allowed together with -i, and was\n> > against allowing it because of the design.\n> \n> In any case, I wouldn't have expected *that* kind of side effect for\n> such a simple case (that does not involve any merge commits).\n> \n> If these options are fundamentally incompatible as you say, would you\n> agree that it makes sense to disallow their usage together (instead of\n> just documenting that you should know what you're doing)?\n\nAs I said already, I agreed with you before you said it, but I was\noverruled.\n\nCiao,\nJohannes\n"},{"id":"327522","messageId":"20170902000417.GE143138@aiede.mtv.corp.google.com","threadId":"46685","inReplyTo":"CAHGBnuMC_10krsdZe2KiQ4jjiL43kogn--dWjPgca_p2xgmQMA@mail.gmail.com","subject":"Re: Commit dropped when swapping commits with rebase -i -p","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-02T00:04:17Z","receivedAt":"2017-09-02T00:05:26Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nSebastian Schuberth wrote:\n> On Wed, Aug 30, 2017 at 8:07 PM, Martin Ågren wrote:\n\n>> The man-page for git rebase says that combining -p with -i is \"generally\n>> not a good idea unless you know what you are doing (see BUGS below)\".\n>\n> Thanks for pointing this out again. I remember to have read this some\n> time ago, but as I general consider myself to know what I'm doing, I\n> forgot about it :-)\n\nHeh.\n\n> Anyway, this should really more explicitly say *what* you need to know\n> about, that is, reordering commits does not work.\n\nIt tries to explain that, even with an example. If you have ideas for\nimproving the wording, that would be welcome.\n\nThat said, ...\n\n>> So if you agree that a \"dropped commit\" is a \"counterintuitive result\",\n>> this is known and documented. Maybe the warning could be harsher, but it\n>> does say \"unless you know what you are doing\".\n>\n> I'd say it's worse than counterintuitive, as counterintuitive might\n> still be correct, while in my case it clearly is not. So yes, the\n> warning must be harsher in my opinion. Maybe we should even abort\n> rebase -i-p if reordering of commits is detected.\n\nThis sounds like a more promising approach. If you can detect when\nthe rebase -i -p is going to cause trouble, then I would be all for\naborting. If you want to be extra nice to people, you can provide a\n--force escape valve to let them experience the broken behavior, but I\ndon't think that is necessary.\n\nI also think a loud warning when -i -p is used even when it is not\ngoing to cause trouble would be a valuable change. E.g. maybe the\ntemplate that opens in the editor could say something about reordering\ncommits not being advisable?\n\nE.g. I could imagine the todo list including some instructions in the\nspirit of\n\n\t# git rebase --preserve-merges does not support reordering commits.\n\t# To attempt reordering anyway, add a line with the text \"reorder\".\n\t# It is not likely to behave as you expect. You have been\n\t# warned.\n\nThanks,\nJonathan\n"},{"id":"327845","messageId":"a47058cc-8ffc-4484-c247-3c8d4f827c07@gmail.com","threadId":"46685","inReplyTo":"20170902000417.GE143138@aiede.mtv.corp.google.com","subject":"Re: Commit dropped when swapping commits with rebase -i -p","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2017-09-11T08:45:40Z","receivedAt":"2017-09-11T08:45:53Z","isPatch":false,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On 2017-09-02 02:04, Jonathan Nieder wrote:\n\n>> Anyway, this should really more explicitly say *what* you need to know\n>> about, that is, reordering commits does not work.\n> \n> It tries to explain that, even with an example. If you have ideas for\n> improving the wording, that would be welcome.\n\nAs a first step, I indeed believe the wording must the stronger / clearer. How about this:\n\nFrom f69854ce7b9359603581317d152421ff6d89f345 Mon Sep 17 00:00:00 2001\nFrom: Sebastian Schuberth \nDate: Mon, 11 Sep 2017 10:41:27 +0200\nSubject: [PATCH] docs: use a stronger wording when describing bugs with rebase -i -p\n\nSigned-off-by: Sebastian Schuberth \n---\n Documentation/git-rebase.txt | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 6805a74aec..ccd0a04d54 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -782,10 +782,11 @@ case\" recovery too!\n \n BUGS\n ----\n-The todo list presented by `--preserve-merges --interactive` does not\n-represent the topology of the revision graph. Editing commits and\n-rewording their commit messages should work fine, but attempts to\n-reorder commits tend to produce counterintuitive results.\n+Be careful when combining the `-i` / `--interactive` and `-p` /\n+`--preserve-merges` options. Reordering commits will drop commits from the\n+main line. This is because the todo list does not represent the topology of the\n+revision graph in this case. However, editing commits and rewording their\n+commit messages 'should' work fine.\n \n For example, an attempt to rearrange\n ------------\n-- \n2.14.1.windows.1\n"},{"id":"328167","messageId":"xmqqbmmbwuq0.fsf@gitster.mtv.corp.google.com","threadId":"46685","inReplyTo":"a47058cc-8ffc-4484-c247-3c8d4f827c07@gmail.com","subject":"Re: Commit dropped when swapping commits with rebase -i -p","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-15T20:52:07Z","receivedAt":"2017-09-15T20:52:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> On 2017-09-02 02:04, Jonathan Nieder wrote:\n>\n>>> Anyway, this should really more explicitly say *what* you need to know\n>>> about, that is, reordering commits does not work.\n>> \n>> It tries to explain that, even with an example. If you have ideas for\n>> improving the wording, that would be welcome.\n>\n> As a first step, I indeed believe the wording must the stronger / clearer. How about this:\n>\n> From f69854ce7b9359603581317d152421ff6d89f345 Mon Sep 17 00:00:00 2001\n> From: Sebastian Schuberth \n> Date: Mon, 11 Sep 2017 10:41:27 +0200\n> Subject: [PATCH] docs: use a stronger wording when describing bugs with rebase -i -p\n>\n> Signed-off-by: Sebastian Schuberth \n> ---\n> Documentation/git-rebase.txt | 9 +++++----\n> 1 file changed, 5 insertions(+), 4 deletions(-)\n>\n> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\n> index 6805a74aec..ccd0a04d54 100644\n> --- a/Documentation/git-rebase.txt\n> +++ b/Documentation/git-rebase.txt\n> @@ -782,10 +782,11 @@ case\" recovery too!\n> \n> BUGS\n> ----\n> -The todo list presented by `--preserve-merges --interactive` does not\n> -represent the topology of the revision graph. Editing commits and\n> -rewording their commit messages should work fine, but attempts to\n> -reorder commits tend to produce counterintuitive results.\n> +Be careful when combining the `-i` / `--interactive` and `-p` /\n> +`--preserve-merges` options. Reordering commits will drop commits from the\n> +main line. This is because the todo list does not represent the topology of the\n> +revision graph in this case. However, editing commits and rewording their\n> +commit messages 'should' work fine.\n> \n> For example, an attempt to rearrange\n> ------------\n\n\nAnybody? I personally feel that the updated text is not all that\nstronger but it is clearer by clarifying what \"counterintuitive\nresults\" actually mean, but I am not the target audience this\nparagraph is trying to help, nor I am the one who is making excuse\nfor a known bug, so...\n\n"},{"id":"328229","messageId":"9e004c75-bc35-06ae-8479-9440059c4d0f@gmail.com","threadId":"46685","inReplyTo":"xmqqbmmbwuq0.fsf@gitster.mtv.corp.google.com","subject":"Re: Commit dropped when swapping commits with rebase -i -p","fromName":"Andreas Heiduk","fromEmail":"asheiduk@gmail.com","sentAt":"2017-09-16T10:41:55Z","receivedAt":"2017-09-16T10:42:05Z","isPatch":false,"sender":{"key":"asheiduk@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9371344?v=4"},"body":"Am 15.09.2017 um 22:52 schrieb Junio C Hamano:\n> Sebastian Schuberth writes:\n>>\n>> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\n>> index 6805a74aec..ccd0a04d54 100644\n>> --- a/Documentation/git-rebase.txt\n>> +++ b/Documentation/git-rebase.txt\n>> @@ -782,10 +782,11 @@ case\" recovery too!\n>> \n>> BUGS\n>> ----\n>> -The todo list presented by `--preserve-merges --interactive` does not\n>> -represent the topology of the revision graph. Editing commits and\n>> -rewording their commit messages should work fine, but attempts to\n>> -reorder commits tend to produce counterintuitive results.\n>> +Be careful when combining the `-i` / `--interactive` and `-p` /\n\n\"Be careful\" is not necessary because the text is already in the \"BUGS\"\nsection.\n\n>> +`--preserve-merges` options. Reordering commits will drop commits from the\n>> +main line. This is because the todo list does not represent the topology of the\n>> +revision graph in this case. However, editing commits and rewording their\n>> +commit messages 'should' work fine.\n>> \n>> For example, an attempt to rearrange\n>> ------------\n> \n> \n> Anybody? I personally feel that the updated text is not all that\n> stronger but it is clearer by clarifying what \"counterintuitive\n> results\" actually mean, but I am not the target audience this\n> paragraph is trying to help, nor I am the one who is making excuse\n> for a known bug, so...\n> \n\nFor me the proposed wording implies that the only bad effect are dropped\ncommits on the mainline. But I experienced something like this:\n\n\nO--O--O--O---M--O ==> O--O--O--O---M--O\n \\ / \\ /\n O--X--O--O O--X O\n\n\nWhere X was a commit without a ref and hence lost. Also the merge commit\nseemed to combine two unrelated histories.\n\nTherefore I would avoid \"definitive wording\" like \"will drop\" and use\nvague wording along \"there are various dragons out there\" like this:\n\n The todo list presented by `--preserve-merges --interactive` does\n not represent the topology of the revision graph. Editing\n commits and rewording their commit messages should work fine.\n But reordering, combining or dropping commits of a complex topology\n can produce unexpected and useless results like missing commits,\n wrong merges, merges combining two unrelated histories and\n similar things.\n\n"},{"id":"328233","messageId":"CAHGBnuMBD1kVJoFLB-apUKbKJipOW3XTGqO+5W8jesY100ZFcg@mail.gmail.com","threadId":"46685","inReplyTo":"9e004c75-bc35-06ae-8479-9440059c4d0f@gmail.com","subject":"Re: Commit dropped when swapping commits with rebase -i -p","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2017-09-16T13:45:02Z","receivedAt":"2017-09-16T13:45:10Z","isPatch":false,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Sat, Sep 16, 2017 at 12:41 PM, Andreas Heiduk wrote:\n\n> Therefore I would avoid \"definitive wording\" like \"will drop\" and use\n> vague wording along \"there are various dragons out there\" like this:\n>\n> The todo list presented by `--preserve-merges --interactive` does\n> not represent the topology of the revision graph. Editing\n\nI tried to avoid this introducing sentence from the original wording\nas it reads like from a scientific research paper instead of from a\nuser's manual.\n\n> commits and rewording their commit messages should work fine.\n> But reordering, combining or dropping commits of a complex topology\n\nThere is no need for complex topology. If you reorder the two most\nrecent commits in a linear history, one gets dropped.\n\n> can produce unexpected and useless results like missing commits,\n> wrong merges, merges combining two unrelated histories and\n> similar things.\n\n\"can produce\" is much too soft, IMO. Reordering commits goes wrong,\nperiod. Like wise \"unexpected and useless results\" is inappropriate.\nThe results are wrong in case of reordering, and wrong results are of\ncourse unexpected and useless.\n\n-- \nSebastian Schuberth\n"},{"id":"328262","messageId":"71c4d0ee-bc64-0c94-7991-2cb6d0a2bfd1@talktalk.net","threadId":"46685","inReplyTo":"CAHGBnuMBD1kVJoFLB-apUKbKJipOW3XTGqO+5W8jesY100ZFcg@mail.gmail.com","subject":"Re: Commit dropped when swapping commits with rebase -i -p","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2017-09-17T13:31:04Z","receivedAt":"2017-09-17T13:31:13Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 16/09/17 14:45, Sebastian Schuberth wrote:\n> \n> On Sat, Sep 16, 2017 at 12:41 PM, Andreas Heiduk wrote:\n> \n>> Therefore I would avoid \"definitive wording\" like \"will drop\" and use\n>> vague wording along \"there are various dragons out there\" like this:\n>>\n>> The todo list presented by `--preserve-merges --interactive` does\n>> not represent the topology of the revision graph. Editing\n> \n> I tried to avoid this introducing sentence from the original wording\n> as it reads like from a scientific research paper instead of from a\n> user's manual.\n> \n>> commits and rewording their commit messages should work fine.\n>> But reordering, combining or dropping commits of a complex topology\n> \n> There is no need for complex topology. If you reorder the two most\n> recent commits in a linear history, one gets dropped.\n> \n>> can produce unexpected and useless results like missing commits,\n>> wrong merges, merges combining two unrelated histories and\n>> similar things.\n> \n> \"can produce\" is much too soft, IMO. Reordering commits goes wrong,\n> period. Like wise \"unexpected and useless results\" is inappropriate.\n> The results are wrong in case of reordering, and wrong results are of\n> course unexpected and useless.\n\nI agree that the wording needs to be explicit that bad things will \nhappen. It should spell out that if commits or reordered, or the fixup \nor squash commands are used then commits will be dropped and if commits \nare deleted from the list or the drop command is used other commits \nother than the intended ones will be dropped as well.\n\n"}]}