BUG: git rebase -i -p silently looses commits

[Constantine Plotnikov]

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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[demerphq]

2009/11/2 Constantine Plotnikov <[hidden email]>:

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

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


--
perl -Mre=debug -e "/just|another|perl|hacker/"
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Sverre Rabbelier-2]

Heya,

On Mon, Nov 2, 2009 at 17:18, Constantine Plotnikov
<[hidden email]> wrote:
> 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).

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

--
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Constantine Plotnikov]

On Mon, Nov 2, 2009 at 7:33 PM, demerphq <[hidden email]> wrote:
> 2009/11/2 Constantine Plotnikov <[hidden email]>:

> 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.
>
Yep, I forgot about it. But it does not seem to work correctly for
interactive rebase either.

Constantine
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Johannes Schindelin]

Hi,

On Mon, 2 Nov 2009, Constantine Plotnikov wrote:

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

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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Greg Price]

On Mon, 2 Nov 2009, Johannes Schindelin wrote:
> 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).

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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Johannes Schindelin]

Hi,

On Wed, 4 Nov 2009, Greg Price wrote:

> On Mon, 2 Nov 2009, Johannes Schindelin wrote:
> > 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).
>
> 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.

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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Greg Price]

On Wed, Nov 11, 2009 at 12:32 PM, Johannes Schindelin
<[hidden email]> 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):

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


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

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


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

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?


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

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.


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

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.


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

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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Johannes Schindelin]

Hi Greg,

On Thu, 12 Nov 2009, Greg Price wrote:

> On Wed, Nov 11, 2009 at 12:32 PM, Johannes Schindelin
> <[hidden email]> 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):
>
> Great, this is helpful, and it overlaps with my existing to-do list. I
> have a couple of questions.
>
> > - rework the mark stuff so that 'todo' works properly, and then change
> >   the  system to use ':<name>' style bookmarks.
>
> 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?

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).
 
> > - fix that nasty bug which makes one revision not pass the tests (I
> >   forgot  which one, but it should be in the TODOs)
>
> 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.

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

> > - 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)
>
> Hmm.  I'll have to think about what the behavior could be here.

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.

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

Correct.

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

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.

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

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