11 patches for repository darcs-unstable@darcs.net:screened:

patch 7c6b7e121ca53756a73865149654511e0518e617
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Mon Feb 22 19:09:59 GMT 2016
  * add wrapper type around 'Named'
  
  At this point the wrapper is isomorphic to Named, preparing
  for a future refactoring where we will introduce a separate
  constructor to hold rebase internal patches.
  

patch bd8b8408e37820deea14c2116fe49515b63364d8
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Mon Feb 22 19:10:09 GMT 2016
  * flip dependency between Named.Wrapped and Rebase.Container
  
  This involves moving a bit of code out of Rebase.Container
  

patch 5c72a05b5198266f0089cfbab04aa342383ab1bf
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Mon Feb 22 19:12:19 GMT 2016
  * add nullary RepoType

patch 423219f14c6148cf904faa3f6516b2cf0591dde8
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Mon Feb 22 19:12:24 GMT 2016
  * stop Convert using Wrapped.patchcontents

patch 3198c5d8df23c2c1e26526a261df778b25d89fe4
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Mon Feb 22 19:12:26 GMT 2016
  * add 'activecontents' to replace 'patchcontents' for use in conflict resolution
  
  The semantics of 'activecontents' are more clearly defined, avoiding any
  uncertainty over how Suspended patches are treated.
  

patch 84e827518a763e67c51316d9e0cb161956325620
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Mon Feb 22 19:32:31 GMT 2016
  * Introduce RebaseP to replace Rebasing type
  
    NormalP (NamedP pi [] (Suspended s :>: NilFL))
  
  is now
  
    RebaseP pi s
  

patch 55107ba1d6f9eda243d23e02f9403f9316c6e545
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Mon Feb 22 19:33:11 GMT 2016
  * make the Rebase import qualified

patch c0cf83f01597b17f283dbcc335dbfe56d433cb00
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Mon Feb 22 19:33:20 GMT 2016
  * inline MaybeInternal module into Named.Wrapped

patch d706a1f844a072d20c6844ef9160f427b305b995
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Mon Feb 22 19:33:23 GMT 2016
  * drop NameHack

patch 6cc6748ed8b6eba6befd5e2691b5b617e2383843
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Mon Feb 22 19:33:25 GMT 2016
  * drop RecontextRebase

patch 174ddbc9f76f405e7b3b71033d689a185ee79c6c
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Mon Feb 22 19:33:27 GMT 2016
  * drop CarrierType - it can't ever be Rebasing p now

Attachments

• add-wrapper-type-around-_named_.dpatch
• patch-preview.txt
• unnamed

This bundle is the meat of the rebase refactoring required for the 
stash feature. If you have local changes, they will likely conflict, 
I'm afraid. The most likely conflict is that the 'PatchInfoAnd' and 
'Repository' types have acquired a new type parameter, 'rt'.

Please feel free to ping me if you need any help merging.

TLDR: everything looks good but I wonder if there was a simpler way of
      doing things.
      
      Also RepoType *needs* comments!
      
      Will wait for Ganesh's reaction and hopefully a comments patch
      before accepting :-)


(Sorry for the "free" formatting, I was juggling between the output of
darcs log, darcs diff | colordiff and darcs diff + meld, I'll do better
next time).

* add wrapper type around 'Named'

    adds module Darcs.Patch.Named.Wrapped, instances show that it's indeed
    a wrapper above Named.
    
    WrappedNamed has only one constructor yer: NamedP.
    
    Lots of code that was using Named now uses WrappedNamed
    (eg, PatchInfoAnd)
     
    Same for Patch.Rebase, Repository.Internal , etc.
    
    In UI.Rebase, use fromRebasing (serves to extract a non-rebase patch
    p from a WrappedNamed (Normal p)).
    
    I cannot claim that the changes are complete (ie that all modules
    that should be modified are), but at least they are correct.
    
    OK then.
    
* flip dependency between Named.Wrapped and Rebase.Container: OK
 
* add nullary RepoType: adds a type parameter to various Patch-related
   types, hence the change is sweeping (also to test harness).
   
   Adds dependency on GHC extensions DataKinds and ConstraintKinds.
   
   See
https://downloads.haskell.org/~ghc/7.6.3/docs/html/users_guide/promotion.html
   for DataKinds. Good read.
   
   Adds module Darcs.Patch.RepoType, which defines a datatype that's
   going to be used as a Kind. in the following:
   
   "data WrappedNamed (rt :: RepoType) p wX wY where"...
   
   in Darcs.Patch.Named.Wrapped; and
   
   "data PatchType (rt :: RepoType) (p :: * -> * -> *) = PatchType"
   
   in Darcs.Patch.Type; and
   
   "data Repository (rt :: RepoType) (p :: * -> * -> *)  wRercorded..."
   
   in Darcs.Repository.InternalTypes
   
   Also Repository and IdentifyRepo get the rt parameter.    
   
   In Darcs.UI.Commands.Annotate we have:
   
   unless (haveNonrangeMatch (PatchType :: PatchType 'RepoType
DummyPatch) matchFlags)
   
   so here's a first use of the RepoType datakind.
   
   This patch seems a "preparation" one for the moment since this kind
   is not really used.
   
   OK
   
   
* stop Convert using Wrapped.patchcontents: OK (patch name misleading?
   seems to me that  this patch is only a consequence of the
   "add wrapper type" patch)
   
* add 'activecontents' to replace 'patchcontents' for use in conflict
resolution:
    OK.
    (After this patch activecontents is exactly equal to patchcontents.
    The next patch removes this function.)
    
* Introduce RebaseP to replace Rebasing type:

   Now RepoType gets a lot of new stuff. It gets a parameter of type
   RebaseType, which is new, and defined as:
   
   data RebaseType = IsRebase | NoRebase
   
   We have new classes IsRebaseType and IsRepoType.
   
   Restriction (IsRepoType rt) *seems* to be added to all function
   signatures that involve Patch-related types.
   
   I don't really get the complexity of IsRepoType, SRepoType and
   SRebaseType .. is all of this necessary?
   
   The RepoType module could use more comments. Intuitively I get that
   all of this enables us to specify at the type level whether some
   operation can be done on repositories with rebase patches or not,
   but the module needs more explanation.

   As a consequence (but I don't get why), this removes the MaybeInternal
   class restriction to lots of functions of, eg, Darcs.Patch.Match.

   The patch also adds the RebaseP constructor to WrappedNamed.

   Heavy part: new functions in the same module:
   
   * fmapFL_WrappedNamed
       :: (FL p wA wB -> FL q wA wB)
          -> (RebaseTypeOf rt :~~: 'IsRebase -> p :~: q)
          -> WrappedNamed rt p wA wB
          -> WrappedNamed rt q wA wB
    fmapFL_WrappedNamed f _ (NormalP n) = NormalP (fmapFL_Named f n)
    fmapFL_WrappedNamed _ whenRebase (RebaseP n s) =
      case whenRebase ReflRebaseType of
        ReflPatch -> RebaseP n s

    I see from the implementation that pattern-matching in the "case"
    can fail. Why not explicitely use error?
   
   * fmapFLPIAP
      :: (FL p wX wY -> FL q wX wY)
         -> (RebaseTypeOf rt :~~: 'IsRebase -> p :~: q)
         -> PatchInfoAnd rt p wX wY
         -> PatchInfoAnd rt q wX wY
   
   `fmapFLPIAP` is used at: in Commands.Log and Patch.Bundle
   
   Looks good.
   
   Can't judge about the ReadPatch 'local hack', I'm going to trust the
   author.
   
   OK      
   
* make the Rebase import qualified: OK
* inline MaybeInternal module into Named.Wrapped: OK
* drop NameHack: OK,
* drop RecontextRebase: OK (same as previous, shorter)
* drop CarrierType - it can't ever be Rebasing p now: OK
  (it was an abstraction for apply/pull with or without --rebase)

Any hope for comments in the RepoType module Ganesh?

Apologies for the delay/silence, was distracted by some other stuff.

I've added some comments, but they're a bit mechanical, so let
me know if any more would help.

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

patch 975b59c91be3415a5f77e98d531d30af97bad57a
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Fri Apr  1 18:47:19 BST 2016
  * add some doc comments to RepoType

Attachments

• add-some-doc-comments-to-repotype.dpatch
• patch-preview.txt
• unnamed

Thanks, I think it's good like this!

On 22/03/2016 16:57, Guillaume Hoffmann wrote:

> * stop Convert using Wrapped.patchcontents: OK (patch name misleading?
>    seems to me that  this patch is only a consequence of the
>    "add wrapper type" patch)

Oops, yes. This patch no longer seems to have anything to do with the
patchcontents function :-) Originally it did, but I suspect that changed
as I refactored my patch sequence and I didn't notice.

> * Introduce RebaseP to replace Rebasing type:
> 
>    Now RepoType gets a lot of new stuff. It gets a parameter of type
>    RebaseType, which is new, and defined as:
>    
>    data RebaseType = IsRebase | NoRebase
>    
>    We have new classes IsRebaseType and IsRepoType.
>    
>    Restriction (IsRepoType rt) *seems* to be added to all function
>    signatures that involve Patch-related types.

Sort of - it's added to all functions that need to explicitly switch at
runtime on the repository type (rebase or no-rebase). That's not
necessarily all functions that use patches/repositories.

>    I don't really get the complexity of IsRepoType, SRepoType and
>    SRebaseType .. is all of this necessary?

I don't know of an alternative. TBH it's fairly simple type-level stuff,
but I appreciate it does add extra complexity.

>    The RepoType module could use more comments. Intuitively I get that
>    all of this enables us to specify at the type level whether some
>    operation can be done on repositories with rebase patches or not,
>    but the module needs more explanation.
> 
>    As a consequence (but I don't get why), this removes the MaybeInternal
>    class restriction to lots of functions of, eg, Darcs.Patch.Match.

Essentially because we can now explicitly pattern-match on RebaseP
instead of needing to use the MaybeInternal class to detect the rebase
container patch. That's one of the wins of this refactoring, it reduces
the hackery required for rebase.

>    The patch also adds the RebaseP constructor to WrappedNamed.
> 
>    Heavy part: new functions in the same module:
>    
>    * fmapFL_WrappedNamed
>        :: (FL p wA wB -> FL q wA wB)
>           -> (RebaseTypeOf rt :~~: 'IsRebase -> p :~: q)
>           -> WrappedNamed rt p wA wB
>           -> WrappedNamed rt q wA wB
>     fmapFL_WrappedNamed f _ (NormalP n) = NormalP (fmapFL_Named f n)
>     fmapFL_WrappedNamed _ whenRebase (RebaseP n s) =
>       case whenRebase ReflRebaseType of
>         ReflPatch -> RebaseP n s
> 
>     I see from the implementation that pattern-matching in the "case"
>     can fail. Why not explicitely use error?

Where exactly should I use error in this implementation?

There is a situation at the use sites where there's a function that can
never be called (with a non-bottom parameter) and I'd like to use an
empty case expression to express that, but can't yet because of
library/GHC versions.

Cheers,

Ganesh