[darcs-devel] conflicted rebase (or rather: rebase with conflicted fixups)

Ben Franksen ben.franksen at online.de
Sun Jun 7 08:22:16 UTC 2020


Am 01.06.20 um 15:43 schrieb Ben Franksen:
> Am 01.06.20 um 10:59 schrieb Ben Franksen:
>> I see two ways to fix this.
>>
>> One is to restrict the special handling of "difference fixups" to
>> situations where this cannot happen. That is, we check that none of the
>> suspended items are in conflict with either the patch to be amended or
>> any earlier patches in our repo. Otherwise we fall back to the plain
>> "push old as fixups, then remove new as fixups" procedure (which does
>> *not* suffer from this problem exactly because it produced conflicts in
>> the rebase state and these do not block patch "2" from commuting past).
>>
>> Another fix could be to first push not only the patch to be amended (as
>> fixups), but also any of its predecessors that are in conflict with any
>> rebase item; and then compensate this by pushing their inverse effect.
>>
>> Both solutions require some effort to implement.
>>
>> Last not least, while thinking about this I discovered that the new
>> amend/rebase interaction has yet another failure mode, namely the case
>> when the patch to be amended is itself conflicted. Here the same problem
>> can arise: we first convert it to fixups (which preserves the conflicts)
>> but then we insert patches that may "separate" these fixups from the
>> patches that it conflicts with. I guess i should add a test for this
>> scenario.
> 
> I have pushed a patch that implements the first solution because it is
> simpler and more efficient, though a bit more restrictive. Also pushed a
> patches that solves the other problem mentioned.
> 
> All tests succeed now on my machine!

I found yet another failure mode for amend-rebase interaction. It was
inspired by your example in another thread:

> If we have patches X;Y where X creates A, Y changes A, then:
> 
>  * Suspend Y
>  * Move A to A' and record
>  * Create a fresh copy of A and record
>  * Unsuspend Y

I wanted to see what happens if I used amend and re-pull instead of
recording new patches:

 * ...
 * Move A to A' and amend X
 * Pull the original X from a clone
 * Unsuspend Y

This results in a crash for darcs-3 when we pull the original X:

| This is a bug! Please report it at http://bugs.darcs.net or via email
to bugs at darcs.net:
| merging identical patches is undefined
| CallStack (from HasCallStack):
|   error, called at src/Darcs/Patch/V3/Core.hs:179:9 in
darcs-2.15.2-inplace:Darcs.Patch.V3.Core

The problem is that this again violates the global invariant that common
patches (patches with the same identity in two branches) must be
commutable into a common context. Because the "difference fixups" that
amend pushes after pushing the fixups for the original X may prevent the
latter from commuting backward. When we pull the original X we convert
them to fixups and merge that with the rebase items:

  X-fixups \/ (amend-diff; X-fixups)

where the X-fixups in the rebase depend on the amend-diff.

BTW, the bug manifests in this way because we try to merge anyway, which
means the LHS first conflicts with the amend-diff and then we hit the
"same" fixup, but in an unconflicted state. So the patches have the same
identity but aren't equal according to =\/=.

I was quite desparate about this at first because I could see no
solution. But in fact there is one and it is quite simple and obvious.
We merely have to apply yet another deep rename to the rebase state.
Concretely, in the special case of amend after we push our difference
fixups, we have to rename the rebase state so that everything that
derives its name from the old name gets a fresh name. This is quite
similar to what we do when we suspend a patch.

The side conditions must remain in place, that is, we can push
difference fixups only if the patch to be amended is not conflicted and
there are no conflicted rebase items that conflict with regular patches.

As a side note, the ability to do the deep renames that are necessary
for this version of rebase to work soundly is a direct consequence of
PrimPatchIds being derived from the PatchInfo i.e. the PatchId of the
parent named patch. If PrimPatchIds were generated independently this
would be impossible! I think this demonstrates that deriving prim patch
IDs is not just a convenient implementation detail, but somewhat more
fundamental, at least for "advanced patch editing" operations like
rebase and amend.

Cheers
Ben



More information about the darcs-devel mailing list