[darcs-devel] [patch1701] refactorings in the Darcs.Patch subsystem

[Ben Franksen]

Ben Franksen <ben.franksen at online.de> added the comment:

> One of the things that's making it harder for me to understand them is the
> idea that a conflict is between a *sequential* pair of patches (p:>q).
> I've always thought of conflicts as between a *parallel* pair of patches
> (p:\/:q).

I had the same difficulty at first. It was the reason I started with
this because I did not understand what "commuteNoConflicts" was supposed
to mean.

Of course a conflict first arises between a parallel pair. But then
Darcs comes along and merges them anyway, and the result is a sequential
pair with one of the patches, say p, in its original form, and the other
one in the form of a conflictor. If presented in this form, a sequential
pair can indeed be in conflict. The conflictor even tells us /which/
patches (in its own past) it conflicts with.

> I know the language you used is somewhat implied by the fact that
> commuteNoConflicts is the one that actually has a definition. But do
you think
> it'd make sense to formulate as something like
> "commuteNoConflicts (p:>q) fails when commute (p:>q) succeeds in the case
> that (p:>q) results from a conflicted merge"?

Yes, that is perhaps a better way to express it. What I wanted to
express is what it means for a /sequential/ pair to be in conflict (as
opposed to a parallel pair).

I still think we should clearly state somewhere that a conflict shows up
in two different forms: before the merge as a parallel pair, after merge
as a sequential pair. Creating a conflictor doesn't magically make the
conflict go away, it's still there and needs to be resolved!

> Maybe further down the line we could also make mergeNoConflicts be the
> primitive operation.

Hmm. commuteNoConflicts is used independently from mergeNoConflicts e.g.
as an optimization for resolveConflicts (for V2 conflictors, at least).
I doubt that commuteNoConflicts can be expressed in terms of
mergeNoConflicts. Also, commuteNoConflicts does /not/ mean that it
doesn't handle conflictors: it does, but it succeeds only for cases
where the patches are unrelated i.e. all the ingredients commute
naturally. (BTW, this is much clearer to see in the camp theory: for the
unrelated cases, the conflicts stay where they are, whereas for the
cases resulting form a conflicted merge, conflicts move from one patch
to the other.)

>>  * improved haddocks for CommuteNoConflicts
>
> Apart from the definition of 'conflict', this is fine. The new
documentation is
> a bit ambiguous about what happens with composite patches: the answer
is that the
> commute fails if any of the individual patch commutes fail.

As this works exactly analogous as for commute I did not think of
mentioning it explicitly.

>>  * replace naturalMerge with mergeNoConflicts
>
> I need to keep thinking about the comment associated with this change.

I fought with this for a long time. This is perhaps the most difficult
to understand part of patch theory. The scenario I describe in the
comment is hard to follow unless you draw yourself a picture. The point
is that the non-conflicting merge exploits the square-commute law, but
this law breaks down (in general) when we add conflictors, more
specifically, it breaks down in the case of a conflicted merge. If we
used the general commute for mergeNoConflicts (and without the extra
side-condition that was previously there), then we would use the
square-commute law in a situation where it no longer applies.

>>  * add laws to classes Commute and CommuteNoConflicts
>
> It would be nice if these laws had associated tests.

Definitely.

>> +-- * If an instance @'Commute' p@ exists, then
>
> Should we just add Commute as a superclass of CommuteNoConflicts?
> Even though it's not necessary for the implementation I don't think it's
> useful not to have it.

I don't think a Patch type p with CommuteNoConflicts p but not Commute p
makes much sense, so yes. (I still have a slightly bad feeling about
this but I can't explain why, exactly.)

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