12 patches for repository /home/ben/src/darcs/without-v3:

patch 39e45329fa32f4a682ba1e557ee756a7a1d74327
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Feb  8 17:46:49 CET 2019
  * renamed fromPrim to fromAnonymousPrim

patch 9e6f89e0479a740cf4b7ff28150f5f9d284cb400
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Feb  8 17:46:49 CET 2019
  * add prim patch identifier when constructing Named patches
  
  This re-adds method fromPrim to class FromPrim, this time with additional
  parameters to construct the identifier. This is then used in function
  infopatch instead of fromAnonymousPrim.

patch d0bbd4aa6e41c41459f09e64239d74f2e64d5850
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Feb  8 17:46:49 CET 2019
  * move Darcs.Patch.V3.Prim to Darcs.Patch.Prim.Named
  
  This is actually a fully generic wrapper for any PrimPatch type and
  technically not tied to V3.

patch 3a797b9fc225df6dbb031b29307f4e30c891fa6c
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Feb  8 17:46:49 CET 2019
  * add new Darcs3 format option

patch f6b41d01a4487e2d5004f5175113d9dfd90ecc98
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Feb  8 17:46:49 CET 2019
  * trivial refactor in D.R.Create

patch 0107cb750c63795b3cd1fb17f94478102ab7dc5d
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Feb  8 17:46:49 CET 2019
  * allow runnign the test scripts with darcs-3 format

patch 47609c8e55be5be81dfed655514762c51140f343
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Feb  8 17:46:49 CET 2019
  * adapt test skip based on repo format
  
  Now that we have three formats, we can no longer assume that skip-formats
  darcs-1 implies test is run only for darcs-2.

patch fffd7cd29873f3c9f5529c79bb859ac86e51887a
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Feb 10 19:28:06 CET 2019
  * fix cloning of darcs-3 format repos

patch 7cd237bf9a779540fe19c332e759e359a1b4cb0d
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Feb 10 21:48:03 CET 2019
  * bugfix in instance ShowContextPatch for NamedPrim

patch dc879f6671f4dbff633ebeeb635a83e8d05337ac
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Feb 10 23:09:51 CET 2019
  * hack: disable showContextSeries for darcs-3 format
  
  The code (in Darcs.Patch.Viewing) that implements showContextSeries messily
  re-implements showing for FLs of primitive patches (hunks in particular),
  circumventing the ShowPatchBasic instance. This means that it cannot take
  prim patch IDs into account since these are added to prim patches as a
  wrapper. This would not be a big problem for display but it is one when we
  use showContextPatch to create a patch bundle because it means that hunks
  are stored without the prim patch ID and thus cannot be read.
  
  The temporary "fix" here is to disable showContextSeries by adding a new
  ListFormatV3, until the context showing code is cleaned up to pass the
  correct prim show function down into showContextSeries.

patch dcceaa03b5ae8ce0c7670e056a1f6ef9eec8aa16
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Feb 12 18:17:39 CET 2019
  * show the PrimPatchId only ForStorage, not ForDisplay
  
  The PrimPatchId is not useful to the user and clutters up the output when
  --verbose is in effect. It also allows more test scripts to pass without
  having to adapt them.

patch e46cc62d1d40c0f98d33b7d850efcc172d3b6be6
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Feb 13 18:20:58 CET 2019
  * fix rebase unsuspend for v3 with a dirty hack
  
  See comment in the code for the why and how.

Attachments

• patch-preview.txt
• renamed-fromprim-to-fromanonymousprim.dpatch
• unnamed

On 14/06/2019 20:55, Ben Franksen wrote:
> 
> New submission from Ben Franksen <ben.franksen@online.de>:
> 
> 12 patches for repository /home/ben/src/darcs/without-v3:
> 
> patch 39e45329fa32f4a682ba1e557ee756a7a1d74327
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Fri Feb  8 17:46:49 CET 2019
>   * renamed fromPrim to fromAnonymousPrim

OK

> 
> patch 9e6f89e0479a740cf4b7ff28150f5f9d284cb400
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Fri Feb  8 17:46:49 CET 2019
>   * add prim patch identifier when constructing Named patches
>   
>   This re-adds method fromPrim to class FromPrim, this time with additional
>   parameters to construct the identifier. This is then used in function
>   infopatch instead of fromAnonymousPrim.

> +    fromPrim :: SHA1 -> Int -> PrimOf p wX wY -> p wX wY

I feel this signature is a bit too operational, but that's really a
consequence of discussions we're already having about SHA1s and the prim
patch ids.

> +    fromPrim _ _ = fromAnonymousPrim

I'm instinctively against having this default implementation, as it
makes it easier to accidentally use it in a case where it shouldn't
be.

Otherwise fine.

> patch d0bbd4aa6e41c41459f09e64239d74f2e64d5850
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Fri Feb  8 17:46:49 CET 2019
>   * move Darcs.Patch.V3.Prim to Darcs.Patch.Prim.Named
>   
>   This is actually a fully generic wrapper for any PrimPatch type and
>   technically not tied to V3.

OK

> patch 3a797b9fc225df6dbb031b29307f4e30c891fa6c
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Fri Feb  8 17:46:49 CET 2019
>   * add new Darcs3 format option

OK (feels like our types for repository formats could be improved, e.g.
to make it clear that Darcs3 and Darcs2 are mutually exclusive, but not
an essential part of this change)

> patch f6b41d01a4487e2d5004f5175113d9dfd90ecc98
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Fri Feb  8 17:46:49 CET 2019
>   * trivial refactor in D.R.Create

OK, though I'm not sure if the 'here' indirection is good or not.

> patch 0107cb750c63795b3cd1fb17f94478102ab7dc5d
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Fri Feb  8 17:46:49 CET 2019
>   * allow runnign the test scripts with darcs-3 format

OK

> patch 47609c8e55be5be81dfed655514762c51140f343
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Fri Feb  8 17:46:49 CET 2019
>   * adapt test skip based on repo format
>   
>   Now that we have three formats, we can no longer assume that skip-formats
>   darcs-1 implies test is run only for darcs-2.

OK

> patch fffd7cd29873f3c9f5529c79bb859ac86e51887a
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Sun Feb 10 19:28:06 CET 2019
>   * fix cloning of darcs-3 format repos

OK

> patch 7cd237bf9a779540fe19c332e759e359a1b4cb0d
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Sun Feb 10 21:48:03 CET 2019
>   * bugfix in instance ShowContextPatch for NamedPrim

OK

> patch dc879f6671f4dbff633ebeeb635a83e8d05337ac
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Sun Feb 10 23:09:51 CET 2019
>   * hack: disable showContextSeries for darcs-3 format
>   
>   The code (in Darcs.Patch.Viewing) that implements showContextSeries messily
>   re-implements showing for FLs of primitive patches (hunks in particular),
>   circumventing the ShowPatchBasic instance. This means that it cannot take
>   prim patch IDs into account since these are added to prim patches as a
>   wrapper. This would not be a big problem for display but it is one when we
>   use showContextPatch to create a patch bundle because it means that hunks
>   are stored without the prim patch ID and thus cannot be read.
>   
>   The temporary "fix" here is to disable showContextSeries by adding a new
>   ListFormatV3, until the context showing code is cleaned up to pass the
>   correct prim show function down into showContextSeries.

OK

> patch dcceaa03b5ae8ce0c7670e056a1f6ef9eec8aa16
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Tue Feb 12 18:17:39 CET 2019
>   * show the PrimPatchId only ForStorage, not ForDisplay
>   
>   The PrimPatchId is not useful to the user and clutters up the output when
>   --verbose is in effect. It also allows more test scripts to pass without
>   having to adapt them.

OK

> patch e46cc62d1d40c0f98d33b7d850efcc172d3b6be6
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Wed Feb 13 18:20:58 CET 2019
>   * fix rebase unsuspend for v3 with a dirty hack
>   
>   See comment in the code for the why and how.

OK :-)

It might be nice to flag places in the code that need further
attention within a definite time horizon somehow, so we can validate
that it happens. But not a big deal as I'm sure you're keeping track
anyway.

Ganesh

>> +    fromPrim :: SHA1 -> Int -> PrimOf p wX wY -> p wX wY
> 
> I feel this signature is a bit too operational, but that's really a
> consequence of discussions we're already having about SHA1s and the prim
> patch ids.

Right. This is an ugly ad-hoc solution and should be regarded as
preliminary until we have a better grip on how we want to handle
PrimPatchIds and named prims in general. You may want to add a FIXME
comment here.

>> +    fromPrim _ _ = fromAnonymousPrim
> 
> I'm instinctively against having this default implementation, as it
> makes it easier to accidentally use it in a case where it shouldn't
> be.

I had exactly the same instinctive reaction. One the other hand,
defining this method manually inside the old RepoPatch types makes it
harder to change the API, so again I'd say refactor this when we have a
clearer idea what we want here.

>>   * add new Darcs3 format option
> 
> OK (feels like our types for repository formats could be improved, e.g.
> to make it clear that Darcs3 and Darcs2 are mutually exclusive, but not
> an essential part of this change)

Fully agreed.

>>   * trivial refactor in D.R.Create
> 
> OK, though I'm not sure if the 'here' indirection is good or not.

I hate repeating "magical constants" in code. BTW, the interface is
horrible anyway: we take a string and assume that if it is "." then we
have a local repo, else it is remote. We should use a proper data type
but doing this right is a somewhat larger refactor. Yet another FIXME.

>>   * fix rebase unsuspend for v3 with a dirty hack
>>   
>>   See comment in the code for the why and how.
> 
> OK :-)

This hack wouldn't be needed if Rebase used (NamedPrim prim) internally,
instead of plain prims. But I couldn't see an easy way to make it do so
for RepoPatchV3 and still work generically for all RepoPatch types. As I
said elsewhere, this would require a generic "PotentiallyNamedPrim"
interface...

> It might be nice to flag places in the code that need further
> attention within a definite time horizon somehow, so we can validate
> that it happens. But not a big deal as I'm sure you're keeping track
> anyway.

I am trying to keep track but regularly fail :-( so appreciate that you
point them out during review.

I like use lentil (https://hackage.haskell.org/package/lentil) for
things like that. We could add "ASAP" as a qualifier ;-) since lentil
reports /lots/ of FIXME and TODO items when you run it on the Darcs
sources...

>> It might be nice to flag places in the code that need further
>> attention within a definite time horizon somehow, so we can validate
>> that it happens. But not a big deal as I'm sure you're keeping track
>> anyway.
> 
> I am trying to keep track but regularly fail :-( so appreciate that you
> point them out during review.
> 
> I like use lentil (https://hackage.haskell.org/package/lentil) for
> things like that. We could add "ASAP" as a qualifier ;-) since lentil
> reports /lots/ of FIXME and TODO items when you run it on the Darcs
> sources...

How about V3INTEGRATION? Or V3INTEG if we want something shorter. That
makes it clear it's related to this specific milestone.

$ lentil . -t v3integration -w V3INTEGRATION

seems to do what we'd want.

Marking this as accepted. There are two followups I want to flag
if we agree on the V3INTEGRATION tag or similar:
 - make the new fromPrim not have a default implementation
 - improve/review the rebase unsuspend hack