Inconsistancy with `git rebase --preserve-merges`

Inconsistancy with `git rebase --preserve-merges`

[Robin Moussu]

Hi. I noticed that the position of the `--preserve-merges` option of
`git rebase` is significant (I think it shouldn't).

The following snippet doesn't preserve the merges:
```
$ git rebase --preserve-merges -i 412f07a~
pick 412f07a Work on dev branch
pick c6efccd Work on master branch
pick 71c8c37 Some work after the merge
```

Whereas this one does what I expect:
```
$ git rebase -i 412f07a~ --preserve-merges
pick 412f07a Work on dev branch
pick 616064c Merge branch 'master' into dev
pick 71c8c37 Some work after the merge
```

For reference:
```
$ git log --graph --oneline
* 71c8c37      (HEAD -> dev) Some work after the merge
*   616064c    Merge branch 'master' into dev
|\
| *   c6efccd  Work on master branch
| *   ... (more work on master)
* |   412f07a  Work on dev branch
* |   ... (more work on dev)
|/
* 4ee50cb Common ancestor
```

Step to reproduce:
```
mkdir temp
cd temp
git init
git commit --allow-empty -m 'Common ancestor'
git checkout -b dev
git commit --allow-empty -m 'Work on dev branch'
git tag some_commit
git checkout master
git commit --allow-empty -m 'Work on master branch'
git checkout dev
git merge master -m 'Merge branch 'master' into dev'
git commit --allow-empty -m 'Some work after the merge'
```
Then you will see that
    git rebase -i some_commit --preserve-merges
and
    git rebase --preserve-merges  -i some_commit
don't have the same output.

I am using git version 2.21.1 on Fedora 30.

Robin.

Re: Inconsistancy with `git rebase --preserve-merges`

[Kevin Daudt]

On Mon, Feb 24, 2020 at 02:10:07PM +0000, Robin Moussu wrote:
> Hi. I noticed that the position of the `--preserve-merges` option of
> `git rebase` is significant (I think it shouldn't).
> 
> The following snippet doesn't preserve the merges:
> ```
> $ git rebase --preserve-merges -i 412f07a~
> pick 412f07a Work on dev branch
> pick c6efccd Work on master branch
> pick 71c8c37 Some work after the merge
> ```
> 
> Whereas this one does what I expect:
> ```
> $ git rebase -i 412f07a~ --preserve-merges
> pick 412f07a Work on dev branch
> pick 616064c Merge branch 'master' into dev
> pick 71c8c37 Some work after the merge
> ```
> 
> For reference:
> ```
> $ git log --graph --oneline
> * 71c8c37      (HEAD -> dev) Some work after the merge
> *   616064c    Merge branch 'master' into dev
> |\
> | *   c6efccd  Work on master branch
> | *   ... (more work on master)
> * |   412f07a  Work on dev branch
> * |   ... (more work on dev)
> |/
> * 4ee50cb Common ancestor
> ```
> 
> Step to reproduce:
> ```
> mkdir temp
> cd temp
> git init
> git commit --allow-empty -m 'Common ancestor'
> git checkout -b dev
> git commit --allow-empty -m 'Work on dev branch'
> git tag some_commit
> git checkout master
> git commit --allow-empty -m 'Work on master branch'
> git checkout dev
> git merge master -m 'Merge branch 'master' into dev'
> git commit --allow-empty -m 'Some work after the merge'
> ```
> Then you will see that
>     git rebase -i some_commit --preserve-merges
> and
>     git rebase --preserve-merges  -i some_commit
> don't have the same output.
> 
> I am using git version 2.21.1 on Fedora 30.
> 
> Robin.
> 

Can you try `--rebase-merges` instead? Since v2.22.0 `--preseve-merges`
is officially deprecated, but even before that it was already known to
have flaws.

Kevin

Re: Inconsistancy with `git rebase --preserve-merges`

[Junio C Hamano]

Kevin Daudt <me@ikke.info> writes:

>> $ git rebase --preserve-merges -i 412f07a~
>
> Can you try `--rebase-merges` instead? Since v2.22.0 `--preseve-merges`
> is officially deprecated, but even before that it was already known to
> have flaws.

The options to "rebase" command seem to follow the usual "last one
wins" pattern.  There is a single opts->type field, and upon seeing
--preserve-merges, the field gets assigned REBASE_PRESERVE_MERGES,
and then when the command line option parser sees "-i", the field
gets overwritten by assigning REBASE_INTERACTIVE.  So

    git rebase --preserve-merges -i 412f07a~
    git rebase -i 412f07a~

both should do the same thing.

I suspect that "--rebase-merges -i" may hide the real issue (which
is "the command line option parsing of rebase is messy"), but it may
deserve to be cleaned up, now that the result of "rewrite it in C"
efforts seems to have sufficiently stablized.

What to clean-up?  There could be two-and-half ways to view it:

 * The options should follow the usual "last one wins" pattern; if
   we take this stance, what happens with "--rebase-merges -i" is
   buggy and "--preserve-merges -i" that behaves as a mere "-i" is
   doing the right thing.  The part of the command line parser that
   implements "--rebase-merges" should be fixed so that its effect
   gets reverted when "-i" is seen later.

 * The options "--rebase-merges" and "--perserve-merges" (there may
   be others) and "--interactive" should be mutually exclusive; if
   we take this stance, we should error out when we see two or more
   of them on the command line.

 * The options "--rebase-merges" and "--preserve-merges" (there may
   be others) should be mutually exclusive or "last one wins", but
   "--interactive" can be combined with them to make them
   interactive.

I am not sure which one is the best.  The impression I got from the
current state of the code is that it started from "last one wins",
and the code wanted to transition to "--interactive makes other
options go interactive" but hasn't done a good job by leaving loose
ends (like what we saw with "--preserve-merges"), so perhaps the
last one is what we want to aim as a long term solution?  I dunno.