[darcs-devel] [patch479] Tighten up type in Darcs.Rollback (and 14 more)

Eric Kow kowey at darcs.net
Fri Jan 7 16:18:02 UTC 2011


I'd neglected to review this last patch because it depended on patch426,
which was a pretty foolish on my part.

I'll try to push this in, dependencies permitting.
Chatter only below.

Random aside about workflow
---------------------------
As I review these patches, I like to describe the gradual evolution of
my workflow as I slowly figure out how to use screened/patch-tracker
more effectively.  I'm not writing it up properly because it keeps
changing.  Hopefully it'll settle down...

First change is that I should review for understanding first, and worry
about the dependencies later.  Second, since I seem to like reviewing
against context, I made an alias to make it more convenient:

   cd /tmp/patch479
   darcs-context ../tighten-up-type-in-darcs_rollback.dpatch
   # does darcs get http://darcs.net/screened --lazy --context tighten
   cd tighten
   darcs apply -i ../../tighten-up-type-in-darcs_rollback.dpatch
   darcs-gdiff --last=1

To grab my aliases,

   darcs get http://code.haskell.org/darcs/darcs-team

> Mon Nov 22 06:42:50 GMT 2010  Ganesh Sittampalam <ganesh at earth.li>
>   * split a basic class out of ShowPatch that requires fewer deps
>   This allows a bunch of unreachable and duplicated code to be removed

split a basic class out of ShowPatch that requires fewer deps
-------------------------------------------------------------
> hunk ./src/Darcs/Patch/Show.lhs 23
> -class ShowPatch p where
> +class ShowPatchBasic p where
>      showPatch :: p C(x y) -> Doc
> hunk ./src/Darcs/Patch/Show.lhs 59
> +
> +class ShowPatchBasic p => ShowPatch p where
>      showNicely :: p C(x y) -> Doc
>      showNicely = showPatch
>      -- | showContextPatch is used to add context to a patch, as diff

Here's the bulk of our patch.  We're splitting ShowPatch into
ShowPatchBasic which provides showPatch and ShowPatch which does all the
fancy stuff and requires more typeclass deps in practice.  This makes it
easier for Braced, which only needs the basic stuff

The patch only looks big, but basically here's what's going on.

  type          Basic  Fancy
  -------       -----  -----
  Prim          X      X
  Named p       X      X
  FL/RL p       X      X
  PatchInfoAnd  X      X
  Patch (V1)    X      X
  Real  (V2)    X      X
  FLM           X            (no braced around singletons)
  Braced p      X
  --------      -----  -----

I've snipped the rest

> hunk ./src/Darcs/Patch/Show.lhs 79
> -writePatch :: ShowPatch p => FilePath -> p C(x y) -> IO ()
> +writePatch :: ShowPatchBasic p => FilePath -> p C(x y) -> IO ()
> hunk ./src/Darcs/Patch/Show.lhs 81
> -gzWritePatch :: ShowPatch p => FilePath -> p C(x y) -> IO ()
> +gzWritePatch :: ShowPatchBasic p => FilePath -> p C(x y) -> IO ()

Not everything needs the full ShowPatch

> hunk ./src/Darcs/Patch/Viewing.hs 258
> -instance (Apply p, Conflict p, PatchListFormat p, ShowPatch p) => ShowPatch (Named p) where
> +instance (PatchListFormat p, ShowPatchBasic p) => ShowPatchBasic (Named p) where

ShowPatchBasic instance for Named p has simpler requirements but
ShowPatch requirements are still tricky.

> hunk ./src/Darcs/Patch/Braced/Instances.hs 13
> -instance MyEq (Braced p) where
> -    unsafeCompare = impossible
> -
> -instance Apply (Braced p) where
> -    apply = impossible
> -
> -instance Conflict (Braced p) where
> -    listConflictedFiles = impossible
> -    resolveConflicts = impossible
> -    commuteNoConflicts = impossible
> -    conflictedEffect = impossible
> -    isInconsistent = impossible
> -
> -instance Effect (Braced p) where
> -    effect = impossible
> -
> -instance Commute (Braced p) where
> -    commute = impossible
> -
> -instance Invert (Braced p) where
> -    invert = impossible
> -    identity = impossible
> -
> -instance ShowPatch p => ShowPatch (Braced p) where
> +instance ShowPatchBasic p => ShowPatchBasic (Braced p) where
>      showPatch (Singleton p) = showPatch p
>      showPatch (Braced NilFL) = blueText "{" $$ blueText "}"
>      showPatch (Braced ps) = blueText "{" $$ vcat (mapFL showPatch ps) $$ blueText "}"

Here's why we're doing this.

> -hashBundle :: RepoPatch p => [PatchInfo] -> FL (Named p) C(x y) -> String
> +hashBundle :: (PatchListFormat p, ShowPatchBasic p) => [PatchInfo] -> FL (Named p) C(x y) -> String
>  hashBundle _ to_be_sent = sha1PS $ renderPS
>                           $ vcat (mapFL showPatch to_be_sent) <> newline
>  
> hunk ./src/Darcs/Patch/Bundle.hs 64
> -hashBundleBraced :: RepoPatch p => [PatchInfo] -> FL (Named (Braced p)) C(x y) -> String
> -hashBundleBraced _ to_be_sent = sha1PS $ renderPS
> -                         $ vcat (mapFL showPatch to_be_sent) <> newline

And here's another reason.  We didn't have a single hashBundle that
takes ShowPatch because implementing ShowPatch for Braced would have
been a pain.

> hunk ./src/Darcs/Repository/LowLevel.hs 80
>  instance ReadPatch p => ReadPatch (FLM p) where
>     readPatch' = mapSeal FLM <$> readMaybeBracketedFL readPatch' '{' '}'
>  
> -instance ShowPatch p => ShowPatch (FLM p) where
> +instance ShowPatchBasic p => ShowPatchBasic (FLM p) where
>     showPatch = showMaybeBracketedFL showPatch '{' '}' . unFLM
>  
>  readPendingContents :: BS.ByteString -> Sealed (FL Prim C(x))

And another reason.

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, try +44 (0)1273 64 2905 or
xmpp:kowey at jabber.fr (Jabber or Google Talk only)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.osuosl.org/pipermail/darcs-devel/attachments/20110107/d4f3d6e2/attachment.pgp>


More information about the darcs-devel mailing list