{"thread":{"id":"23957","subject":"[PATCH] rebase -i -p: document shortcomings","startedAt":"2010-06-01T01:43:35Z","lastAt":"2010-06-01T17:24:03Z","messageCount":5,"participants":["Jonathan Nieder","Greg Price","Sverre Rabbelier"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"142627","messageId":"20100601014335.GA21970@progeny.tock","threadId":"23957","inReplyTo":null,"subject":"[PATCH] rebase -i -p: document shortcomings","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-06-01T01:43:35Z","receivedAt":"2010-06-01T01:43:35Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The rebase --preserve-merges facility presents a list of commits\nin its instruction sheet and uses a separate table to keep\ntrack of their parents. Unfortunately, in practice this means\nthat with -p after most attempts to rearrange patches, some\ncommits have the \"wrong\" parent and the resulting history is\nrarely what the caller expected.\n\nYes, it would be nice to fix that. But first, add a warning to the\nmanual to help the uninitiated understand what is going on.\n\nReported-by: Jiří Paleček \nCc: Johannes Schindelin \nCc: Thomas Rast \nSigned-off-by: Jonathan Nieder \n---\nPointers towards a real fix:\n\n . http://thread.gmane.org/gmane.comp.version-control.git/79424/focus=79504\n . http://thread.gmane.org/gmane.comp.version-control.git/77965/focus=80835\n . http://thread.gmane.org/gmane.comp.version-control.git/131921/focus=132679\n . http://repo.or.cz/w/git/price.git\n . http://repo.or.cz/w/git/dscho.git/shortlog/refs/heads/rebase-i-p\n\nThanks to all who have worked on this before.\n\nThoughts?\n\n Documentation/git-rebase.txt | 26 ++++++++++++++++++++++++++\n t/t3404-rebase-interactive.sh | 6 ++++++\n 2 files changed, 32 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 5863dec..330e9ea 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -306,6 +306,11 @@ link:howto/revert-a-faulty-merge.txt[revert-a-faulty-merge How-To] for details).\n -p::\n --preserve-merges::\n \tInstead of ignoring merges, try to recreate them.\n++\n+This uses the `--interactive` machinery internally, but combining it\n+with the `--interactive` option explicitly is generally not a good\n+idea (see BUGS below).\n+\n \n --root::\n \tRebase all commits reachable from , instead of\n@@ -607,6 +612,27 @@ The ripple effect of a \"hard case\" recovery is especially bad:\n case\" recovery too!\n \n \n+BUGS\n+----\n+The instruction sheet presented by `--preserve-merges --interactive`\n+does not represent the topology of the revision graph. Attempts to\n+reorder it tend to produce counterintuitive results.\n+\n+For example, an attempt to rearrange\n+------------\n+1 --- 2 --- 3 --- 4 --- 5\n+------------\n+to\n+------------\n+1 --- 2 --- 4 --- 3 --- 5\n+------------\n+by moving the \"pick 4\" line will result in the following history:\n+------------\n+ 3\n+ /\n+1 --- 2 --- 4 --- 5\n+------------\n+\n Authors\n ------\n Written by Junio C Hamano and\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex f20ea38..6668907 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -181,6 +181,12 @@ test_expect_success '-p handles \"no changes\" gracefully' '\n \ttest $HEAD = $(git rev-parse HEAD)\n '\n \n+test_expect_failure 'exchange two commits with -p' '\n+\tFAKE_LINES=\"2 1\" git rebase -i -p HEAD~2 &&\n+\ttest H = $(git cat-file commit HEAD^ | sed -ne \\$p) &&\n+\ttest G = $(git cat-file commit HEAD | sed -ne \\$p)\n+'\n+\n test_expect_success 'preserve merges with -p' '\n \tgit checkout -b to-be-preserved master^ &&\n \t: > unrelated-file &&\n-- \n1.7.1\n"},{"id":"142628","messageId":"AANLkTimjcHpbbpfwNoZGq3_1TjfLAzJL3C8Jcp6LeyW0@mail.gmail.com","threadId":"23957","inReplyTo":"20100601014335.GA21970@progeny.tock","subject":"Re: [PATCH] rebase -i -p: document shortcomings","fromName":"Greg Price","fromEmail":"price@ksplice.com","sentAt":"2010-06-01T02:22:14Z","receivedAt":"2010-06-01T02:22:14Z","isPatch":true,"sender":{"key":"price@mit.edu","avatar":"https://avatars.githubusercontent.com/u/28173?v=4"},"body":"Thanks, this is long needed. I regret that I haven't had time in\nseveral months to continue work on the real fix.\n\nThe combination \"rebase -i -p\" does work correctly in one common use\ncase: where you only use \"edit\" and similar commands, and do not\nreorder the commits. I used it regularly for this purpose before I\neven noticed that it fails when you do reorder commits. Not sure if\nit's worth trying to make that distinction clear, though.\n\nI suggest s/instruction sheet/todo list/, as the latter terminology\nseems more natural to me, and also appears already elsewhere in the\n\"git rebase\" documentation.\n\nCheers,\nGreg\n\n\n\nOn Mon, May 31, 2010 at 9:43 PM, Jonathan Nieder wrote:\n> The rebase --preserve-merges facility presents a list of commits\n> in its instruction sheet and uses a separate table to keep\n> track of their parents.  Unfortunately, in practice this means\n> that with -p after most attempts to rearrange patches, some\n> commits have the \"wrong\" parent and the resulting history is\n> rarely what the caller expected.\n>\n> Yes, it would be nice to fix that.  But first, add a warning to the\n> manual to help the uninitiated understand what is going on.\n>\n> Reported-by: Jiří Paleček \n> Cc: Johannes Schindelin \n> Cc: Thomas Rast \n> Signed-off-by: Jonathan Nieder \n> ---\n> Pointers towards a real fix:\n>\n>  . http://thread.gmane.org/gmane.comp.version-control.git/79424/focus=79504\n>  . http://thread.gmane.org/gmane.comp.version-control.git/77965/focus=80835\n>  . http://thread.gmane.org/gmane.comp.version-control.git/131921/focus=132679\n>  . http://repo.or.cz/w/git/price.git\n>  . http://repo.or.cz/w/git/dscho.git/shortlog/refs/heads/rebase-i-p\n>\n> Thanks to all who have worked on this before.\n>\n> Thoughts?\n>\n>  Documentation/git-rebase.txt  |   26 ++++++++++++++++++++++++++\n>  t/t3404-rebase-interactive.sh |    6 ++++++\n>  2 files changed, 32 insertions(+), 0 deletions(-)\n>\n> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\n> index 5863dec..330e9ea 100644\n> --- a/Documentation/git-rebase.txt\n> +++ b/Documentation/git-rebase.txt\n> @@ -306,6 +306,11 @@ link:howto/revert-a-faulty-merge.txt[revert-a-faulty-merge How-To] for details).\n>  -p::\n>  --preserve-merges::\n>        Instead of ignoring merges, try to recreate them.\n> ++\n> +This uses the `--interactive` machinery internally, but combining it\n> +with the `--interactive` option explicitly is generally not a good\n> +idea (see BUGS below).\n> +\n>\n>  --root::\n>        Rebase all commits reachable from , instead of\n> @@ -607,6 +612,27 @@ The ripple effect of a \"hard case\" recovery is especially bad:\n>  case\" recovery too!\n>\n>\n> +BUGS\n> +----\n> +The instruction sheet presented by `--preserve-merges --interactive`\n> +does not represent the topology of the revision graph.  Attempts to\n> +reorder it tend to produce counterintuitive results.\n> +\n> +For example, an attempt to rearrange\n> +------------\n> +1 --- 2 --- 3 --- 4 --- 5\n> +------------\n> +to\n> +------------\n> +1 --- 2 --- 4 --- 3 --- 5\n> +------------\n> +by moving the \"pick 4\" line will result in the following history:\n> +------------\n> +        3\n> +       /\n> +1 --- 2 --- 4 --- 5\n> +------------\n> +\n>  Authors\n>  ------\n>  Written by Junio C Hamano and\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index f20ea38..6668907 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -181,6 +181,12 @@ test_expect_success '-p handles \"no changes\" gracefully' '\n>        test $HEAD = $(git rev-parse HEAD)\n>  '\n>\n> +test_expect_failure 'exchange two commits with -p' '\n> +       FAKE_LINES=\"2 1\" git rebase -i -p HEAD~2 &&\n> +       test H = $(git cat-file commit HEAD^ | sed -ne \\$p) &&\n> +       test G = $(git cat-file commit HEAD | sed -ne \\$p)\n> +'\n> +\n>  test_expect_success 'preserve merges with -p' '\n>        git checkout -b to-be-preserved master^ &&\n>        : > unrelated-file &&\n> --\n> 1.7.1\n>\n>\n"},{"id":"142660","messageId":"20100601085005.GA32320@progeny.tock","threadId":"23957","inReplyTo":"AANLkTimjcHpbbpfwNoZGq3_1TjfLAzJL3C8Jcp6LeyW0@mail.gmail.com","subject":"Re: [PATCH] rebase -i -p: document shortcomings","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-06-01T08:50:05Z","receivedAt":"2010-06-01T08:50:05Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Greg,\n\nGreg Price wrote:\n\n> The combination \"rebase -i -p\" does work correctly in one common use\n> case:\n[...]\n> I suggest s/instruction sheet/todo list/, as the latter terminology\n> seems more natural to me\n\nHere’s a rough patch to squash in. Thanks for the ideas.\n\nSleepily,\nJonathan\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 330e9ea..e27099e 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -309,7 +309,7 @@ link:howto/revert-a-faulty-merge.txt[revert-a-faulty-merge How-To] for details).\n +\n This uses the `--interactive` machinery internally, but combining it\n with the `--interactive` option explicitly is generally not a good\n-idea (see BUGS below).\n+idea unless you know what you are doing (see BUGS below).\n \n \n --root::\n@@ -614,9 +614,10 @@ case\" recovery too!\n \n BUGS\n ----\n-The instruction sheet presented by `--preserve-merges --interactive`\n-does not represent the topology of the revision graph. Attempts to\n-reorder it tend to produce counterintuitive results.\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 \n For example, an attempt to rearrange\n ------------\n-- \n1.7.1.246.g2fc1b.dirty\n"},{"id":"142677","messageId":"AANLkTimy22yhbxDEfcIg5nPxguWslCIrIEk6wH9kzZqg@mail.gmail.com","threadId":"23957","inReplyTo":"AANLkTimjcHpbbpfwNoZGq3_1TjfLAzJL3C8Jcp6LeyW0@mail.gmail.com","subject":"Re: [PATCH] rebase -i -p: document shortcomings","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-06-01T11:40:38Z","receivedAt":"2010-06-01T11:40:38Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Tue, Jun 1, 2010 at 04:22, Greg Price wrote:\n> Thanks, this is long needed.  I regret that I haven't had time in\n> several months to continue work on the real fix.\n\nIs it your or Jonathan's intent on finishing Dscho's work on rebase -i -p?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"142712","messageId":"AANLkTik-piGwunSM4zsSBR5U8uzuN58IgtFRFV54XzkJ@mail.gmail.com","threadId":"23957","inReplyTo":"AANLkTimy22yhbxDEfcIg5nPxguWslCIrIEk6wH9kzZqg@mail.gmail.com","subject":"Re: [PATCH] rebase -i -p: document shortcomings","fromName":"Greg Price","fromEmail":"price@ksplice.com","sentAt":"2010-06-01T17:24:03Z","receivedAt":"2010-06-01T17:24:03Z","isPatch":true,"sender":{"key":"price@mit.edu","avatar":"https://avatars.githubusercontent.com/u/28173?v=4"},"body":"On Tue, Jun 1, 2010 at 7:40 AM, Sverre Rabbelier wrote:\n> On Tue, Jun 1, 2010 at 04:22, Greg Price wrote:\n>> Thanks, this is long needed. I regret that I haven't had time in\n>> several months to continue work on the real fix.\n>\n> Is it your or Jonathan's intent on finishing Dscho's work on rebase -i -p?\n\nThat was my plan. I did some work in that direction, which is at\nhttp://repo.or.cz/w/git/price.git/shortlog/refs/heads/rebase-i-p\nand which I discussed with Dscho last November:\nhttp://thread.gmane.org/gmane.comp.version-control.git/131921/focus=132156\n\nUnfortunately work has made me busy with non-Git subjects since\nDecember. I would like to finish the work, but I cannot make\npromises.\n\nGreg\n"}]}