BUG: git rebase -i -p silently looses commits

I have encountered what looks like critical bugs in the git rebase -i -p (it can be reproduced on mingw and cygwin, I have not tried other platforms).

Let's create a git repository with

git init # the next line is for mingw git config core.autocrlf input echo a >a.txt echo b >b.txt git add a.txt b.txt git commit -m "init commit" echo aa >a.txt git add a.txt git commit -m "aa commit" echo bb >b.txt git add b.txt git commit -m "bb commit" echo aaa >a.txt git add a.txt git commit -m "aaa commit"

Now let's use the following rebase command:

git rebase -i -p HEAD~3

When the editor will appear, just move the commit "bb commit" to the end of the list. The rebase process will complete successfully, but commit "aaa commit" will be missing from the history and working tree will not be affected by that commit.

Other bug is that if we move "bb commit" to the top of the list in the editor, the rebase process will apply "bb commit", but instead of applying "aa commit" and than "aaa commit", the rebase process fails with a merge conflict.

This can be reproduced with git 1.6.5.1 (msys) and 1.6.1.2 (cygwin). I consider these to be a critical bugs that make "-p" option extremely dangerous for interactive rebase. It might even make sense to disable it for interactive rebase until the bug is fixed.

Regards, Constantine

2009/11/2 Constantine Plotnikov <constantine.plotnikov@gmail.com>:

Doesnt -p ONLY work for interactive rebase?

       -p, --preserve-merges
           Instead of ignoring merges, try to recreate them. This
option only works in interactive mode.

Yves

On Mon, Nov 2, 2009 at 7:33 PM, demerphq <demerphq@gmail.com> wrote:

Yep, I forgot about it. But it does not seem to work correctly for interactive rebase either.

Constantine

Heya,

On Mon, Nov 2, 2009 at 17:18, Constantine Plotnikov <constantine.plotnikov@gmail.com> wrote:

Johannes has been working off and on on fixing git rebase -i -p to DTRT, perhaps this is related?

Hi,

On Mon, 2 Nov 2009, Constantine Plotnikov wrote:

rebase -i -p was never intended to reorder commits. In fact, the "-i" of it was only for convenience: I was more familiar with the code base of rebase -i than that of rebase.

Having said that, I worked for some time on fixing this issue, and I actually run a version of rebase -i -p here that allows reordering commits, but it is far from stable (and due to GSoC and day-job obligations, I had no time to work on it in months).

Ciao, Dscho

On Mon, 2 Nov 2009, Johannes Schindelin wrote:

I'm interested in this topic too.  Some weeks ago I took your
rebase-i-p branch from January and rebased it onto the latest release;
it's at
  git://repo.or.cz/git/price.git rebase-i-p
and now based on v1.6.5.2.  I fixed a few bugs and added a feature,
and it's the version I run day to day.

Constantine and others interested in reordering commits with -p, you're welcome to pull and build this version and try it out. It mostly solves my problems, and maybe it will solve yours. Be warned it does have bugs, and also be warned that I may rewind and rebase that branch. I'd be glad to hear about bugs you see, though.

Dscho, do you have a TODO written somewhere of what work you're aware the topic still needs? I plan to continue spending a little time working on it, and I have my own list but it'd be good to compare it with yours.

Cheers, Greg

PS - I'm open to suggestions on the workflow for how to develop a topic branch like this. Some rewinding seems necessary as half-baked patches get finished, etc, but if Dscho or someone else finds time to work on it too, then the rewinding gets in the way of pull/push collaboration.

PPS - For those just scanning along, the shortlog so far:

Greg Price (6):
      rebase -i -p: honor -s
      rebase -i -p: get full message from original merge commit
      rebase -i -p: always merge --no-ff
      rebase -i: Add the "ref" command
      rebase -i -p: Preserve author information on merges.
      [broken] rebase -i: implement pause

Johannes Schindelin (19):
      Some of Dscho's tools
      debug settings in Makefile
      Make CFLAGS more strict
      rebase -i --root: simplify code
      rebase -i: make pick_one() safer
      rebase -i -p: add helper parse_commit() to find rewritten commits
      rebase -i: add the "goto" command
      rebase -i -p: add a helper to add mappings for rewritten commits
      rebase -i -p: Add the "merge" command
      rebase -i: make sure that the commands record the rewritten commits
      rebase -i: move the code to write the rebase script into generate_script()
      rebase -i: let generate_script output to stdout
      rebase -i -p: refactor the preparation for -p into its own function
      rebase -i -p: use patch-id directly to determine the dropped commits
      rebase tests' fake-editor.sh: allow debugging with DEBUG_EDIT
      rebase's fake-editor: prepare for "goto" and "merge" commands
      rebase -i -p: generate a script using "goto" and "merge"
      TODO
      Make some tests in t3412 a little bit stricter

Hi,

On Wed, 4 Nov 2009, Greg Price wrote:

That is very interesting!

However, for rebase-i-p to have a chance to be accepted, I think a few things are necessary still (this is all from memory, so please take everything with a grain of salt):

- reorder the series to have the -i fixes first, the new commands next, 
  and then the changes to the actual -p mode

- rework the mark stuff so that 'todo' works properly, and then change the 
  system to use ':<name>' style bookmarks.

- fix that nasty bug which makes one revision not pass the tests (I forgot 
  which one, but it should be in the TODOs)

- add proper handling for the case when a patch has been applied in 
  upstream already, but was not correctly identified as that by 
  --cherry-pick (well, this TODO is actually not really related to rebase 
  -i -p, but something I deeply care about)

Unfortunately, I am getting more and more deprived of Git time budget these days, so that I cannot seem to find a few hours to at least restart my efforts.

Ciao, Dscho

On Wed, Nov 11, 2009 at 12:32 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Great, this is helpful, and it overlaps with my existing to-do list. I have a couple of questions.

This one will be easy when everything else is ready, I think.

This is the biggest change I was going to suggest!  Glad we're on the
same page.  To be clear, what I want to do here is
 - add a 'mark' command
 - emit 'mark' commands in the TODO generation for the target of each
'goto', and use them.
Is that also what you had in mind?

Hmm. I see one TODO comment in your patches, and it doesn't sound like this. Is there a TODO somewhere else that I'm missing? Alternatively, I can always end up just running the tests on all the revisions and find out.

Hmm. I'll have to think about what the behavior could be here. Unless you've already worked out a behavior you would like to see? For context, I think the issue you're referring to is that sometimes the patch-id changed, so that --cherry-pick doesn't identify the patch; and then some later upstream patch has touched the same code again, so that there's a conflict when we try to apply the older patch. I would also like to see this fixed, but I don't see offhand what the right behavior would be.

The "read my mind" behavior might be something like, somewhere between the merge-base and the upstream there is a commit after which this one would apply as no changes, so let's say that commit already applied this patch. But that could be the wrong thing if e.g. a patch was applied and later reverted. And I don't know offhand how to implement it efficiently.

Anyway, I think you're right that this improvement is orthogonal to rebase -i -p.

Understood.  I may have some time to work on this soon, we'll see.  I
think the priorities will be to
 - add "mark" as you say
 - add the "pause" command, to make it possible to amend a merge
 - write tests
 - fix a couple of bugs, track down the one you mentioned
 - write documentation

At that point, and with the reordering you suggested, I think it will be ready to submit for inclusion.

Further comments, and bug reports from anyone else using the development version, are welcome.

Thanks, Greg

Hi Greg,

On Thu, 12 Nov 2009, Greg Price wrote:

Almost. I called it "bookmark" so that the abbreviated command does not clash with "merge". And there are possible goto targets you have never been at:

- A - B - C
    \   /
      D

If C is your HEAD, and you "rebase -i -p B", before cherry-picking D, you have to "goto A".

So I strongly advise against trying to give all goto targets a name, just 
the obvious one: onto (you do not need upstream, as all the commits which 
have upstream as parent are supposed to be applied on top of onto anyway).
 

It should be in one commit message (something like WIP...).

At the moment, it gives you the status (which is multiple pages long here, due to untracked files that I am unwilling to move elsewhere) and then "nothing to commit". You do not even see which commit was to be applied.

My plan was to detect that condition in the error case and _not_ output what the cherry-pick printed, but a much more helpful message along the lines

	It appears "Bla bli blu" was already applied

For the exact message, I am sure all kinds of painters will want to help you.

Correct.

No, that is not what I mean. I mean when more than one context line changes. Then patch-ids will differ, but a 3-way merge will still succeed in finding that there was actually no change.

Thanks, Dscho

P.S.: I am mostly off-line until Monday.