threads / patch / 23957

patchrebase -i -p: document shortcomings

Subject: [PATCH] rebase -i -p: document shortcomings

## tl;dr

5 messages between Jun 1, 2010 and Jun 1, 2010. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Jonathan Nieder· Jun 1, 2010, 01:43 UTC · lore

The rebase --preserve-merges facility presents a list of commits in its instruction sheet and uses a separate table to keep track of their parents. Unfortunately, in practice this means that with -p after most attempts to rearrange patches, some commits have the "wrong" parent and the resulting history is rarely what the caller expected.

Yes, it would be nice to fix that. But first, add a warning to the manual to help the uninitiated understand what is going on.

Reported-by: Jiří Paleček <jpalecek@web.de>
Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: Thomas Rast <trast@student.ethz.ch>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Pointers towards a real fix:
 . http://thread.gmane.org/gmane.comp.version-control.git/79424/focus=79504
 . http://thread.gmane.org/gmane.comp.version-control.git/77965/focus=80835
 . http://thread.gmane.org/gmane.comp.version-control.git/131921/focus=132679
 . http://repo.or.cz/w/git/price.git
 . http://repo.or.cz/w/git/dscho.git/shortlog/refs/heads/rebase-i-p
Thanks to all who have worked on this before.
Thoughts?
 Documentation/git-rebase.txt  |   26 ++++++++++++++++++++++++++
 t/t3404-rebase-interactive.sh |    6 ++++++
 2 files changed, 32 insertions(+), 0 deletions(-)
Show changes to 2 files +32 −0

Documentation/git-rebase.txt, t/t3404-rebase-interactive.sh

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 5863dec..330e9ea 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -306,6 +306,11 @@ link:howto/revert-a-faulty-merge.txt[revert-a-faulty-merge How-To] for details).
 -p::
 --preserve-merges::
 	Instead of ignoring merges, try to recreate them.
++
+This uses the `--interactive` machinery internally, but combining it
+with the `--interactive` option explicitly is generally not a good
+idea (see BUGS below).
+
 
 --root::
 	Rebase all commits reachable from <branch>, instead of
@@ -607,6 +612,27 @@ The ripple effect of a "hard case" recovery is especially bad:
 case" recovery too!
 
 
+BUGS
+----
+The instruction sheet presented by `--preserve-merges --interactive`
+does not represent the topology of the revision graph.  Attempts to
+reorder it tend to produce counterintuitive results.
+
+For example, an attempt to rearrange
+------------
+1 --- 2 --- 3 --- 4 --- 5
+------------
+to
+------------
+1 --- 2 --- 4 --- 3 --- 5
+------------
+by moving the "pick 4" line will result in the following history:
+------------
+        3
+       /
+1 --- 2 --- 4 --- 5
+------------
+
 Authors
 ------
 Written by Junio C Hamano <gitster@pobox.com> and
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index f20ea38..6668907 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -181,6 +181,12 @@ test_expect_success '-p handles "no changes" gracefully' '
 	test $HEAD = $(git rev-parse HEAD)
 '
 
+test_expect_failure 'exchange two commits with -p' '
+	FAKE_LINES="2 1" git rebase -i -p HEAD~2 &&
+	test H = $(git cat-file commit HEAD^ | sed -ne \$p) &&
+	test G = $(git cat-file commit HEAD | sed -ne \$p)
+'
+
 test_expect_success 'preserve merges with -p' '
 	git checkout -b to-be-preserved master^ &&
 	: > unrelated-file &&
-- 
1.7.1
Greg Price· Jun 1, 2010, 02:22 UTC · re: Jonathan Nieder · lore

Re: [PATCH] rebase -i -p: document shortcomings

Thanks, this is long needed. I regret that I haven't had time in several months to continue work on the real fix.

The combination "rebase -i -p" does work correctly in one common use case: where you only use "edit" and similar commands, and do not reorder the commits. I used it regularly for this purpose before I even noticed that it fails when you do reorder commits. Not sure if it's worth trying to make that distinction clear, though.

I suggest s/instruction sheet/todo list/, as the latter terminology seems more natural to me, and also appears already elsewhere in the "git rebase" documentation.

Cheers, Greg

On Mon, May 31, 2010 at 9:43 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 96 quoted lines
> The rebase --preserve-merges facility presents a list of commits
> in its instruction sheet and uses a separate table to keep
> track of their parents.  Unfortunately, in practice this means
> that with -p after most attempts to rearrange patches, some
> commits have the "wrong" parent and the resulting history is
> rarely what the caller expected.
>
> Yes, it would be nice to fix that.  But first, add a warning to the
> manual to help the uninitiated understand what is going on.
>
> Reported-by: Jiří Paleček <jpalecek@web.de>
> Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Cc: Thomas Rast <trast@student.ethz.ch>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Pointers towards a real fix:
>
>  . http://thread.gmane.org/gmane.comp.version-control.git/79424/focus=79504
>  . http://thread.gmane.org/gmane.comp.version-control.git/77965/focus=80835
>  . http://thread.gmane.org/gmane.comp.version-control.git/131921/focus=132679
>  . http://repo.or.cz/w/git/price.git
>  . http://repo.or.cz/w/git/dscho.git/shortlog/refs/heads/rebase-i-p
>
> Thanks to all who have worked on this before.
>
> Thoughts?
>
>  Documentation/git-rebase.txt  |   26 ++++++++++++++++++++++++++
>  t/t3404-rebase-interactive.sh |    6 ++++++
>  2 files changed, 32 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 5863dec..330e9ea 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -306,6 +306,11 @@ link:howto/revert-a-faulty-merge.txt[revert-a-faulty-merge How-To] for details).
>  -p::
>  --preserve-merges::
>        Instead of ignoring merges, try to recreate them.
> ++
> +This uses the `--interactive` machinery internally, but combining it
> +with the `--interactive` option explicitly is generally not a good
> +idea (see BUGS below).
> +
>
>  --root::
>        Rebase all commits reachable from <branch>, instead of
> @@ -607,6 +612,27 @@ The ripple effect of a "hard case" recovery is especially bad:
>  case" recovery too!
>
>
> +BUGS
> +----
> +The instruction sheet presented by `--preserve-merges --interactive`
> +does not represent the topology of the revision graph.  Attempts to
> +reorder it tend to produce counterintuitive results.
> +
> +For example, an attempt to rearrange
> +------------
> +1 --- 2 --- 3 --- 4 --- 5
> +------------
> +to
> +------------
> +1 --- 2 --- 4 --- 3 --- 5
> +------------
> +by moving the "pick 4" line will result in the following history:
> +------------
> +        3
> +       /
> +1 --- 2 --- 4 --- 5
> +------------
> +
>  Authors
>  ------
>  Written by Junio C Hamano <gitster@pobox.com> and
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index f20ea38..6668907 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -181,6 +181,12 @@ test_expect_success '-p handles "no changes" gracefully' '
>        test $HEAD = $(git rev-parse HEAD)
>  '
>
> +test_expect_failure 'exchange two commits with -p' '
> +       FAKE_LINES="2 1" git rebase -i -p HEAD~2 &&
> +       test H = $(git cat-file commit HEAD^ | sed -ne \$p) &&
> +       test G = $(git cat-file commit HEAD | sed -ne \$p)
> +'
> +
>  test_expect_success 'preserve merges with -p' '
>        git checkout -b to-be-preserved master^ &&
>        : > unrelated-file &&
> --
> 1.7.1
>
>
Jonathan Nieder· Jun 1, 2010, 08:50 UTC · re: Greg Price · lore

Re: [PATCH] rebase -i -p: document shortcomings

Hi Greg,
Greg Price wrote:
> The combination "rebase -i -p" does work correctly in one common use
> case:
[...]
> I suggest s/instruction sheet/todo list/, as the latter terminology
> seems more natural to me
Here’s a rough patch to squash in.  Thanks for the ideas.

Sleepily, Jonathan

Show changes to Documentation/git-rebase.txt +5 −4
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 330e9ea..e27099e 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -309,7 +309,7 @@ link:howto/revert-a-faulty-merge.txt[revert-a-faulty-merge How-To] for details).
 +
 This uses the `--interactive` machinery internally, but combining it
 with the `--interactive` option explicitly is generally not a good
-idea (see BUGS below).
+idea unless you know what you are doing (see BUGS below).
 
 
 --root::
@@ -614,9 +614,10 @@ case" recovery too!
 
 BUGS
 ----
-The instruction sheet presented by `--preserve-merges --interactive`
-does not represent the topology of the revision graph.  Attempts to
-reorder it tend to produce counterintuitive results.
+The todo list presented by `--preserve-merges --interactive` does not
+represent the topology of the revision graph.  Editing commits and
+rewording their commit messages should work fine, but attempts to
+reorder commits tend to produce counterintuitive results.
 
 For example, an attempt to rearrange
 ------------
-- 
1.7.1.246.g2fc1b.dirty
Sverre Rabbelier· Jun 1, 2010, 11:40 UTC · re: Greg Price · lore

Re: [PATCH] rebase -i -p: document shortcomings

Heya,
On Tue, Jun 1, 2010 at 04:22, Greg Price <price@ksplice.com> wrote:
> Thanks, this is long needed.  I regret that I haven't had time in
> several months to continue work on the real fix.
Is it your or Jonathan's intent on finishing Dscho's work on rebase -i -p?
-- 
Cheers,

Sverre Rabbelier
Greg Price· Jun 1, 2010, 17:24 UTC · re: Sverre Rabbelier · lore

Re: [PATCH] rebase -i -p: document shortcomings

On Tue, Jun 1, 2010 at 7:40 AM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
Show 5 quoted lines
> On Tue, Jun 1, 2010 at 04:22, Greg Price <price@ksplice.com> wrote:
>> Thanks, this is long needed.  I regret that I haven't had time in
>> several months to continue work on the real fix.
>
> Is it your or Jonathan's intent on finishing Dscho's work on rebase -i -p?

That was my plan. I did some work in that direction, which is at http://repo.or.cz/w/git/price.git/shortlog/refs/heads/rebase-i-p and which I discussed with Dscho last November: http://thread.gmane.org/gmane.comp.version-control.git/131921/focus=132156

Unfortunately work has made me busy with non-Git subjects since December. I would like to finish the work, but I cannot make promises.

Greg

← back to recent threads