This bundle consists purely of refactors with no change in behavior. The
goal is to prepare screened for integration of RepoPatchV3 which needs to
add identities to the prim patches it contains.

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

patch c9c9bd06432fd34b305a2bcba6d05d68c0e4b3ff
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jan 22 14:21:11 CET 2019
  * inline function Darcs.Patch.Named.namepatch
  
  This is in preparation of adding identifiers to prims when constructing
  named patches.

patch b0785ecb52aef2e8740b7fc92c747dde0e52116c
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jan 22 18:21:58 CET 2019
  * refactor: eliminate class FromPrims
  
  Again, preparation for adding identities to prim patches. The method
  fromPrims was used mainly to construct Named patches via infopatch or
  anonymous from Darcs.Patch.Named. These functions now take an FL of prim
  patches as input.

patch 36033bed448aa323bc06b92dc367b193a9b86f13
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jan 22 18:31:08 CET 2019
  * eliminate an unneeded use of fromPrim

patch c7832c1bef06d3c495eacc201a2a3b2984e32d14
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jan 22 18:57:20 CET 2019
  * eliminate use of fromPrim in convertDarcs2
  
  The code now uses the data constructor V2.Normal instead.

patch b08f89556cd0304dc3bf024dcee9e4d4870ca363
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jan 22 19:30:12 CET 2019
  * remove instances for FromPrim and PrimPatchBase for prim types

patch 08423b471438c372f0580dcb1046f09f2dbff090
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jan 22 19:40:24 CET 2019
  * move instance FromPrim (FL p) to Darcs.Test.Patch.Examples.Set1
  
  This is the only place where the instance is used.

patch fbcdcb8f5cb8c8dcff596d1c7a26c3a56bf0ec12
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jan 22 20:24:39 CET 2019
  * move classes PrimPatchBase, FromPrim, and ToFromPrim to their own module
  
  These classes are not part of the Prim patch API but the RepoPatch API.

patch 15b4e8f05c2fbf44854339aa26f99181881c3e2b
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jan 22 20:33:14 CET 2019
  * remove fromPrim from Darcs.Patch
  
  This is now no longer an official part of the Patch API.

patch 8a2472cf734f71e1ca48660d0865399eff2d5718
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jan 22 20:42:21 CET 2019
  * avoid direct imports of Darcs.Patch.FromPrim from outside of Darcs.Patch
  
  The only exception is now the implementation of rebase inject which needs
  low-level access to fromPrim.

patch d7a58ef262d38d0a3a4adbdf234e2421ddc1adde
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Jan 23 15:58:55 CET 2019
  * remove class PrimPatchCommon

Attachments

• inline-function-darcs_patch_named_namepatch.dpatch
• patch-preview.txt
• unnamed

Two more patches that help prepare V3 integration but make sense independently.

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

patch b9370f3f3bad1bc10c10a8558c192ce3e8465d2f
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Jan 24 14:53:42 CET 2019
  * add class IdEq2 to Darcs.Patch.Ident
  
  This allows a faster equality test for FLs of patches with identity.

patch ea200408863e1030fea867ac33f845f080f40eff
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Jan 23 19:42:50 CET 2019
  * annotate all uses of anonymous with comments
  
  Whenever we call anonymous to construct a Named patch we must now check
  that we don't accidentally store patches that result from merging them
  with normal patches.

Attachments

• add-class-ideq2-to-darcs_patch_ident.dpatch
• patch-preview.txt
• unnamed

>   * inline function Darcs.Patch.Named.namepatch
>   
>   This is in preparation of adding identifiers to prims when constructing
>   named patches.

> hunk ./src/Darcs/Patch/Named.hs 132
> -namepatch :: String -> String -> String -> [String] -> FL p wX wY ->
> IO (Named p wX wY)
> -namepatch date name author desc p
> -    | '\n' `elem` name = error "Patch names cannot contain newlines."
> -    | otherwise = do pinf <- patchinfo date name author desc
> -                     return $ NamedP pinf [] p
> -

We're losing the newline safety check, but fair enough if it's needed
for other refactoring. I've never seen it actually hit. I assume other
code protects us from the user entering that in the UI.

>   * refactor: eliminate class FromPrims

This is a nice simplification.

>   * eliminate an unneeded use of fromPrim

OK

>   * eliminate use of fromPrim in convertDarcs2

OK

>   * remove instances for FromPrim and PrimPatchBase for prim types

Another nice simplification. I vaguely recall being unhappy about
having to make these instances.

>   * move instance FromPrim (FL p) to Darcs.Test.Patch.Examples.Set1
>   
>   This is the only place where the instance is used.

OK (in a future patch we should aim to remove this completely rather
than having an orphan).

>   * move classes PrimPatchBase, FromPrim, and ToFromPrim to their own module
>   
>   These classes are not part of the Prim patch API but the RepoPatch API.

OK

>   * remove fromPrim from Darcs.Patch
>   
>   This is now no longer an official part of the Patch API.

We have an official Patch API??? :-)

>   * avoid direct imports of Darcs.Patch.FromPrim from outside of Darcs.Patch

OK

>   * remove class PrimPatchCommon

OK

>   * add class IdEq2 to Darcs.Patch.Ident
>   
>   This allows a faster equality test for FLs of patches with identity.

I'm a little skeptical of this one. The existing instances of Ident
(ignoring any forthcoming changes from you) already have fast equality
via their identities.

Also, the existing Eq2 for FLs actually commutes them, which for FL
(Named p) means that two lists would compare equal if one can be
reordered to the other [hopefully laziness would mean that the patch
bodies don't actually get commuted, though I wouldn't be sure of that.]

So I think for now your operators are equivalent to eqFL/eqFLRev.


>   * annotate all uses of anonymous with comments
>   
>   Whenever we call anonymous to construct a Named patch we must now check
>   that we don't accidentally store patches that result from merging them
>   with normal patches.

OK

>>   This is now no longer an official part of the Patch API.
> 
> We have an official Patch API??? :-)

Well, what other use would the Darcs.Patch re-exports have? It has
severely bit-rotten and lots of code outside Darcs.Patch imports
directly from sub-modules but I guess this was the original idea...

I guess the main problem is that the re-exports are so heavy on
maintenance, which is why people started not to bother to add things to
Darcs.Patch. Another disadvantage is that it makes it harder to find out
where things are defined. We should make a decision about this at some
point (and similarly about Darcs.Repository) and then either get rid of
them or actually remove all direct imports from sub-modules.

>>   * add class IdEq2 to Darcs.Patch.Ident
>>   
>>   This allows a faster equality test for FLs of patches with identity.
> 
> I'm a little skeptical of this one. The existing instances of Ident
> (ignoring any forthcoming changes from you) already have fast equality
> via their identities.
> 
> Also, the existing Eq2 for FLs actually commutes them, which for FL
> (Named p) means that two lists would compare equal if one can be
> reordered to the other [hopefully laziness would mean that the patch
> bodies don't actually get commuted, though I wouldn't be sure of that.]
> 
> So I think for now your operators are equivalent to eqFL/eqFLRev.

eqFL/eqFLRev sound unfamiliar to me. I may well have re-implemented some
code that already exists. Let us check that as soon as my V3/camp code
is in screened. I am always for simplifying things.

>>>   * add class IdEq2 to Darcs.Patch.Ident
>>>   
>>>   This allows a faster equality test for FLs of patches with identity.
>>
>> I'm a little skeptical of this one. The existing instances of Ident
>> (ignoring any forthcoming changes from you) already have fast equality
>> via their identities.
>>
>> Also, the existing Eq2 for FLs actually commutes them, which for FL
>> (Named p) means that two lists would compare equal if one can be
>> reordered to the other [hopefully laziness would mean that the patch
>> bodies don't actually get commuted, though I wouldn't be sure of that.]
>>
>> So I think for now your operators are equivalent to eqFL/eqFLRev.
> 
> eqFL/eqFLRev sound unfamiliar to me. I may well have re-implemented some
> code that already exists. Let us check that as soon as my V3/camp code
> is in screened. I am always for simplifying things.

Sorry, I was confused and answered without looking at the changes in detail.

Yes, Named patches already have fast equality. And I do think laziness
indeed helps a bit here, since in some, perhaps many cases commute fails
early in the process. But the new operators compare sets of PatchIds
without having to do /any/ commutation. That should, in general, be a
lot faster than what the existing Eq2 instance for FL does.

A possible source of confusion here may be that the patch in question
does not yet define any instances except the generic one for FLs. This
is in another patch (declare instances of IdEq2 for Named, NamedPrim,
and PatchInfoAnd) but that one depends on the V3 additions and thus I
did not add it to this bundle. At least for V3 patches this optimization
is absolutely essential for efficiency (via the NamedPrim instance) and
it may be helpful in other situations, too.

Re IdEq2: I think our equality story is already a bit complicated
and this makes it more so. I'll keep thinking about it while I look
at the V3 patches and maybe I'll come up with some more concrete
ideas to improve the situation. Accepting this for now, anyway.

> Re IdEq2: I think our equality story is already a bit complicated
> and this makes it more so.

I agree and I find it quite ugly that I had to introduce a new class for
this! I tried very hard to avoid it but couldn't find a way that works
generically for FLs or RLs of patches and automatically selects the
right instance without introducing overlapping instances; and I think we
both agree that we don't want to go down that particular rat hole...