[darcs-devel] [patch1902] add failing test for rebase of conflicted patches

[Ganesh Sittampalam]

Ganesh Sittampalam <ganesh at earth.li> added the comment:

> I had to pipe one more 'y' into the suspend commands. Then the test
> indeed succeeded! Good work so far.

FWIW they succeed as-is with the more recent change I sent, as it
doesn't introduce unexpected extra patches.

> I looked at what unwind does for v3 and I think it makes a lot of sense.
> Your idea to separate the conflicted effect into three sections (fixups
> before; the actual patch; fixups after) is ingenious.

Just following the types :-) (Which took a while for V1 mergers...)

I first tried using Sealed and just producing fixups_before ; actual.
But it soon became clear that the fixups_after are needed to compose the
unwound patches.

> Your unwind doesn't look at the set of conflicts in a conflictor at all.
> This could turn out to be problematic for more complicated conflicts.
> For instance, the last conflictor of a three-way conflicted merge has an
> empty effect.

I did think about that, but my intuition is that it doesn't actually
matter. The point of the unwind is to preserve the actual underlying
patch, with proper context so we can slot it into a larger sequence. The
set of conflicts that currently have no effect is irrelevant to that,
and only needed for the commute behaviour of the original conflictor.

Once we suspend a patch, we don't care about that commute behaviour;
when we unsuspend we'll have a new unconflicted patch with different
commute behaviour. (So rebase will continue to be a fundamentally lossy
operation.)

I could include the set of conflicts with the p;p^ trick (which is
indeed very useful for thinking about conflicts), but it'd just
disappear as soon as the fixups were simplified anyway.

> I will try to extend the test to cover this scenario, too, My guess is
> that it will not give the expected result yet.

That'd be great in case I'm wrong and would give us better coverage anyway.

> I guess in the end you'll have to bite the bullet and more or less
> reflect the structure of what is in a Named patch a bit more faithfully.

I did manage to avoid this at least for simple cases in my followup. And
I have an informal proof that it must be _possible_ to unwind an entire
Named into a single sequence fixups_before;actuals;fixups_after.

Consider that the Named was originally recorded in a context where all
its constituents were unconflicted. To simplify things, consider its
minimal context where it would also be completely unconflicted. That
minimal context is a subset of our current repository.

Define

context_diff = the patches in the current context (without the Named we
are suspending) minus the patches in the minimal context

tosuspend_minimal = the Named we are suspending in its minimal context
tosuspend = the Named in its current context

fixups_before = effect(context_diff^)
actuals = tosuspend_minimal
fixups_after = tosuspend_minimal^;effect(context_diff);effect(tosuspend)

In practice many of the fixups would be unnecessary so the unwound
sequence could be much smaller. But the difficulty is finding an
algorithm that can produce it without traversing the whole repository
history, and being confident it is reliable.

So far what I have is in the Unwind instance for FL, but it's pretty
hacky and I really don't want it to depend on coalesce/canonize; I would
like it to work just by cancelling inverses and commuting, in a way that
isn't dependent on the particular order we do it.

> That is, ToEdit will have to consist of a PatchInfo plus explicit
> dependencies, plus a /sequence/ of (fixup;patch), not just a single one.

> This could be done by embedding a Named (RebasePatch prim), where
> (RebasePatch prim) lives at the same level as (RepoPatchVx prim) and
> consists of (fixups;prim) or perhaps even (fixups;prim;fixups). The
> latter would be particularly elegant, as unwind would then become a
> patch converter:
> 
>   unwind : p wX wY -> RebasePatch (PrimOf p) wX wY

Hmm, yes, that is a good alternative strategy. It might even make the
rebase code simpler as currently we end up making things like
RebaseSelect and RebaseChanges essentially to work around the fact that
we don't have FL Named in the rebase.

> I think this would also free you from having to create random junk on
> the fly.

I think doing that is completely unviable, I just did it initially to
see if I could get something halfway working. It's gone now and is
staying gone :-) The UI implications of presenting the user with
multiple pieces of their original patch, each in their own suspended
patch with apparently identical names, are just too horrible.

__________________________________
Darcs bug tracker <bugs at darcs.net>
<http://bugs.darcs.net/patch1902>
__________________________________