1 patch for repository /home/ben/src/darcs/sent:

patch 6214e01928baab9123f03566f12d15e91d86c3f5
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Feb 26 23:29:10 CET 2019
  * fix a bug in darcs1->darcs-2 conversion
  
  The test data for threewayanddep threewayandmultideps were quite obviously
  wrong! The darcs-2 Conflictors are complete bogus, referring to patches that
  don't appear in the repo. This is caused by erroneous calls to
  sortCoalesceFL in the RepoPatchV1 implementation in unravel and in effect.
  The way these functions are normally used (effect when we apply a patch and
  unravel to generate conflict markup) is quite tolerant wrt coalescing.
  However, unravel is also used to convert darcs-1 Mergers to darcs-2
  Conflictors, and here the result is catastrophic. Instead of sortCoalesceFL
  we must merely cancel inverses, just like we do in the darcs-2 and darcs-3
  theory when we construct contexted patches (aka Nons).

Attachments

• fix-a-bug-in-darcs1__darcs_2-conversion.dpatch
• patch-preview.txt
• unnamed

I haven't been screening this yet since this patch changes the 
semantics of V1 patches, however slightly. I believe what I did here 
is correct in principle, it may have unintended consequences though. A 
more conservative approach would be to change only the publicUnravel 
and leave effect and unravel alone.

both sortCoalesceFL and dropAllInverses should be no-ops from the
point of view of effect, right?

unravel only seems to be used in conflict marking, so I think it's safe
to change it. Not sure if we should or not though.

> both sortCoalesceFL and dropAllInverses should be no-ops from the
> point of view of effect, right?

Yes, that is, assuming effect is only used to apply patches or similar
operations that really only care about the effect of a patch. I hope
this is the case.

> unravel only seems to be used in conflict marking, so I think it's safe
> to change it. Not sure if we should or not though.

I have been looking at the code again and I agree with you that the
change is safe.

Point in favour: the V2 implementation does not use sortCoalesceFL in
equivalent places, in fact the only occurrence of it is in the obscure
remNons function (where I think it could be replaced by dropAllInverses,
though I have no proof of that, of course). Neither does V3 use
sortCoalesceFL anywhere.

Point against: it worked in the past and there seems to be no pressing
reason to make the change.

I think the sortCoalesceFL in unravel may indeed be detrimental to
reliably correct conflict markup. I'll try to create a test case to
demonstrate that, based on my improved understanding of how conflict
resolution does/should work.

I am screening this patch now to avoid having to drag it along with 
(semantically) unrelated changes.

Fine - just a reminder that you mentioned possibly writing a test case
that demonstrates why sortCoalesceFL was bad, in case you think it's
still useful to do so.

> Fine - just a reminder that you mentioned possibly writing a test case
> that demonstrates why sortCoalesceFL was bad, in case you think it's
> still useful to do so.

It is still on my list, though not with a high priority.