[darcs-devel] [patch1826] v3: resolve conflicts

Ben Franksen bugs at darcs.net
Mon Jul 15 22:58:23 UTC 2019


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

>>   * change the type of resolveConflicts
> 
>>   It now gets two RLs of patches as input and produces a simple (not nested)
>>   list of resolutions. The change in the input type(s) has been done because
>>   otherwise a RepoPatchV3 cannot correctly implement resolveConflicts, which
>>   requires that we know the transitive set of conflicting patches for each
>>   conflict. But a V3 conflictor contains only the patches that directly
>>   conflict. The separation into two input RLs is so that we can still resolve
>>   only the conflicts inside a (trailing) segment of all patches in a repo,
>>   which is how we call it when merging patches.
>>   
>>   There are no longer instances of class Conflict for RLs and FLs. Instead, we
>>   offer the stand-alone function combineConflicts and use that in the
>>   implementation of resolveConflicts for RepoPatchV1/2.
> 
> It took me a little while to understand that this was effectively a
> shift in level: where previously we were calling the resolveConflicts
> instance for RL RepoPatch now we are calling it for RepoPatch.

Exactly. It was awfully hard and took lots of experimental changes I had
to throw away before I got to this point. A bit like re-discovering
surgery: you gotta cut exactly this way and then that way and then you
can fit together the pieces without killing the patient...

> But the
> end result seems reasonable/an improvement.

And I think with your refactor of the result data type (which in my
rebased version I took over mostly unchanged) and my further refactors
on top of that the whole interface slowly approaches "understandable for
mere humans".

>>   The change in the result type is just a cleanup: instead of adding the
>>   mangled resolutions as a first element and then taking the head (in
>>   standardResolution) we now replace the inner list with the mangled version.
> 
> OK. I don't find the name particularly intuitive but the comment is
> clear enough.

Which name? combineConflicts? That was just the first thing I could
think of, sorry. Please fix if you have a better one.

>>   Passing the full context to resolveConflicts requires a number changes
>>   downstream. This is not strictly needed for V1/V2 which ignore the context,
>>   so we could pass undefined, but we need to make this change for V3 anyway.
>>   Instead of adding yet another parameter to all functions involved, we now
>>   pass a (Fork common us them) which cleans up type signatures (and
>>   incidentally some of the code, too).
> 
> This feels like a significant complication (e.g. how does it interact
> with lazy repos), but if it's necessary for V3 then I guess we have to
> live with it.

Yes, passing the common part around is a bit of complication and it is
unfortunate that we cannot avoid that. I tried to mitigate by passing a
Fork instead of adding yet another parameter.

There is no loss of lazyness, fortunately. Calculating the outermost
layer of the common PatchSet doesn't access patch contents at all and
thus works for lazy repos just as before. We don't even have to read all
inventories, only those after the latest common clean tag. Oh, and
PatchSet2RL is also fully lazy, evaluating nothing until you access its
patches and then only the outermost inventory etc.

I am also convinced that findUncommon and findCommonWithThem could both
be redefined as specializations of findCommonAndUncommon without any
loss of efficiency or laziness.

>> {-
>> Keep this code as a reminder that we want to QC test conflict resolution.
>> It doesn't work like this anymore, though, we need a full repo. Also should
>> do it generically for all patch types i.e. in Darcs.Test.Patch.Properties.Gewneric.
> 
> I guess this should at least have a TODO/V3INTEGRATION comment.

Yes, definitely. I had completely forgotten about this out-commented code.

> I'm also
> generally in favour of leaving code disabled rather than commented out
> so it keeps compiling, but that's not very important in this case as the
> code is just an indication of how a future test might look.

I have written this test a while ago (not yet sent) and it certainly did
uncover some bugs in my conflict resolution code for V3. (So I did not
forget we need the test, just the old code and my comment.) The
commented code can be deleted; I will send that along with the new tests.

BTW, I also added a property that says conflict resolutions are
independent of the ordering of patches; to my astonishment it seemed to
hold up for all 3 RepoPatch types. I investigated and found that indeed
V1 and V2 sort or nubSort the unravelled conflicts. And V3 does that
too, though not by calling nubSort but rather because it gathers them in
a Set to begin with.

> The change in the signature of tentativelyMergePatches might be
> helpfully accompanied by an update to the comment that describes the
> patches and their relationships.

Right, I missed that. Will fix.

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


More information about the darcs-devel mailing list