threads / discuss / 30117

Re: rebase -p loses amended changes

Subject: Re: rebase -p loses amended changes

## tl;dr

18 messages between Mar 31, 2012 and Apr 10, 2012.

replies: 17people: 7as markdown or json

Jon Seymour· Mar 31, 2012, 05:55 UTC · lore
| 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:
Show 6 quoted lines
> 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.
> ...
Show 6 quoted lines
>
> 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.
Thomas Rast· Mar 31, 2012, 09:35 UTC · re: Jon Seymour · lore
Jon Seymour <jon.seymour@gmail.com> writes:
Show 14 quoted lines
> 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
Jon Seymour· Mar 31, 2012, 09:39 UTC · re: Thomas Rast · lore
On Sat, Mar 31, 2012 at 8:35 PM, Thomas Rast <trast@student.ethz.ch> wrote:
Show 12 quoted lines
> 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.
J Robert Ray· Apr 3, 2012, 18:55 UTC · re: Jon Seymour · lore
On Sat, Mar 31, 2012 at 2:39 AM, Jon Seymour <jon.seymour@gmail.com> wrote:
Show 17 quoted lines
> 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.

Jon Seymour· Apr 3, 2012, 21:43 UTC · re: J Robert Ray · lore
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.
Show 8 quoted lines
>
> 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.
Junio C Hamano· Apr 3, 2012, 22:02 UTC · re: Jon Seymour · lore
Jon Seymour <jon.seymour@gmail.com> writes:
Show 12 quoted lines
> 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.
Show 6 quoted lines
> 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.
Phil Hord· Apr 3, 2012, 22:09 UTC · re: Jon Seymour · lore
On Tue, Apr 3, 2012 at 5:43 PM, Jon Seymour <jon.seymour@gmail.com> wrote:
Show 26 quoted lines
> 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
Johannes Sixt· Apr 4, 2012, 06:30 UTC · re: Phil Hord · lore
Am 4/4/2012 0:09, schrieb Phil Hord:
Show 15 quoted lines
> 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
Junio C Hamano· Apr 4, 2012, 17:11 UTC · re: Johannes Sixt · lore
Johannes Sixt <j.sixt@viscovery.net> writes:
Show 9 quoted lines
> 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
Junio C Hamano· Apr 4, 2012, 20:26 UTC · re: Junio C Hamano · lore
Junio C Hamano <gitster@pobox.com> writes:
Show 15 quoted lines
> 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^.

Jon Seymour· Apr 4, 2012, 22:55 UTC · re: Junio C Hamano · lore
On Thu, Apr 5, 2012 at 6:26 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 29 quoted lines
> 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.
Junio C Hamano· Apr 4, 2012, 22:59 UTC · re: Jon Seymour · lore
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.

Jon Seymour· Apr 4, 2012, 23:05 UTC · re: Junio C Hamano · lore
On Thu, Apr 5, 2012 at 8:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 6 quoted lines
> 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.
Junio C Hamano· Apr 4, 2012, 23:20 UTC · re: Jon Seymour · lore
Jon Seymour <jon.seymour@gmail.com> writes:
Show 9 quoted lines
> 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 ;-)

Martin von Zweigbergk· Apr 5, 2012, 16:16 UTC · re: Johannes Sixt · lore
On Tue, Apr 3, 2012 at 11:30 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
Show 9 quoted lines
> 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
Jon Seymour· Apr 5, 2012, 16:33 UTC · re: Martin von Zweigbergk · lore
Show 16 quoted lines
>
> 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.
Martin von Zweigbergk· Apr 5, 2012, 16:43 UTC · re: Jon Seymour · lore
On Thu, Apr 5, 2012 at 9:33 AM, Jon Seymour <jon.seymour@gmail.com> wrote:
Show 23 quoted lines
>>
>> 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').

Johannes Sixt· Apr 10, 2012, 07:14 UTC · re: Martin von Zweigbergk · lore
Am 4/5/2012 18:16, schrieb Martin von Zweigbergk:
Show 32 quoted lines
> 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

← back to recent threads