Commit dropped when swapping commits with rebase -i -p

Commit dropped when swapping commits with rebase -i -p

[Sebastian Schuberth]

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

Re: Commit dropped when swapping commits with rebase -i -p

[Martin Ågren]

On 30 August 2017 at 12:11, Sebastian Schuberth <sschuberth@gmail.com> 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

Re: Commit dropped when swapping commits with rebase -i -p

[Sebastian Schuberth]

On Wed, Aug 30, 2017 at 8:07 PM, Martin Ågren <martin.agren@gmail.com> 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

Re: Commit dropped when swapping commits with rebase -i -p

[Johannes Schindelin]

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

Re: Commit dropped when swapping commits with rebase -i -p

[Sebastian Schuberth]

On Wed, Aug 30, 2017 at 10:28 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> 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

Re: Commit dropped when swapping commits with rebase -i -p

[Johannes Schindelin]

Hi Sebastian,

On Wed, 30 Aug 2017, Sebastian Schuberth wrote:

> On Wed, Aug 30, 2017 at 10:28 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> 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)?

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

Ciao,
Johannes

Re: Commit dropped when swapping commits with rebase -i -p

[Jonathan Nieder]

Hi,

Sebastian Schuberth wrote:
> On Wed, Aug 30, 2017 at 8:07 PM, Martin Ågren <martin.agren@gmail.com> 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 :-)

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

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

Re: Commit dropped when swapping commits with rebase -i -p

[Sebastian Schuberth]

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

>> 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 <sschuberth@gmail.com>
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 <sschuberth@gmail.com>
---
 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

Re: Commit dropped when swapping commits with rebase -i -p

[Junio C Hamano]

Sebastian Schuberth <sschuberth@gmail.com> writes:

> On 2017-09-02 02:04, Jonathan Nieder wrote:
>
>>> 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 <sschuberth@gmail.com>
> 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 <sschuberth@gmail.com>
> ---
>  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
>  ------------


Anybody?  I personally feel that the updated text is not all that
stronger but it is clearer by clarifying what "counterintuitive
results" actually mean, but I am not the target audience this
paragraph is trying to help, nor I am the one who is making excuse
for a known bug, so...

Re: Commit dropped when swapping commits with rebase -i -p

[Andreas Heiduk]

Am 15.09.2017 um 22:52 schrieb Junio C Hamano:
> Sebastian Schuberth <sschuberth@gmail.com> writes:
>>
>> 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` /

"Be careful" is not necessary because the text is already in the "BUGS"
section.

>> +`--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
>>  ------------
> 
> 
> Anybody?  I personally feel that the updated text is not all that
> stronger but it is clearer by clarifying what "counterintuitive
> results" actually mean, but I am not the target audience this
> paragraph is trying to help, nor I am the one who is making excuse
> for a known bug, so...
> 

For me the proposed wording implies that the only bad effect are dropped
commits on the mainline. But I experienced something like this:


O--O--O--O---M--O        ==>   O--O--O--O---M--O
 \          /                   \          /
  O--X--O--O                     O--X     O


Where X was a commit without a ref and hence lost. Also the merge commit
seemed to combine two unrelated histories.

Therefore I would avoid "definitive wording" like "will drop" and use
vague wording along "there are various dragons out there" like this:

    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 reordering, combining or dropping commits of a complex topology
    can produce unexpected and useless results like missing commits,
    wrong merges, merges combining two unrelated histories and
    similar things.

Re: Commit dropped when swapping commits with rebase -i -p

[Sebastian Schuberth]

On Sat, Sep 16, 2017 at 12:41 PM, Andreas Heiduk <asheiduk@gmail.com> wrote:

> Therefore I would avoid "definitive wording" like "will drop" and use
> vague wording along "there are various dragons out there" like this:
>
>     The todo list presented by `--preserve-merges --interactive` does
>     not represent the topology of the revision graph.  Editing

I tried to avoid this introducing sentence from the original wording
as it reads like from a scientific research paper instead of from a
user's manual.

>     commits and rewording their commit messages should work fine.
>     But reordering, combining or dropping commits of a complex topology

There is no need for complex topology. If you reorder the two most
recent commits in a linear history, one gets dropped.

>     can produce unexpected and useless results like missing commits,
>     wrong merges, merges combining two unrelated histories and
>     similar things.

"can produce" is much too soft, IMO. Reordering commits goes wrong,
period. Like wise "unexpected and useless results" is inappropriate.
The results are wrong in case of reordering, and wrong results are of
course unexpected and useless.

-- 
Sebastian Schuberth

Re: Commit dropped when swapping commits with rebase -i -p

[Phillip Wood]

On 16/09/17 14:45, Sebastian Schuberth wrote:
> 
> On Sat, Sep 16, 2017 at 12:41 PM, Andreas Heiduk <asheiduk@gmail.com> wrote:
> 
>> Therefore I would avoid "definitive wording" like "will drop" and use
>> vague wording along "there are various dragons out there" like this:
>>
>>      The todo list presented by `--preserve-merges --interactive` does
>>      not represent the topology of the revision graph.  Editing
> 
> I tried to avoid this introducing sentence from the original wording
> as it reads like from a scientific research paper instead of from a
> user's manual.
> 
>>      commits and rewording their commit messages should work fine.
>>      But reordering, combining or dropping commits of a complex topology
> 
> There is no need for complex topology. If you reorder the two most
> recent commits in a linear history, one gets dropped.
> 
>>      can produce unexpected and useless results like missing commits,
>>      wrong merges, merges combining two unrelated histories and
>>      similar things.
> 
> "can produce" is much too soft, IMO. Reordering commits goes wrong,
> period. Like wise "unexpected and useless results" is inappropriate.
> The results are wrong in case of reordering, and wrong results are of
> course unexpected and useless.

I agree that the wording needs to be explicit that bad things will 
happen. It should spell out that if commits or reordered, or the fixup 
or squash commands are used then commits will be dropped and if commits 
are deleted from the list or the drop command is used other commits 
other than the intended ones will be dropped as well.