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

From: Greg Price <price@ksplice.com>
Date: Jun 1, 2010, 02:22 UTC
Message-ID: <AANLkTimjcHpbbpfwNoZGq3_1TjfLAzJL3C8Jcp6LeyW0@mail.gmail.com>
In-Reply-To: <20100601014335.GA21970@progeny.tock>

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
>
>