This is broken out of my unwind changes.

Until those changes are submitted the newly exported functions
aren't actually used, but I think they would be worthwhile
helpers anyway.

1 patch for repository darcs-unstable@darcs.net:screened:

patch 80715460da0bdfa1ac95cd2361a920bb50fe6bb6
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Tue Jan 28 06:36:39 GMT 2020
  * introduce some generic merge function helpers

Attachments

• introduce-some-generic-merge-function-helpers.dpatch
• patch-preview.txt
• unnamed

> patch 80715460da0bdfa1ac95cd2361a920bb50fe6bb6
> Author: Ganesh Sittampalam <ganesh@earth.li>
> Date:   Tue Jan 28 06:36:39 GMT 2020
>   * introduce some generic merge function helpers

+    -- TODO this should use mergerFLFL but that currently causes
+    -- a break in convert-darcs2, probably because it's sensitive
+    -- to the exact order of the merged patches

Indeed. More precisely, what differs now is the order of the conflicting
Nons in the conflictor. The Eq2 instance for RepoPatchV2 uses eqSet
(which compares lists as sets, IIUC), but the ShowPatch instance does
not sort them. RepoPatchV3 does that, BTW, but it is easier there
because we have a natural Ord instance via PatchIds.

So, should we sort the conflicting Nons in a V2 conflictor when we write
them to disk (or for simplicity whenever we show them)? What ordering
should we use? Prim.V1 comes with a suitable ordering but FileUUID does
not. However, I guess testing FileUUID with RepoPatchV2 is sort of
wasted anyway, so we should simply drop the unit tests for that combination.

Even of we do that, we need to decide how to treat the context patches
in a Non. I am inclined to be practical here and just compare the prims
at the end. This is not perfect as theoretically we could have equal
prims with different contexts (i.e. duplicate prims), except that I
believe in this case RepoPatchV2 would have created a Duplicate, so this
should be safe.

Attachments

• pEpkey.asc

Here is a follow-up to my comments. I think this is okay but I'd like you to
take a closer look before I screen this.

2 patches for repository http://darcs.net/screened:

patch 0e80c5c65e9706de477cf6cdd2beefac0a0e7655
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Feb 22 12:59:41 CET 2020
  * sort Nons when showing a RepoPatchV2

patch ae013c0993e12637942a34ea24b5e9e654026e15
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Feb 22 13:04:53 CET 2020
  * simplify instance Eq Non

Attachments

• patch-preview.txt
• sort-nons-when-showing-a-repopatchv2.dpatch
• unnamed

Here is a follow-up to my comments. I think this is okay but I'd like you to
take a closer look before I screen this.

2 patches for repository http://darcs.net/screened:

patch 0e80c5c65e9706de477cf6cdd2beefac0a0e7655
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Feb 22 12:59:41 CET 2020
  * sort Nons when showing a RepoPatchV2

patch ae013c0993e12637942a34ea24b5e9e654026e15
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Feb 22 13:04:53 CET 2020
  * simplify instance Eq Non

Attachments

• patch-preview.txt
• sort-nons-when-showing-a-repopatchv2.dpatch
• unnamed

On 22/02/2020 11:09, Ben Franksen wrote:
> 
> Ben Franksen <ben.franksen@online.de> added the comment:
> 
>> patch 80715460da0bdfa1ac95cd2361a920bb50fe6bb6
>> Author: Ganesh Sittampalam <ganesh@earth.li>
>> Date:   Tue Jan 28 06:36:39 GMT 2020
>>   * introduce some generic merge function helpers
> 
> +    -- TODO this should use mergerFLFL but that currently causes
> +    -- a break in convert-darcs2, probably because it's sensitive
> +    -- to the exact order of the merged patches
> 
> Indeed. More precisely, what differs now is the order of the conflicting
> Nons in the conflictor. The Eq2 instance for RepoPatchV2 uses eqSet
> (which compares lists as sets, IIUC), but the ShowPatch instance does
> not sort them. RepoPatchV3 does that, BTW, but it is easier there
> because we have a natural Ord instance via PatchIds.
> 
> So, should we sort the conflicting Nons in a V2 conflictor when we write
> them to disk (or for simplicity whenever we show them)?

Is this actually necessary? What I really had in mind with my TODO
comment was just to take a closer look at the new output, check it's
still correct and then update the test. But maybe having a canonical
output format is worth it.

> What ordering
> should we use? Prim.V1 comes with a suitable ordering but FileUUID does
> not. However, I guess testing FileUUID with RepoPatchV2 is sort of
> wasted anyway, so we should simply drop the unit tests for that combination.

Agreed.

> Even of we do that, we need to decide how to treat the context patches
> in a Non. I am inclined to be practical here and just compare the prims
> at the end. This is not perfect as theoretically we could have equal
> prims with different contexts (i.e. duplicate prims), except that I
> believe in this case RepoPatchV2 would have created a Duplicate, so this
> should be safe.

Two patches could be completely different while in the same context, but
identical textually in different contexts. e.g.

1: addfile foo/X
2: addfile bar/X

in context:
1: [] ; addfile foo/X
2: [mv foo old ; mv bar foo] ; addfile foo/X

Maybe that's not possible for Non as the context has to not commute with
the prim, but I'm not too convinced by the general statement.

Why not just sort including the contexts? If this is just about having a
canonical ordering we don't really care what it is, but having any
possibility of equality would mean there'll be edge cases.

> patch 0e80c5c65e9706de477cf6cdd2beefac0a0e7655
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Sat Feb 22 12:59:41 CET 2020
>   * sort Nons when showing a RepoPatchV2

You remove the tests for RepoPatchV2 FileUUID in this patch without
explanation, might be better in a separate patch.

PrimOrd would benefit from a bit more description of when it's
appropriate to use it.

>> So, should we sort the conflicting Nons in a V2 conflictor when we write
>> them to disk (or for simplicity whenever we show them)?
> 
> Is this actually necessary?

I don't think so. But it would avoid running into the same problem again.

> What I really had in mind with my TODO
> comment was just to take a closer look at the new output, check it's
> still correct and then update the test.

This wold be fine, too, modulo above comment. BTW, making the format
canonical, we will also mean a one-time re-generation of the test
bundles, see my patch.

>> What ordering
>> should we use? Prim.V1 comes with a suitable ordering but FileUUID does
>> not. However, I guess testing FileUUID with RepoPatchV2 is sort of
>> wasted anyway, so we should simply drop the unit tests for that combination.
> 
> Agreed.
> 
>> Even of we do that, we need to decide how to treat the context patches
>> in a Non. I am inclined to be practical here and just compare the prims
>> at the end. This is not perfect as theoretically we could have equal
>> prims with different contexts (i.e. duplicate prims), except that I
>> believe in this case RepoPatchV2 would have created a Duplicate, so this
>> should be safe.
> 
> Two patches could be completely different while in the same context, but
> identical textually in different contexts. e.g.
> 
> 1: addfile foo/X
> 2: addfile bar/X
> 
> in context:
> 1: [] ; addfile foo/X
> 2: [mv foo old ; mv bar foo] ; addfile foo/X
> 
> Maybe that's not possible for Non as the context has to not commute with
> the prim, but I'm not too convinced by the general statement.

Right. I did not take into account that the prims at the end are not
comparable due to different contexts.

> Why not just sort including the contexts?

Because that is more difficult ;-) It means we have to write a
comparison function for RepoPatchV2. Doable, but I am not too keen.
Another option could be to take the effect of the context and use that
for sorting. This is much easier, but I am not 100% sure it will avoid
all edge cases.

> If this is just about having a
> canonical ordering we don't really care what it is, but having any
> possibility of equality would mean there'll be edge cases.

True.

>> patch 0e80c5c65e9706de477cf6cdd2beefac0a0e7655
>> Author: Ben Franksen <ben.franksen@online.de>
>> Date:   Sat Feb 22 12:59:41 CET 2020
>>   * sort Nons when showing a RepoPatchV2
> 
> You remove the tests for RepoPatchV2 FileUUID in this patch without
> explanation, might be better in a separate patch.

Agreed.

> PrimOrd would benefit from a bit more description of when it's
> appropriate to use it.

I think we should reject the patch and just regenerate the test bundles.
The fact that the V2 conflictor format is not canonical is just one more
infelicity of a format with lots of other problems, most of them more
severe. Not worth the effort.

Attachments

• pEpkey.asc

I've marked your patches as "dead". If one of us gets round to it soon
we can fix the TODO and update the test case before accepting
this patch, or just leave it for a future cleanup.

To make progress with review, I have added two follow-up patches, which I'll
self-accept.

3 patches for repository http://darcs.net/reviewed:

patch 80715460da0bdfa1ac95cd2361a920bb50fe6bb6
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Tue Jan 28 07:36:39 CET 2020
  * introduce some generic merge function helpers

patch 9b0829c6113fb002675d42120a47b106f0b58584
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu May 21 10:19:25 CEST 2020
  * use mergerFLFL in instance Merge (FL p)

patch 32da7bf3481dfe70c962e3553be591c62fb424e1
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu May 21 10:20:54 CEST 2020
  * fix convert darcs2 test data after patch 9b0829c6113f
  
  This accounts for the fact that the on-disk format of V2 conflictors does
  not order the conflicting patches. This order is different now because the
  new definition merge for FLs merges in a different order.

Attachments

• introduce-some-generic-merge-function-helpers_0.dpatch
• patch-preview.txt
• unnamed