Commit dropped when swapping commits with rebase -i -p

Hi,

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:

$ git clone -b git-bug --single-branch https://github.com/heremaps/scancode-toolkit.git
$ git rebase -i -p HEAD~2
# In the editor, swap the order of the two (non-merge) commits.
$ git diff origin/git-bug

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").

I'm observing this with Git 2.14.1 on Linux.

Regards,
Sebastian

On 30 August 2017 at 12:11, Sebastian Schuberth [off-list ref] wrote:

Hi,

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:

$ git clone -b git-bug --single-branch https://github.com/heremaps/scancode-toolkit.git
$ git rebase -i -p HEAD~2
# In the editor, swap the order of the two (non-merge) commits.
$ git diff origin/git-bug

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").

I'm observing this with Git 2.14.1 on Linux.

The man-page for git rebase says that combining -p with -i is "generally
not a good idea unless you know what you are doing (see BUGS below)".

Under BUGS, it says

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

So if you agree that a "dropped commit" is a "counterintuitive result",
this is known and documented. Maybe the warning could be harsher, but it
does say "unless you know what you are doing".

Martin

On Wed, Aug 30, 2017 at 8:07 PM, Martin Ågren [off-list ref] wrote:

The man-page for git rebase says that combining -p with -i is "generally
not a good idea unless you know what you are doing (see BUGS below)".

Thanks for pointing this out again. I remember to have read this some
time ago, but as I general consider myself to know what I'm doing, I
forgot about it :-)

Anyway, this should really more explicitly say *what* you need to know
about, that is, reordering commits does not work.

So if you agree that a "dropped commit" is a "counterintuitive result",
this is known and documented. Maybe the warning could be harsher, but it
does say "unless you know what you are doing".

I'd say it's worse than counterintuitive, as counterintuitive might
still be correct, while in my case it clearly is not. So yes, the
warning must be harsher in my opinion. Maybe we should even abort
rebase -i-p if reordering of commits is detected.

-- 
Sebastian Schuberth

Hi,

Sebastian Schuberth wrote:

On Wed, Aug 30, 2017 at 8:07 PM, Martin Ågren [off-list ref] wrote:

quoted

The man-page for git rebase says that combining -p with -i is "generally
not a good idea unless you know what you are doing (see BUGS below)".

Thanks for pointing this out again. I remember to have read this some
time ago, but as I general consider myself to know what I'm doing, I
forgot about it :-)

Heh.

Anyway, this should really more explicitly say *what* you need to know
about, that is, reordering commits does not work.

It tries to explain that, even with an example.  If you have ideas for
improving the wording, that would be welcome.

That said, ...

quoted

So if you agree that a "dropped commit" is a "counterintuitive result",
this is known and documented. Maybe the warning could be harsher, but it
does say "unless you know what you are doing".

I'd say it's worse than counterintuitive, as counterintuitive might
still be correct, while in my case it clearly is not. So yes, the
warning must be harsher in my opinion. Maybe we should even abort
rebase -i-p if reordering of commits is detected.

This sounds like a more promising approach.  If you can detect when
the rebase -i -p is going to cause trouble, then I would be all for
aborting.  If you want to be extra nice to people, you can provide a
--force escape valve to let them experience the broken behavior, but I
don't think that is necessary.

I also think a loud warning when -i -p is used even when it is not
going to cause trouble would be a valuable change.  E.g. maybe the
template that opens in the editor could say something about reordering
commits not being advisable?

E.g. I could imagine the todo list including some instructions in the
spirit of

	# git rebase --preserve-merges does not support reordering commits.
	# To attempt reordering anyway, add a line with the text "reorder".
	# It is not likely to behave as you expect.  You have been
	# warned.

Thanks,
Jonathan

On 2017-09-02 02:04, Jonathan Nieder wrote:

quoted

Anyway, this should really more explicitly say *what* you need to know
about, that is, reordering commits does not work.

It tries to explain that, even with an example.  If you have ideas for
improving the wording, that would be welcome.

As a first step, I indeed believe the wording must the stronger / clearer. How about this:

From f69854ce7b9359603581317d152421ff6d89f345 Mon Sep 17 00:00:00 2001
From: Sebastian Schuberth <redacted>
Date: Mon, 11 Sep 2017 10:41:27 +0200
Subject: [PATCH] docs: use a stronger wording when describing bugs with rebase -i -p

Signed-off-by: Sebastian Schuberth <redacted>
---
 Documentation/git-rebase.txt | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 6805a74aec..ccd0a04d54 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt

@@ -782,10 +782,11 @@ case" recovery too!
 
 BUGS
 ----
-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.
+Be careful when combining the `-i` / `--interactive` and `-p` /
+`--preserve-merges` options.  Reordering commits will drop commits from the
+main line. This is because the todo list does not represent the topology of the
+revision graph in this case.  However, editing commits and rewording their
+commit messages 'should' work fine.
 
 For example, an attempt to rearrange
 ------------

-- 
2.14.1.windows.1

Hi Sebastian,

On Wed, 30 Aug 2017, Sebastian Schuberth wrote:

I believe stumbled upon a nasty bug in Git: It looks like a commits gets
dropped during interactive rebase when asking to preserve merges.

Please see 'exchange two commits with -p' in t3404. This is a known
breakage, and due to the fact that -p and -i are fundamentally
incompatible with one another (even if -p's implementation was based on
-i's). I never had in mind for -p to be allowed together with -i, and was
against allowing it because of the design.

Short version: do not use -p with -i.

Ciao,
Johannes

On Wed, Aug 30, 2017 at 10:28 PM, Johannes Schindelin
[off-list ref] wrote:

Please see 'exchange two commits with -p' in t3404. This is a known

Thank for pointing out the test.

breakage, and due to the fact that -p and -i are fundamentally
incompatible with one another (even if -p's implementation was based on
-i's). I never had in mind for -p to be allowed together with -i, and was
against allowing it because of the design.

In any case, I wouldn't have expected *that* kind of side effect for
such a simple case (that does not involve any merge commits).

If these options are fundamentally incompatible as you say, would you
agree that it makes sense to disallow their usage together (instead of
just documenting that you should know what you're doing)?

-- 
Sebastian Schuberth

Hi Sebastian,

On Wed, 30 Aug 2017, Sebastian Schuberth wrote:

On Wed, Aug 30, 2017 at 10:28 PM, Johannes Schindelin
[off-list ref] wrote:

quoted

Please see 'exchange two commits with -p' in t3404. This is a known

Thank for pointing out the test.

quoted

breakage, and due to the fact that -p and -i are fundamentally
incompatible with one another (even if -p's implementation was based on
-i's). I never had in mind for -p to be allowed together with -i, and was
against allowing it because of the design.

In any case, I wouldn't have expected *that* kind of side effect for
such a simple case (that does not involve any merge commits).

If these options are fundamentally incompatible as you say, would you
agree that it makes sense to disallow their usage together (instead of
just documenting that you should know what you're doing)?

As I said already, I agreed with you before you said it, but I was
overruled.

Ciao,
Johannes