rebase -p loses amended changes

rebase -p loses amended changes

[J Robert Ray]

[-- Attachment #1: Type: text/plain, Size: 885 bytes --]

I've found that 'rebase -p' can discard changes from a merge commit
when it recreates the merge.

If a merge is amended to add changes to a file unaffected by the
merge, these changes are lost after a rebase. Attached is a script to
demonstrate the problem.

I've tested the script against git 1.7.9.5.

I'll outline the steps starting from an empty repo.

1. Create a file (a) with some contents, then create three branches
from this point, "mod-a," "mod-b," and "master-copy."

2. In "mod-a," modify a.

3. In "mod-b," create a different file (b).

4. Merge (--no-ff) "mod-a" onto master.

5. Amend the merge with a modification to a.

6. Merge (--no-ff) "mod-b" onto "master-copy."

7. Rebase -p master onto "master-copy."

Notice the change to "a" from step #5 is now gone. "a" reverts to the
pre-amended version after step #4.

I assume this isn't expected behavior and is a bug.

[-- Attachment #2: rebase-p.sh --]
[-- Type: application/x-sh, Size: 834 bytes --]

Re: rebase -p loses amended changes

[Thomas Rast]

J Robert Ray <jrobertray@gmail.com> writes:

> If a merge is amended to add changes to a file unaffected by the
> merge, these changes are lost after a rebase. Attached is a script to
> demonstrate the problem.

> 4. Merge (--no-ff) "mod-a" onto master.
>
> 5. Amend the merge with a modification to a.

> 7. Rebase -p master onto "master-copy."
>
> Notice the change to "a" from step #5 is now gone. "a" reverts to the
> pre-amended version after step #4.

That's pretty much expected. rebase -p attempts to (conflicts will
happen again) replay the merge.  I don't think anybody's come up with a
clear idea of how to apply the conflicted or evil parts of the merge
mechanically.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: rebase -p loses amended changes

[Jon Seymour]

| My apologies to Thomas and Robert who also received my previous
attempts to send this to this list which were foiled by my iPad's
instance on sending HTML(via two different clients) and the mailing
list's (quite understandable) refusal to accept it.

On Sat, Mar 31, 2012 at 9:49 AM, Thomas Rast <trast@student.ethz.ch> wrote:
> J Robert Ray <jrobertray@gmail.com> writes:
>
>> If a merge is amended to add changes to a file unaffected by the
>> merge, these changes are lost after a rebase. Attached is a script to
>> demonstrate the problem.
> ...


>
> That's pretty much expected. rebase -p attempts to (conflicts will
> happen again) replay the merge.  I don't think anybody's come up with a
> clear idea of how to apply the conflicted or evil parts of the merge
> mechanically.
>

I wonder if there are any really good justifications for changing the
content, as distinct from the comments of a merge during an amendment?

If not, perhaps git could be a little bit noisy about the circumstance
at the point of the --amend commit?

jon.

Re: rebase -p loses amended changes

[Thomas Rast]

Jon Seymour <jon.seymour@gmail.com> writes:

> On Sat, Mar 31, 2012 at 9:49 AM, Thomas Rast <trast@student.ethz.ch> wrote:
>> J Robert Ray <jrobertray@gmail.com> writes:
>>
>>> If a merge is amended to add changes to a file unaffected by the
>>> merge, these changes are lost after a rebase. Attached is a script to
>>> demonstrate the problem.
>>
>> That's pretty much expected. rebase -p attempts to (conflicts will
>> happen again) replay the merge.  I don't think anybody's come up with a
>> clear idea of how to apply the conflicted or evil parts of the merge
>> mechanically.
>
> I wonder if there are any really good justifications for changing the
> content, as distinct from the comments of a merge during an amendment?

Semantic conflicts do not necessarily show up as
conflicts-to-be-resolved.  The canonical example is when you change the
signature of a function on one side of the merge, and introduce new
callers on the other side.  The merge must then patch all new callers
too.

> If not, perhaps git could be a little bit noisy about the circumstance
> at the point of the --amend commit?

That could still be done of course.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: rebase -p loses amended changes

[Jon Seymour]

On Sat, Mar 31, 2012 at 8:35 PM, Thomas Rast <trast@student.ethz.ch> wrote:
> Jon Seymour <jon.seymour@gmail.com> writes:
>
>> On Sat, Mar 31, 2012 at 9:49 AM, Thomas Rast <trast@student.ethz.ch> wrote:
>>
>> I wonder if there are any really good justifications for changing the
>> content, as distinct from the comments of a merge during an amendment?
>
> Semantic conflicts do not necessarily show up as
> conflicts-to-be-resolved.  The canonical example is when you change the
> signature of a function on one side of the merge, and introduce new
> callers on the other side.  The merge must then patch all new callers
> too.

Fair enough - I was thinking that you could these with a commit after
the merge, but I can see that's not the right thing to do, from a
correctness point of view.

jon.

Re: rebase -p loses amended changes

[J Robert Ray]

On Sat, Mar 31, 2012 at 2:39 AM, Jon Seymour <jon.seymour@gmail.com> wrote:
> On Sat, Mar 31, 2012 at 8:35 PM, Thomas Rast <trast@student.ethz.ch> wrote:
>> Jon Seymour <jon.seymour@gmail.com> writes:
>>
>>> On Sat, Mar 31, 2012 at 9:49 AM, Thomas Rast <trast@student.ethz.ch> wrote:
>>>
>>> I wonder if there are any really good justifications for changing the
>>> content, as distinct from the comments of a merge during an amendment?
>>
>> Semantic conflicts do not necessarily show up as
>> conflicts-to-be-resolved.  The canonical example is when you change the
>> signature of a function on one side of the merge, and introduce new
>> callers on the other side.  The merge must then patch all new callers
>> too.
>
> Fair enough - I was thinking that you could these with a commit after
> the merge, but I can see that's not the right thing to do, from a
> correctness point of view.

I thought there would be more concern about the silent data loss.
Instead of throwing away the amended changes I would prefer the rebase
to at least fail, if not have the problem require manual conflict
resolution.

A warning at the time of the amend could confuse or scare users. But a
mention of this problem in the rebase docs would help.

Re: rebase -p loses amended changes

[Jon Seymour]

On Wed, Apr 4, 2012 at 4:55 AM, J Robert Ray <jrobertray@gmail.com> wrote:
> On Sat, Mar 31, 2012 at 2:39 AM, Jon Seymour <jon.seymour@gmail.com> wrote:
>> On Sat, Mar 31, 2012 at 8:35 PM, Thomas Rast <trast@student.ethz.ch> wrote:
>>> Jon Seymour <jon.seymour@gmail.com> writes:
orrectness point of view.
>
> I thought there would be more concern about the silent data loss.
> Instead of throwing away the amended changes I would prefer the rebase
> to at least fail, if not have the problem require manual conflict
> resolution.
>
> A warning at the time of the amend could confuse or scare users. But a
> mention of this problem in the rebase docs would help.

It'd be quite expensive to attempt to detect cases where this might be
a problem, but let's articulate, as a thought experiment, what such a
solution might look like - I am not suggesting that it is a good idea,
but the thought experiment might be illustrative.

Suppose you have a merge of a A and B that produces M, which is then
amended as M'.

During rebase, you redo the merge A and B. If there are no conflicts,
then compare M with M' to produce D. You now have enough information
to reproduce the amended commit M' during a rebase (merge Ar and,Br
then apply D).

What isn't clear to me, in this thought experiment, is how you might
attempt to solve the case where there were syntactic (as opposed to
semantic) conflicts between the merge of A and B originally. You have
no idea, at that point, how the user originally resolved the conflicts
to produce M, thus no idea how to calculate D or even if there was a
non-trivial D.

In this circumstance, you stop and warn the user about the merge
conflict, but you have no good way (??) to warn them about the
potential loss of the amendment, D, since at this point you have no
clue whether there was such an amendment and you don't want to worry
them about conditions that usually do not occur (amended merges, I
would have thought, are quite rare).

??: does git rerere help at all, in this case, I wonder?

Certainly, there is no harm, I think, adding a note to the
documentation of --preserve-merges that states something like:

--preserve-merges does not attempt to preserve the resolution of merge
conflicts or the amendment of merge commits ???; any histories
containing such changes cannot be reliably recreated by git rebase.

??? - except to the extent allowed by git rerere?

jon.

Re: rebase -p loses amended changes

[Junio C Hamano]

Jon Seymour <jon.seymour@gmail.com> writes:

> It'd be quite expensive to attempt to detect cases where this might be
> a problem, but let's articulate, as a thought experiment, what such a
> solution might look like - I am not suggesting that it is a good idea,
> but the thought experiment might be illustrative.
>
> Suppose you have a merge of a A and B that produces M, which is then
> amended as M'.
>
> During rebase, you redo the merge A and B. If there are no conflicts,
> then compare M with M' to produce D. You now have enough information
> to reproduce the amended commit M' during a rebase (merge Ar and,Br
> then apply D).

Not necessarily.  You realized yourself a half of it.

> What isn't clear to me, in this thought experiment, is how you might
> attempt to solve the case where there were syntactic (as opposed to
> semantic) conflicts between the merge of A and B originally. You have
> no idea, at that point, how the user originally resolved the conflicts
> to produce M, thus no idea how to calculate D or even if there was a
> non-trivial D.

True.  The other half of the issue is that the merge heuristics may have
improved between the version of Git used to originally create M and the
version of Git used when you are attempting the rebase.  The original one
may have given a clean but incorrect merge, and your trial merge during a
rebase may have given a clean and correct merge.  The patch D may be the
right fix-up to apply to M, but it may not be if applied to M'.

In short, it is not "quite expensive", but is fundamentally impossible.

Re: rebase -p loses amended changes

[Phil Hord]

On Tue, Apr 3, 2012 at 5:43 PM, Jon Seymour <jon.seymour@gmail.com> wrote:
> On Wed, Apr 4, 2012 at 4:55 AM, J Robert Ray <jrobertray@gmail.com> wrote:
>> On Sat, Mar 31, 2012 at 2:39 AM, Jon Seymour <jon.seymour@gmail.com> wrote:
>>> On Sat, Mar 31, 2012 at 8:35 PM, Thomas Rast <trast@student.ethz.ch> wrote:
>>>> Jon Seymour <jon.seymour@gmail.com> writes:
> orrectness point of view.
>>
>> I thought there would be more concern about the silent data loss.
>> Instead of throwing away the amended changes I would prefer the rebase
>> to at least fail, if not have the problem require manual conflict
>> resolution.
>>
>> A warning at the time of the amend could confuse or scare users. But a
>> mention of this problem in the rebase docs would help.
>
> It'd be quite expensive to attempt to detect cases where this might be
> a problem, but let's articulate, as a thought experiment, what such a
> solution might look like - I am not suggesting that it is a good idea,
> but the thought experiment might be illustrative.
>
> Suppose you have a merge of a A and B that produces M, which is then
> amended as M'.
>
> During rebase, you redo the merge A and B. If there are no conflicts,
> then compare M with M' to produce D. You now have enough information
> to reproduce the amended commit M' during a rebase (merge Ar and,Br
> then apply D).

How does M' know it is an amended version of M?  When you amended the
commit M you threw away this linkage.

If you created M' as a new commit D instead, then I would agree that
you have enough information to do what you seek.  In fact, I'm pretty
sure git does this already.

Phil

Re: rebase -p loses amended changes

[Johannes Sixt]

Am 4/4/2012 0:09, schrieb Phil Hord:
> On Tue, Apr 3, 2012 at 5:43 PM, Jon Seymour <jon.seymour@gmail.com> wrote:
>> Suppose you have a merge of a A and B that produces M, which is then
>> amended as M'.
>>
>> During rebase, you redo the merge A and B. If there are no conflicts,
>> then compare M with M' to produce D. You now have enough information
>> to reproduce the amended commit M' during a rebase (merge Ar and,Br
>> then apply D).
> 
> How does M' know it is an amended version of M?  When you amended the
> commit M you threw away this linkage.
> 
> If you created M' as a new commit D instead, then I would agree that
> you have enough information to do what you seek.  In fact, I'm pretty
> sure git does this already.

IMO, it is a sub-optimal implementation of rebase -p that it attempts to
redo the merge. A better strategy is to just replay the changes between
the first parent and the merge commit, and then generate a new merge commit:

   git diff-tree -p M^ M | git apply --index &&
   git rev-parse M^2 > .git/MERGE_HEAD &&
   git commit -c M

This would side-step all the issues discussed here, no?

-- Hannes

Re: rebase -p loses amended changes

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> IMO, it is a sub-optimal implementation of rebase -p that it attempts to
> redo the merge. A better strategy is to just replay the changes between
> the first parent and the merge commit, and then generate a new merge commit:
>
>    git diff-tree -p M^ M | git apply --index &&
>    git rev-parse M^2 > .git/MERGE_HEAD &&
>    git commit -c M
>
> This would side-step all the issues discussed here, no?

Or cherry-pick the change made by the merge to its first parent, i.e.

	git cherry-pick -m 1 M

Re: rebase -p loses amended changes

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Sixt <j.sixt@viscovery.net> writes:
>
>> IMO, it is a sub-optimal implementation of rebase -p that it attempts to
>> redo the merge. A better strategy is to just replay the changes between
>> the first parent and the merge commit, and then generate a new merge commit:
>>
>>    git diff-tree -p M^ M | git apply --index &&
>>    git rev-parse M^2 > .git/MERGE_HEAD &&
>>    git commit -c M
>>
>> This would side-step all the issues discussed here, no?
>
> Or cherry-pick the change made by the merge to its first parent, i.e.
>
> 	git cherry-pick -m 1 M

Err, that was a confusing unfinished message.  I meant the step to replace
the part that uses pipe to "git apply", more like

        git rev-parse M^2 >.git/MERGE_HEAD &&
	git cherry-pick --no-commit -m 1 M &&
        git commit -c M

The primary difference is that, because "apply -3" is not implemented yet,
this will help when the base has drifted too much from the corresponding
blob recorded in M^.

Re: rebase -p loses amended changes

[Jon Seymour]

On Thu, Apr 5, 2012 at 6:26 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Johannes Sixt <j.sixt@viscovery.net> writes:
>>
>>> IMO, it is a sub-optimal implementation of rebase -p that it attempts to
>>> redo the merge. A better strategy is to just replay the changes between
>>> the first parent and the merge commit, and then generate a new merge commit:
>>>
>>>    git diff-tree -p M^ M | git apply --index &&
>>>    git rev-parse M^2 > .git/MERGE_HEAD &&
>>>    git commit -c M
>>>
>>> This would side-step all the issues discussed here, no?
>>
>> Or cherry-pick the change made by the merge to its first parent, i.e.
>>
>>       git cherry-pick -m 1 M
>
> Err, that was a confusing unfinished message.  I meant the step to replace
> the part that uses pipe to "git apply", more like
>
>        git rev-parse M^2 >.git/MERGE_HEAD &&
>        git cherry-pick --no-commit -m 1 M &&
>        git commit -c M
>
> The primary difference is that, because "apply -3" is not implemented yet,
> this will help when the base has drifted too much from the corresponding
> blob recorded in M^.
>

Ah, not so impossible after all :-).

Yeah, I know, you were talking specifically about the approach
suggested in my thought experiment.

It does seem like such an approach would yield an outcome much closer
to J Robert Ray's expectation.

Good suggestion, Hannes! Are there any flaws, I wonder?

jon.

Re: rebase -p loses amended changes

[Junio C Hamano]

Jon Seymour <jon.seymour@gmail.com> writes:

> Are there any flaws, I wonder?

It all depends on how close the base is to M^1.  The difference might make
it unnecessary (or even more necessary) to do any evil merge.

Re: rebase -p loses amended changes

[Jon Seymour]

On Thu, Apr 5, 2012 at 8:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jon Seymour <jon.seymour@gmail.com> writes:
>
>> Are there any flaws, I wonder?
>
> It all depends on how close the base is to M^1.  The difference might make
> it unnecessary (or even more necessary) to do any evil merge.

True. I guess it is still true there are no perfect solutions in this
area. This does seem like a step in a better direction, though.

jon.

Re: rebase -p loses amended changes

[Junio C Hamano]

Jon Seymour <jon.seymour@gmail.com> writes:

> On Thu, Apr 5, 2012 at 8:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jon Seymour <jon.seymour@gmail.com> writes:
>>
>>> Are there any flaws, I wonder?
>>
>> It all depends on how close the base is to M^1. ...
>
> True. I guess it is still true there are no perfect solutions in this
> area. This does seem like a step in a better direction, though.

The nice thing about Hannes's "replay the change between M^1 and M" is
that it sidesteps the fundamentally impossible part of your original idea,
which is to mechanically sift the "evil" part out of "pure merge" part
out.  Instead, it replays the change as a single ball of wax.

That difference between these two approaches may make the replaying of M
easier or harder, depending on how M^1 and your new base relate to each
other, so it is hard to say if it is really a step in a better direction.
Perhaps M is a merge made to the maint branch, and you may be replaying it
on top of a newer codebase based on the master track.  M^1 may lack many
changes the new base has, so replaying M by applying the patch between M^1
and M may give you a lot of conflict that you should not have to resolve
if you simply merged M^2 to the new base.  And the worst part of this
conflict resolution is that you cannot easily tell what the right solution
should be unless you look at M, M^1 and M^2---that forces you to sift the
"evil" part out of "pure merge" part in the original merge. The good news
is that this is *not* mechanical, as you have your own intelligence to
help that process ;-)

Re: rebase -p loses amended changes

[Martin von Zweigbergk]

On Tue, Apr 3, 2012 at 11:30 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> IMO, it is a sub-optimal implementation of rebase -p that it attempts to
> redo the merge. A better strategy is to just replay the changes between
> the first parent and the merge commit, and then generate a new merge commit:
>
>   git diff-tree -p M^ M | git apply --index &&
>   git rev-parse M^2 > .git/MERGE_HEAD &&
>   git commit -c M
>
> This would side-step all the issues discussed here, no?

Maybe. How would it handle the following, though?

With this history

          .-e-.
         /     \
      .-c---d---f
     /
a---b---g

, "git rebase -p --onto g b f" produces


              .-e'.
             /     \
a---b---g---c'--d'--f'

If the merge was interactive (or was made interactive due to merge
conflicts), e'-c' (the diff between c' and e') might be very different
from e-c. Creating f' by replaying f-d on top of d' would lose any
changes done in e'-c' as compared to e-c, no?

Sorry I this has already been said or if I'm missing the point; I have
not been following the conversion completely.

Martin

Re: rebase -p loses amended changes

[Jon Seymour]

>
> With this history
>
>          .-e-.
>         /     \
>      .-c---d---f
>     /
> a---b---g
>
> , "git rebase -p --onto g b f" produces
>
>
>              .-e'.
>             /     \
> a---b---g---c'--d'--f'
>

Perhaps there is a a formatting issue here, but wouldn't the resulting
history be, instead:

                  . -e'.
                 /     \
 a---b---g---c'--d'--f'

I don't really understand why e' - c' would be different to e-c?

jon.

Re: rebase -p loses amended changes

[Martin von Zweigbergk]

On Thu, Apr 5, 2012 at 9:33 AM, Jon Seymour <jon.seymour@gmail.com> wrote:
>>
>> With this history
>>
>>          .-e-.
>>         /     \
>>      .-c---d---f
>>     /
>> a---b---g
>>
>> , "git rebase -p --onto g b f" produces
>>
>>
>>              .-e'.
>>             /     \
>> a---b---g---c'--d'--f'
>>
>
> Perhaps there is a a formatting issue here, but wouldn't the resulting
> history be, instead:
>
>                  . -e'.
>                 /     \
>  a---b---g---c'--d'--f'

That's how it looked before I pasted it into gmail. Sorry about that
and thanks for clarifying.

> I don't really understand why e' - c' would be different to e-c?

Not "would", but "could", if you're using -i as well and choose the
"edit" action for commit e (or if merge conflict makes it halt while
creating e').

Re: rebase -p loses amended changes

[Johannes Sixt]

Am 4/5/2012 18:16, schrieb Martin von Zweigbergk:
> On Tue, Apr 3, 2012 at 11:30 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
>> IMO, it is a sub-optimal implementation of rebase -p that it attempts to
>> redo the merge. A better strategy is to just replay the changes between
>> the first parent and the merge commit, and then generate a new merge commit:
>>
>>   git diff-tree -p M^ M | git apply --index &&
>>   git rev-parse M^2 > .git/MERGE_HEAD &&
>>   git commit -c M
>>
>> This would side-step all the issues discussed here, no?
> 
> Maybe. How would it handle the following, though?
> 
> With this history
> 
>           .-e-.
>          /     \
>       .-c---d---f
>      /
> a---b---g
> 
> , "git rebase -p --onto g b f" produces
> 
> 
>               .-e'.
>              /     \
> a---b---g---c'--d'--f'
> 
> If the merge was interactive (or was made interactive due to merge
> conflicts), e'-c' (the diff between c' and e') might be very different
> from e-c. Creating f' by replaying f-d on top of d' would lose any
> changes done in e'-c' as compared to e-c, no?

If I understand correctly, rebase -p is not intended to move branchy
history to a new fork point (that would be the task of the still
hypothetical generic sequencer). Its purpose is only to keep the merges of
other "unrelated" topics, e.g.:

...--o--o        <- bugfix topic (needed by my-topic)
         \
    x--y--M--z   <- my-topic
   /
--A--o--o--B     <- master

to be turned into

...--o--o--------.        <- bugfix topic (needed by my-topic)
                  \
             x'-y'-M'-z'  <- my-topic
            /
--A--o--o--B              <- master

-- Hannes