darcs

Patch 1815 fix a bug in darcs1->darcs-2 conversion

Title fix a bug in darcs1->darcs-2 conversion
Superseder Nosy List bf
Related Issues
Status in-discussion Assigned To
Milestone

Created on 2019-06-12.11:23:54 by bf, last changed 2019-06-15.15:10:23 by bf.

Files
File name Status Uploaded Type Edit Remove
fix-a-bug-in-darcs1__darcs_2-conversion.dpatch bf, 2019-06-12.11:23:54 application/x-darcs-patch
patch-preview.txt bf, 2019-06-12.11:23:54 text/x-darcs-patch
unnamed bf, 2019-06-12.11:23:54 text/plain
See mailing list archives for discussion on individual patches.
Messages
msg20748 (view) Author: bf Date: 2019-06-12.11:23:54
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
msg20756 (view) Author: bf Date: 2019-06-12.14:57:52
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.
msg20789 (view) Author: ganesh Date: 2019-06-14.11:56:49
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.
msg20799 (view) Author: bf Date: 2019-06-14.15:35:31
> 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.
msg20838 (view) Author: bf Date: 2019-06-15.15:10:23
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.
History
Date User Action Args
2019-06-12 11:23:54bfcreate
2019-06-12 14:57:52bfsetmessages: + msg20756
2019-06-14 11:56:49ganeshsetmessages: + msg20789
2019-06-14 15:35:31bfsetmessages: + msg20799
2019-06-15 14:14:11bfsetstatus: needs-screening -> in-discussion
2019-06-15 15:10:23bfsetmessages: + msg20838