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

[Ben Franksen]

Am 14.06.20 um 03:04 schrieb Ganesh Sittampalam:
> I just started playing with the latest on your branch, as follows:
> 
> - create these patches:
>  patch A: create 'file' with "initial content"
>  patch B: update 'file' with "line 1"
>  patch C: update 'file' with "line 2"
> 
> - suspend patch C
> 
> - amend patch B so that 'file' contains "line 1A"
> - amend patch B so that 'file' contains "line 1B"
> - amend patch B so that 'file' contains "line 1C"
> (of course this could continue, really two of the amendments are enough)
> 
> At this point looking in _darcs/rebase, we can see that each successive
> amendment is building up as a separate fixup. I'm not sure if this is a
> real concern or not but I find it a bit disquieting.

See below for a possible solution.

> - now unsuspend patch C
> 
> It unsuspends without any conflicts, putting "line 2" back into 'file'.
> I think this is wrong.

I agree that this is wrong.

I think this shows that pushing difference fixups is not quite the
correct solution for the amend/rebase interaction.

> This was pretty much the first example I tried, both now and when I was
> playing with it earlier (on 30th May), because at the back of my mind
> was a concern that if we drop coalesce, we can't avoid the build up of
> amendments. In your first implementation it didn't happen because you
> weren't pushing the amending diff, but now you are it does happen. As I
> say, I am not totally sure that this is a real concern, but it feels a
> bit wrong to me.
> 
> And if the problem with the conflict not being reported is fixed
> (assuming you agree that is a problem), then I worry that we would
> instead get a deeply nested conflict with each of the intermediate
> states. IMO the right behaviour is just to get a conflict between "line
> 1C" and "line 2".
> 
> 
> Looking at the bigger picture, we have three possible implementations of
> rebase:
> 
> A The original one before I turned everything into prims
> B The one where everything is prims (my recent impl)
> C The one where everything is potentially conflicted (the one currently
> being discussed)
> 
> I'm not clear if B is definitely superior to A or not. It fixes some
> crashes in A, but there are some cases where A produces "better"
> behaviour with conflicts than B, though perhaps more by luck than design.
> 
> C fixes some of the fundamental problems of B (suspending then
> unsuspending resolved conflicts can produce new conflicts) but I think
> so far introduces some potentially serious regressions from A or B. For
> the design, I have some reservations that the lack of coalescing may be
> fundamentally unworkable. But if the problems above are fixed and then I
> spend some time stress testing it, that would be enough to convince me
> my reservations are unfounded.

I think the considerations that led me to the last patch (see my
previous message) can be extended. To re-iterate, once a fixup gets
stuck we can rest assured that no other rebase item is in conflict with
it, so we know we do not have to keep its identity intact. This means we
are free to do more invasive editing (not just cancelling inverses),
like coalescing or even canonizing, without breaking any invariants. I
think this would address your concerns about the build-up of successive
fixups in the rebase state (and possibly complicated conflicts resulting
from this).

The other problem is somewhat more fundamental. Pushing inverted
differences gives us the expected results in the case of a move simply
because the move commutes past the suspended patch. But if we push
something that does not commute past, then this suppresses the fact that
we have a conflict here.

This should not surprise us: we know that if p^;q commutes (to, say,
q';p'^), then we can calculate merge(p\/q) as cleanMerge(p\/q)=(q'/\p').
But if p^;q does not commute, then this is invalid. In light of this,
there is an obvious solution: Instead of pushing the inverse difference,
we really have to /merge/ the (un-inverted) difference with the rebase
items, i.e. we have to call 'removeFixups diff suspended'.

> At some point I think we need to make a decision between these
> strategies for 2.16. We did survive with A for quite a while without it
> being a major headache, so if both B and C introduce some form of
> regressions compared to A we might actually be better going back to A
> for a release and then revisiting the design afterwards.

I agree that this is a viable option, but I'd like to see if we can't
fix the problems in C with the two changes I sketched above.

Cheers
Ben