I'm not going to screen this for a bit, to allow a chance
for comments. It's a followup to the discussion in patch1709.

I think it makes options for further refactorings clearer,
e.g. we could delay the mangling until we actually want to
mark the conflicts. But particularly the V1 code is a bit
involved so I'd rather take it slowly.

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

patch c2e1a252c039522eb7d92b2a1c69f512e955db3b
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Sun Jun  9 12:43:43 BST 2019
  * add an explicit type for the output of resolveConflicts
  
  This makes it clear that the output contains first the
  'mangled' view followed by all the conflicting parts.
  Currently the mangled view is the conflict markers if
  the conflict is all hunks, and one of the conflicting
  parts if not.
  
  There is a small behaviour change for the conflict parts
  for V1 patches: previously the mangled value would be
  removed from the parts if present, and now it's not. This
  doesn't make any difference at the use sites, and is
  consistent with V2 patches.

Attachments

• add-an-explicit-type-for-the-output-of-resolveconflicts.dpatch
• patch-preview.txt
• unnamed

> I'm not going to screen this for a bit, to allow a chance
> for comments. It's a followup to the discussion in patch1709.

Please don't. It will certainly conflict with the API changes I needed
to make to integrate RepoPatchV3. These changes remove one layer of
lists in the API and instead add a generic function combineConflicts to
Darcs.Patch.Conflicts that the existing (V1 and V2) RepoPatch
implementations are able to use to reduce the two-layer list to one
layer. The list-of-lists really is an internal implementation detail
here and should not be exposed to any user code (i.e. in
Darcs.Repository.Resolution). I'll add some explanations when I send
this patch.

> I think it makes options for further refactorings clearer,
> e.g. we could delay the mangling until we actually want to
> mark the conflicts. But particularly the V1 code is a bit
> involved so I'd rather take it slowly.

I have done some refactoring there, too, in order to make the changes
mentioned above feasible. Please postpone until you have taken a look at
them, I think the code is a lot clearer now and also uses much less
witness casting.

> Please don't. It will certainly conflict with the API changes I 
> needed to make to integrate RepoPatchV3.

Sure. I would just comment that my change is purely a representation
change + a single tiny behaviour change to V1 patches, so would be
relatively easy to merge with and might make the intent of further
refactorings clearer. But I'm happy to just review other changes
directly based on my understanding of the current representation
anyway.

Please take a look at the patch named "change the type of 
resolveConflicts" in the latest bunch of patches I attached to 
patch1730. I think this already clears up the confusion regarding the 
nested list. If not, I am fully open to introduce a new type as an 
additional refactor.

I like the idea, but I think the types aren't right, yet.

As noted in my latest remarks to issue2550, the least thing we should do for conflicts 
that we don't know how to properly mark (i.e. everything that involves non-hunk prims) is 
to be honest about it and warn the user, displaying the conflicting unmangled changes; and 
then /refrain/ from doing half-assed things like arbitrarily choosing one of the 
alternatives.

This means that we want resolveConflicts to return a list of either successfully mangled 
markups or unmodified raw alternatives. Taking the de-facto nomenclature from V1, I would 
define

-- | A list of conflicting alternatives. They form a connected
-- component of the conflict graph i.e. one transitive conflict.
-- The SHA1s are optional meta-data hashes to identify provenance
-- of the alternative (a list since one alternative may consist
-- of many prims that don't directly conflict).
type Unravelled prim wX = [(Sealed (FL prim wX), [SHA1])]

-- | Result of mangling a single Unravelled.
type Mangled prim wX = Sealed (FL prim wX)

class PrimMangleUnravelled prim where
  -- | Mangle conflicting alternatives if possible
  mangleUnravelled :: Unravelled prim wX
                   -> Maybe (Mangled prim wX)

class Conflict p where
  resolveConflicts
    :: RL p wX wY -> [Either (Unravelled (PrimOf p) wY) (Mangled (PrimOf p) wY)]

-- | This is for all discovered conflicts, basically a partitionEithers
-- of the result of resolveConflicts, with all mangled conflicts merged
-- into a single FL. Since we avoid pseudo-mangling of alternatives we
-- can now really be sure that this merge always succeeds.
data ConflictDetails prim wX = ConflictDetails
    { conflictUnmangled :: [Unravelled prim wX]
    , conflictMangled :: Mangled prim wX
    }

standardResolution :: (...)
                   => RL p wX wY
                   -> ConflictDetails (PrimOf p) wY

warnUnmangled :: (...)
              => ConflictDetails prim wX -> IO ()

Another possibility is to use the ConflictDetails data type for a single
(transitive) conflict as in your patch i.e.

resolveConflicts :: RL p wX wY -> [ConflictDetails (PrimOf p) wX]

but with mangled/unmangled as alternatives, not record fields:

data ConflictDetails prim wX
  = ConflictMangled (Mangled prim wX)
  | ConflictUnmangled (Unravelled prim wX) -- or ConflictParts if you
prefer that

Or we could turn Mangled and Unravelled into newtypes:

newtype Mangled prim wX =
  Mangled (Sealed (FL prim wX))
newtype Unravelled prim wX =
  Unravelled ([(Sealed (FL prim wX), [SHA1])])

and use higher kinded type synonyms

type EitherHO2 f g p wX = Either (f p wX) (f p wX)
type ConflictDetails = EitherHO2 Mangled Unmangled

Any further ideas? Comments?

________________________________

Helmholtz-Zentrum Berlin für Materialien und Energie GmbH

Mitglied der Hermann von Helmholtz-Gemeinschaft Deutscher Forschungszentren e.V.

Aufsichtsrat: Vorsitzender Dr. Volkmar Dietz, stv. Vorsitzende Dr. Jutta Koch-Unterseher
Geschäftsführung: Prof. Dr. Bernd Rech (Sprecher), Prof. Dr. Jan Lüning, Thomas Frederking

Sitz Berlin, AG Charlottenburg, 89 HRB 5583

Postadresse:
Hahn-Meitner-Platz 1
D-14109 Berlin

Another possibility is to use the ConflictDetails data type for a single
(transitive) conflict as in your patch i.e.

resolveConflicts :: RL p wX wY -> [ConflictDetails (PrimOf p) wX]

but with mangled/unmangled as alternatives, not record fields:

data ConflictDetails prim wX
  = ConflictMangled (Mangled prim wX)
  | ConflictUnmangled (Unravelled prim wX) -- or ConflictParts if you
prefer that

Or we could turn Mangled and Unravelled into newtypes:

newtype Mangled prim wX =
  Mangled (Sealed (FL prim wX))
newtype Unravelled prim wX =
  Unravelled ([(Sealed (FL prim wX), [SHA1])])

and use higher kinded type synonyms

type EitherHO2 f g p wX = Either (f p wX) (f p wX)
type ConflictDetails = EitherHO2 Mangled Unmangled

Any further ideas? Comments?

Initial comments for now, as I still haven't got to reviewing your
first round of refactoring.

I'm happy with the general idea of identifying when we couldn't 
properly mangle the patches. Looking at the representation
I proposed, could it just be expressed by making the 
'conflictMangled' field a Maybe? Or is it more complicated than 
that?

My personal preference is for proceeding in small refactorings
or changes that I can quickly understand in isolation, so if
I was doing the refactoring I would probably first go to my type
or something like it, then change conflictMangled to a Maybe.

I didn't understand what the SHA1s in your type are about. If we
have them (i.e. a V3 repo) aren't they in the prims anyway?

> I'm happy with the general idea of identifying when we couldn't 
> properly mangle the patches. 

BTW, there is more to it than being nice(r) to users. I added a QC test
for the property that says "mangled conflicts never conflict with each
other", and found that this regularly fails. Investigating revealed that
the reason are replace patches that involve the single-letter word "v"
or "^" or any other word used in the mangling of hunks.

These failures go away if we refuse to add pseudo-mangling involving
non-hunks (and instead display them to the user and/or write them to a
file).

> Looking at the representation
> I proposed, could it just be expressed by making the 
> 'conflictMangled' field a Maybe? Or is it more complicated than 
> that?

Possible, but I think either mangled or unmangled better captures how we
want to use the data. At least I can't see a reason to keep the
unmangled version around if mangling was successful. But it's okay for a
first step if we want to split this into a number of smaller refactors.

> My personal preference is for proceeding in small refactorings
> or changes that I can quickly understand in isolation, so if
> I was doing the refactoring I would probably first go to my type
> or something like it, then change conflictMangled to a Maybe.

Yeah, I have been doing it similarly, of course, but the intermediate
steps have been lost because I have done this in a completely messed up
repo where I mixed it with trying to fix conflict resolution for v3...

I'll take your suggestion into account and split the refactor into
smaller steps that are hopefully easy to understand.

> I didn't understand what the SHA1s in your type are about. If we
> have them (i.e. a V3 repo) aren't they in the prims anyway?

Unfortunately not! For RepoPatchV3 we have

  PrimOf (RepoPatchV3 prim) ~ prim

and not

  PrimOf (RepoPatchV3 prim) ~ NamedPrim prim

The reason for this is that many operations on plain prims (e.g.
coalescing and shrinking of sequences, constructing prims from hunks
etc) cannot be implemented for named prims without resorting to
dangerous unsafe operations: you need to conjure a universally unique
PrimPatchId out of thin air; and when you do that you must be /very/
careful that the resulting named prims never leak into any patches that
may later be stored on disk. Indeed most of the code in Darcs.Repository
that directly deals with prims really operates on anonymous prims that
have never been part of a Named patch: diffTrees freshly creates unnamed
prims, the pending patch consists of unnamed prims.

So the NamedPrim wrapper is currently treated as an implementation
detail of RepoPatchV3, and named prims don't appear anywhere in the
RepoPatch API.

This was a design decision I made in order to keep the full integration
of v3 manageable. Things have settled a bit since then and we could
re-consider that decision. For instance, the Rebase implementation
internally deals with prims (for Fixups) and these should rightfully be
named prims in a v3 repo, rather than plain prims as they are now.
However, tackling this raises some horny design questions: the
(potentially) named prims in a Named patch don't support the same API
(i.e. class instances) that unnamed prims do; they really are different
types, so we would need a new type family (PotentiallyNamedPrimOf?) and
a new version of class PrimPatch (PotentiallyNamedPrimPatch?). The ugly
names should serve to illustrate my hesitation in adding them...

Here is an updated bundle containing a rebases version of you patch,
together with further refactorings in small(er) steps as discussed.

Unfortunately they depend on a lot of unrelated changes. So this bundle is
sort of preliminary. These are the relevant patches:

  * move instance PrimMangleUnravelled for Prim.V1 to its own module
  * cleanup and refactor mangleUnravelled
  * add an explicit type for the output of resolveConflicts
  * change the return type of mangleUnravelled
  * introduce type synonyms Mangled and Unravelled
  * apply only properly mangled conflict resolutions, warn about any others

If could just look over these and tell me if this is more to your liking...?

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

patch 8a3bc69e7a55b30aefd58d6b5d2f348e286d35b0
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Jul  5 23:30:01 CEST 2019
  * demote errors in defaults file and commandline to warnings
  
  When we change the UI by e.g. adding a new command line option, we want the
  user to be able to use these switches in their defaults file without this
  giving an error when they use an older version of darcs. So this change
  improves forwards compatibility.

patch 89f2ddd59c03d54ae656930206e02b80031c6731
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Jun 19 22:06:47 CEST 2019
  * turn fancyPrinters into an IO action
  
  This is what it actually is, via getPolicy. The unsafePerformIO was
  perfunctory, since fancyPrinters (almost) always gets passed to a real IO
  action that does the actual printing. The only exceptions are when we use it
  for tracing and in the harness when converting our TestResult to HUnit or
  QuickCheck. Consequently showDoc has been renamed unsafeRenderStringColored
  (to discourage its use) and a function debugDoc added.

patch 3fd4dfddbb35169970ad18fb09c873aa45357583
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Jul 11 18:25:35 CEST 2019
  * colorize warnings

patch 544e1f134a90196da293d5c904bdde8f16d91157
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Jul 11 19:07:19 CEST 2019
  * tests: expect conflicts warning on stderr

patch de792b6ffc34f785e7f02463d6ab503fdd465207
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Jun 28 20:25:32 CEST 2019
  * automatically specialize merging and equality for sequences of patches with identity
  
  The idea is to allow patch sequences to adapt their behavior depending on
  whether the patch type has an Ident instance or not. This is done for two
  reasons:
  (1) equality tests for, and removing elements from sequences of patches is
      much faster if we can use PatchIds;
  (2) merging sequences of patches with identity must first remove duplicate
      patches, otherwise these would be wrongly treated as conflicting.
  The new class Sequence abstracts over the basic operations we want to
  specialize for FL and RL: equality and removing elements. Similarly, class
  Merge gets a new method mergeFLs.
  By using the new DerivingVia extension we can derive the Sequence instances
  for patch types with an Ident instance from one template definition for a
  suitable newtype wrapper named HasIdent.
  The fastRemoveXXX functions are now gone from the Patch API and only used
  internally inside D.P.Ident, the ugly IdEq2 class could be fully removed.

patch 884eb09011c67a65d042e337987b3415f3f8eb0e
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Jul 12 19:25:29 CEST 2019
  * move instance PrimMangleUnravelled for Prim.V1 to its own module
  
  This is a pure code move without any changes to the definitions. It serves
  as preparation for a larger refactor but makes sense in its own right.

patch a220e5e668d13464ef1a23c4e43c0e3b3bbaaef1
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Jul 12 20:26:56 CEST 2019
  * cleanup and refactor mangleUnravelled

patch 1362fe7224611f2b5593ce90406551a9b21cf3c6
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Jun 30 21:26:07 CEST 2019
  * clean up patch equality
  
  We make a clear distinction between semantic and structural equality. The
  former is captured by Eq2, the latter by the new class StructuralEq2. We use
  nominal equality as optimization for semantic equality where available.
  Structural equality is understood to be local; it should not refer to
  structural equality of its components but rather to their semantic equality.
  Thus the new definitions for StructuralEq2 are quite similar to those which
  previously defined Eq2. Some of the equality definitions have been cleaned
  up and streamlined, for instance the two basic prim patch types now use
  deriving Eq instead of manually written definitions.
  There remain no /uses/ of unsafeCompare throughout the code except in
  RepoPatchV1 and in the default method definitions for Eq2. It still appears
  in many places to define Eq2, though, since this is quite convenient.

patch e0a99e974f34a54fe9b0bcccb225c9499c8b7bb5
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Jul 10 19:43:41 CEST 2019
  * add two properties about conflict resolution and test them
  
  This required adding a utility function permutationsRL and factoring out
  mergeList from D.R.Resolution into D.P.CommuteNoConflicts.

patch 56596643644e3f0e81c6d51371b3add360960489
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Jul 12 21:43:03 CEST 2019
  * add an explicit type for the output of resolveConflicts
  
  This makes it clear that the output contains first the
  'mangled' view followed by all the conflicting parts.
  Currently the mangled view is the conflict markers if
  the conflict is all hunks, and one of the conflicting
  parts if not.

patch 4d5f45cd18be910226b17636663e17dc34601bd7
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Jul 10 23:11:29 CEST 2019
  * v3: remove debug stuff and other minor cleanups

patch 93b5c1f0b9ca14d9e1193d60ca14cd9829f1c168
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Jul 10 19:19:14 CEST 2019
  * move v3 helper functions idsFL and idsRL to harness

patch 26a5c2acbd18e96890b17250e51a3a6cba6c77a7
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Jul 10 23:17:25 CEST 2019
  * v3: fix and restructure conflict resolution
  
  It probably doesn't make much sense to view the diffs here, since I really
  restructured things a lot, re-wrote the documentation comments, and renamed
  and reordered functions.
  
  The main semantic changes are:
  - fixed the 'components' function which was buggy
  - fixed and cleaned up the history traversal to find vertices
  - throw out the no longer needed 'dropDominated'
  - calculate conflicts between vertices freshly
  
  The last point was crucial. In my original version I made the mistake to
  think that we can read off conflicts between vertices, i.e. contexted prims,
  directly from the conflictor. This is only so in the simplest case of two
  conflicting prims. In general it is false: the contexted patch represented
  by a conflictor need not directly conflict with all of the conflicts stored
  in the conflictor. This cannot be so, since e.g. a prim can conflict with a
  conflictor simply because it conflicts with one of the conflicts stored in
  the conflictor. The side-conditions in the commuteNoConflict cases make that
  pretty clear.

patch 052205481d6a0c8bb8a6a5951fe7c76ea0a84c4a
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Jul 12 21:43:11 CEST 2019
  * change the return type of mangleUnravelled
  
  This makes it clear that mangleUnravelled is unable to deal with conflicts
  involving anything other than hunks. Previously we returned the unmodified
  head of the input list if that was the case, now we return Nothing.

patch e0054d261d978f7ecd02f3e6abf0a2ea64bef2f7
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Jul 12 22:16:05 CEST 2019
  * introduce type synonyms Mangled and Unravelled

patch fb1318dd47c73bf042fbb09cf10908c878097c53
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Jul 12 22:17:04 CEST 2019
  * apply only properly mangled conflict resolutions, warn about any others
  
  This means we no longer arbitrarily choose the first of the conflicting
  alternatives when conflict mangling fails.

patch bec71165983ceb6c103a90cf0d9cb619d33d9485
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Jul 13 10:02:59 CEST 2019
  * tests: adapt conflict-doppleganger.sh
  
  A conflict involving 'addfile' prims cannot be cleanly mangled. This means
  that there is nothing to record after the pull and the test therefore fails.
  But this isn't what we want to test, so we record the 'addfile' separately
  and share it.

Attachments

• demote-errors-in-defaults-file-and-commandline-to-warnings.dpatch
• patch-preview.txt
• unnamed

Please disregard this bundle for the moment. I think I severely messed
up the changes during a rebase. Will clean up and re-send.

On 12/07/2019 12:56, Ben Franksen wrote:

>> Looking at the representation
>> I proposed, could it just be expressed by making the 
>> 'conflictMangled' field a Maybe? Or is it more complicated than 
>> that?
> 
> Possible, but I think either mangled or unmangled better captures how we
> want to use the data. At least I can't see a reason to keep the
> unmangled version around if mangling was successful. But it's okay for a
> first step if we want to split this into a number of smaller refactors.

On this specific point, now that I've realised you've already changed
resolveConflicts to only return the mangled version (or rather the
possibly badly mangled version), I think your proposal to move from that
to "mangled or unmangled" makes sense.

Or maybe we should always store the unmangled ones and do the mangling
at the last possible moment.

>>> Looking at the representation
>>> I proposed, could it just be expressed by making the 
>>> 'conflictMangled' field a Maybe? Or is it more complicated than 
>>> that?
>>
>> Possible, but I think either mangled or unmangled better captures how we
>> want to use the data. At least I can't see a reason to keep the
>> unmangled version around if mangling was successful. But it's okay for a
>> first step if we want to split this into a number of smaller refactors.
> 
> On this specific point, now that I've realised you've already changed
> resolveConflicts to only return the mangled version (or rather the
> possibly badly mangled version), I think your proposal to move from that
> to "mangled or unmangled" makes sense.

...and in the meantime I realised that keeping all the "unravelled"
conflicts around /does/ have some advantages over an either/or data
type... =:O

For instance, it makes it easier to calculate the list of all conflicted
files: just nubSort and concatMap all the conflictParts. Therefore, in
the rebased and fixed bundle I am preparing to send, I have adopted your
approach of making the 'conflictMangled' field a Maybe. This also works
quite well with a related refactor that changes the output type of
mangleUnravelled to 'Maybe ...'; which in turn makes it clear that it
can indeed fail to produce a meaningfull mangling of the input, and
paves the way for the final patch that actually changes the behavior by
refusing to apply pseudo-mangled resolutions and instead reports what we
couldn't mangle.

> Or maybe we should always store the unmangled ones and do the
> mangling at the last possible moment.
I like the idea. However, we should keep in mind that for V3 we will
want better markup that annotates hunks with their provenance (e.g.
hashes). Originally I thought that this would most naturally be done
with an instance PrimMangleUnravelled for NamedPrim, and that we can
only use such an instance for RepoPatchV3 if we keep it as part of
resolveConflicts. But now that I have refactored mangleUnravelled into
something clear and understandable, I think I could also add this in
different ways. Still, let's keep it as part of resolveConflicts for the
moment, until the dust has settled over this particular refactor.

Okay, here is a fixed bundle. The dependency on adding the two properties is
a feature, this time ;)

I elided the second paragraph of your patch comment, since the change you
talked about was already done in my previous refactors. I also modified it
by factoring out the utility function mangleOrChooseFirst.

For now this is all pure refactors (apart from the added tests). I plan on
sending another bundle that changes the behavior (no longer select one of
the unravelled alternatives but report conflicts we could not mangle). This
will include adapting some of the test scripts and will finally make all
tests succeed (discounting failed QC tests for RepoPatchV2 due to crashes
and other misbehaviors).

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

patch 73775adfdeb239fd23fe6a8df6bc497601976c77
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Jul 10 19:43:41 CEST 2019
  * add two properties about conflict resolution and test them
  
  This required adding a utility function permutationsRL and factoring out
  mergeList from D.R.Resolution into D.P.CommuteNoConflicts.

patch 73480ca7345e7bdd0caf08c47e4c41b9fb3da07a
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jul 16 15:40:58 CEST 2019
  * add an explicit type for the output of resolveConflicts
  
  This makes it clear that the output contains first the
  'mangled' view followed by all the conflicting parts.
  Currently the mangled view is the conflict markers if
  the conflict is all hunks, and one of the conflicting
  parts if not.

patch 44c77f65bfd0af1d05888ae7f80fe796d6540ab0
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jul 16 15:52:22 CEST 2019
  * introduce type synonyms Mangled and Unravelled

patch 393678ac3d7d347e40f9d4dc50cda96393f2cc04
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jul 16 15:52:31 CEST 2019
  * add haddocks for mangleUnravelled method

patch 3900e63f717181f325ffd78c1a51bcf77f3c879f
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Jul 12 19:25:29 CEST 2019
  * move instance PrimMangleUnravelled for Prim.V1 to its own module
  
  This is a pure code move without any changes to the definitions. It serves
  as preparation for a larger refactor but makes sense in its own right.

patch 440b6e59a5dcfa644bf8405068cdc5872ceaedff
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Jul 12 20:26:56 CEST 2019
  * cleanup and refactor mangleUnravelled

patch 7273a6626d65b0faf0fac837fe93624ede366ade
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jul 16 16:06:00 CEST 2019
  * change the return type of mangleUnravelled
  
  This makes it clear that mangleUnravelled is unable to deal with conflicts
  involving anything other than hunks. Previously we returned the unmodified
  head of the input list if that was the case, now we return Nothing.

Attachments

• add-two-properties-about-conflict-resolution-and-test-them.dpatch
• patch-preview.txt
• unnamed

I think the remaining active bundle is ready for screened now.

Fine with me.

Sorry, looks as if I amended or rebased in the meantime. Sending again and
will screen immediately.

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

patch 3900e63f717181f325ffd78c1a51bcf77f3c879f
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Jul 12 19:25:29 CEST 2019
  * move instance PrimMangleUnravelled for Prim.V1 to its own module
  
  This is a pure code move without any changes to the definitions. It serves
  as preparation for a larger refactor but makes sense in its own right.

patch 440b6e59a5dcfa644bf8405068cdc5872ceaedff
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Jul 12 20:26:56 CEST 2019
  * cleanup and refactor mangleUnravelled

patch 0a4d1af29a1982ca59540b66a72d637a14e5ec51
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Jul 12 21:43:03 CEST 2019
  * add an explicit type for the output of resolveConflicts
  
  This makes it clear that the output contains first the
  'mangled' view followed by all the conflicting parts.
  Currently the mangled view is the conflict markers if
  the conflict is all hunks, and one of the conflicting
  parts if not.

patch d1d723faa6f3dc0bfc6c827e18a5ca49b386892f
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Jul 12 21:43:11 CEST 2019
  * change the return type of mangleUnravelled
  
  This makes it clear that mangleUnravelled is unable to deal with conflicts
  involving anything other than hunks. Previously we returned the unmodified
  head of the input list if that was the case, now we return Nothing.

patch 1cbd89a776d0e75c05a7e4fbc0156de14c165cd5
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Jul 12 22:16:05 CEST 2019
  * introduce type synonyms Mangled and Unravelled

Attachments

• move-instance-primmangleunravelled-for-prim_v1-to-its-own-module.dpatch
• patch-preview.txt
• unnamed

> patch 3900e63f717181f325ffd78c1a51bcf77f3c879f
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Fri Jul 12 19:25:29 CEST 2019
>   * move instance PrimMangleUnravelled for Prim.V1 to its own 
module
>   
>   This is a pure code move without any changes to the definitions. 
It serves
>   as preparation for a larger refactor but makes sense in its own 
right.

OK

> patch 440b6e59a5dcfa644bf8405068cdc5872ceaedff
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Fri Jul 12 20:26:56 CEST 2019
>   * cleanup and refactor mangleUnravelled

Looks good, the new version is easier to read and properly 
documented (plus a bit safer with the FileState type).

> patch 0a4d1af29a1982ca59540b66a72d637a14e5ec51
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Fri Jul 12 21:43:03 CEST 2019
>   * add an explicit type for the output of resolveConflicts

This doesn't actually build without the next patch, but never mind 
:-) (it changes the type of resolveConflicts without changing any 
instances)

> patch d1d723faa6f3dc0bfc6c827e18a5ca49b386892f
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Fri Jul 12 21:43:11 CEST 2019
>   * change the return type of mangleUnravelled
>   
>   This makes it clear that mangleUnravelled is unable to deal with 
conflicts
>   involving anything other than hunks. Previously we returned the 
unmodified
>   head of the input list if that was the case, now we return 
Nothing.
Good idea to separate this decision from the hunk mangling logic. I 
guess that logic could actually eventually be made independent of 
Prim.V1 as the core of it really only cares about FileHunk.
> patch 1cbd89a776d0e75c05a7e4fbc0156de14c165cd5
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Fri Jul 12 22:16:05 CEST 2019
>   * introduce type synonyms Mangled and Unravelled

OK

>>   * add an explicit type for the output of resolveConflicts
> 
> This doesn't actually build without the next patch, but never mind 
> :-) (it changes the type of resolveConflicts without changing any 
> instances)

Hm, I was pretty sure I had fixed these problems in the last bundle I
attached.

>>   * change the return type of mangleUnravelled
>>   
>>   This makes it clear that mangleUnravelled is unable to deal with 
> conflicts
>>   involving anything other than hunks. Previously we returned the 
> unmodified
>>   head of the input list if that was the case, now we return 
> Nothing.
> Good idea to separate this decision from the hunk mangling logic. I 
> guess that logic could actually eventually be made independent of 
> Prim.V1 as the core of it really only cares about FileHunk.

True, though this doesn't help us much, since FileUUID (in its present
form) cannot easily implement IsHunk (it's hunks aren' line-based but
offset-based). Indeed, mangleUnravelled was originally "independent" of
Prim.V1 (part of Darcs.Patch.Conflict), via classes like IsHunk,
PrimConstruct, PrimClassify etc.