[darcs-devel] [patch484] split out IsHunk from Effect (and 23 more)

Eric Kow kowey at darcs.net
Thu Jan 6 17:27:34 UTC 2011


On Tue, Nov 23, 2010 at 23:18:37 +0000, Ganesh Sittampalam wrote:
>   * hide the internals of Prim
>   * move FileNameFormat out of Prim
>   * move Prim instances for Show into Prim.Show
>   * simplify ShowPatch constraints
>   * clean up some unused exports
>   * given token replacement its own module
>   * use API function for identity patch
>   * simplify Effect instances

Review part 2.  All look good, nothing but chatter below.
Accepted pending deps.

hide the internals of Prim
--------------------------
Quite happy to see this one

> hunk ./src/Darcs/Patch/FileHunk.hs 12
> -import Darcs.Patch.Prim ( Prim, primIsHunk )
> +data FileHunk C(x y) = FileHunk !FileName !Int [B.ByteString] [B.ByteString]

Compare with

   Hunk !Int [B.ByteString] [B.ByteString]

Basically we represent the 9 prim patch types with 3 types:

  Prim
   |- FilePatchType (5 patch types)
   |- DirPatchType  (2 patch types)
   |- (move)
   |- (setpref)

We have several places in the code that want to talk about hunk patches
including the file they affect.  We could pattern-matches using the two
layer FP (Hunk ...), but then we'd have functions that need to accept the
other 8 prim patches eg. with impossible.  Using the notion of FileHunk makes
the code a bit safer by letting us write functions that only deal with this
common case without having to worry about the rest.

>  class IsHunk p where
> hunk ./src/Darcs/Patch/FileHunk.hs 15
> -    isHunk :: p C(x y) -> Maybe (Prim C(x y))
> -
> -instance IsHunk Prim where
> -    isHunk p = if primIsHunk p then Just p else Nothing
> +    isHunk :: p C(x y) -> Maybe (FileHunk C(x y))

If you want to know if we have a hunk patch, you're likely going to want
a FileHunk

> hunk ./src/Darcs/Patch/ConflictMarking.hs 16
> -import Darcs.Patch.Prim ( Prim(..), FilePatchType(..), primIsHunk )
> +import Darcs.Patch.Prim ( Prim, is_filepatch, primIsHunk, primFromHunk )

I usually treat imports as boring in patch review, but here the story is
no constructors imported, which seems notable.

Hmm, wonder how is_filepatch was spared from the semi-automated camel
caser.

>  applyHunks :: [Maybe B.ByteString] -> FL Prim C(x y) -> [Maybe B.ByteString]
> -applyHunks ms (FP _ (Hunk l o n):>:ps) = applyHunks (rls l ms) ps
> +applyHunks ms ((isHunk -> Just (FileHunk _ l o n)):>:ps) = applyHunks (rls l ms) ps

>  getAFilename :: [Sealed (FL Prim C(x))] -> FileName
> -getAFilename ((Sealed (FP f _:>:_)):_) = f
> +getAFilename ((Sealed ((is_filepatch -> Just f):>:_)):_) = f

Just some hiding here with the help of view patterns which I'm sure I'll
start using for my own stuff after learning from example.
  
>  mangleUnravelledHunks pss =
> -                     else Sealed (FP filename (Hunk l old new))
> +                     else Sealed (primFromHunk (FileHunk filename l old new))

> hunk ./src/Darcs/Patch/Prim.hs 2
>  module Darcs.Patch.Prim
> -       ( Prim(..), showPrim, showPrimFL,
> -         DirPatchType(..), FilePatchType(..),
> +       ( Prim, showPrim, showPrimFL,

Again, notice the hiding here.  Darcs code slowly maturing, starting to
wear clothing instead of running around naked.  Now to make patches we have to
go through official channels, using functions like 'hunk' or 'addfile'

> hunk ./src/Darcs/Patch/Prim/Core.lhs 139
> +instance IsHunk Prim where
> +   isHunk (FP fn (Hunk line before after)) = Just (FileHunk fn line before after)
> +   isHunk _ = Nothing
> +
> +primFromHunk :: FileHunk C(x y) -> Prim C(x y)
> +primFromHunk (FileHunk fn line before after) = FP fn (Hunk line before after)

There are cases where we need to tweak a hunk patch, which primFromHunk lets us
do, what with all those exports hidden.
  
> hunk ./src/Darcs/Patch/V1/Viewing.hs 20
>  instance ShowPatch Patch where
> -    showContextPatch (PP x) | primIsHunk x = showContextHunk x
> +    showContextPatch (PP (isHunk -> Just fh)) = showContextHunk fh

... things that look like this.
> hunk ./src/Darcs/Patch/Viewing.hs 64
>  
>  showContextSeries :: (Apply p, ShowPatch p, IsHunk p) => FL p C(x y) -> TreeIO Doc
>  showContextSeries patches = scs Nothing patches
> -    where scs :: (Apply p, ShowPatch p, IsHunk p) => Maybe (Prim C(w x)) -> FL p C(x y) -> TreeIO Doc
> +    where scs :: (Apply p, ShowPatch p, IsHunk p) => Maybe (FileHunk C(w x)) -> FL p C(x y) -> TreeIO Doc

And hey now we get a tiny bit of extra safety. Now there's no way scs
will ever get one of those other 8 prim patch types without us knowing
it.

> hunk ./src/Darcs/Patch/Viewing.hs 79
> --- |Thist must only be called with a hunk patch
> -showContextHunk :: Prim C(x y) -> TreeIO Doc
> +showContextHunk :: FileHunk C(x y) -> TreeIO Doc

And look!  This is the kind of case where we're happier to have less
documentation instead of more.

Good:   write documentation
Better: remove need for documentation

> hunk ./src/Darcs/Patch/Viewing.hs 113
> -coolContextHunk _ _ _ = impossible

And more of the kind of thing we want to see in our code.  Sealing up
places where things could blow up.

move FileNameFormat out of Prim
-------------------------------
>  Although this is essentially a legacy concept, both Prim and patch types
>  that build on Prim need to know about it, so it's cleanest to have it
>  somewhere central.

OK

> hunk ./src/Darcs/Patch/Format.hs 30
>                        -- Read with arbitrary nested braces and parens and flatten them out.
>    | ListFormatV2      -- ^Show lists without braces
>                        -- Read with arbitrary nested parens and flatten them out.
> +
> +data FileNameFormat = OldFormat | NewFormat

Hunk moved from Darcs.Patch.Prim.Core

We should probably haddock this on a rainy day if somebody can be bothered
to track down all the discussion and history and boil it down to some nice
concise motivation haddock.

> -readFileName :: FileNameFormat -> B.ByteString -> FileName
> -readFileName OldFormat = ps2fn
> -readFileName NewFormat = fp2fn . decodeWhite . BC.unpack

> -formatFileName :: FileNameFormat -> FileName -> Doc
> -formatFileName OldFormat = packedString . fn2ps
> -formatFileName NewFormat = text . encodeWhite . fn2fp

And these are hunk-moved from Darcs.Patch.Prim.{Read,Show} up to
                              Darcs.Patch.{Read,Show}

move Prim instances for Show into Prim.Show
-------------------------------------------
Not completely sure I understand the story her.

> Ganesh Sittampalam <ganesh at earth.li>**20101123074227
>  Ignore-this: f55b3e37f27521311dfe9a0912d77d26
> ] hunk ./src/Darcs/Patch/Conflict.hs 10
> -import Darcs.Patch.Prim ( Prim )
> +import Darcs.Patch.Prim.Commute ()
> +import Darcs.Patch.Prim.Core ( Prim )

I *think* this and other changes like it is because now we want to be
careful not to import the instance which is exposed from Prim.Show.

Why exactly I'm not entirely sure

> hunk ./src/Darcs/Patch/FileHunk.hs 2
>  module Darcs.Patch.FileHunk
> -    ( FileHunk(..), IsHunk(..)
> +    ( FileHunk(..), IsHunk(..), showFileHunk

> +showFileHunk :: FileNameFormat -> FileHunk C(x y) -> Doc
> +showFileHunk x (FileHunk f line old new) =
> +           blueText "hunk" <+> formatFileName x f <+> text (show line)
> +        $$ lineColor Magenta (prefix "-" (vcat $ map userchunkPS old))
> +        $$ lineColor Cyan    (prefix "+" (vcat $ map userchunkPS new))

I think I have some slight misgivings about all that patch show code no
longer being in one place, I guess because we don't have any
documentation on patch formats...  but let's see where this goes.

Note that this was moved from a "scattered" place Darcs.Patch.Prim.Show
anyway, which I think I accepted earlier in this patch bundle, so it's
not making things any worse.  I guess to group and layer things nicely
on one axis sometimes you have to slightly sacrifice a bit of grouping on
another axis.

> hunk ./src/Darcs/Patch/Prim/Show.lhs 35
>  -- TODO this instance shouldn't really be necessary, as Prims aren't used generically
>  instance PatchListFormat Prim
>  
> +instance ShowPatchBasic Prim where
> +    showPatch = showPrim OldFormat
> +
> +instance ShowPatch Prim where
> +    showContextPatch (isHunk -> Just fh) = showContextHunk fh
> +    showContextPatch p = return $ showPatch p
> +    summary = plainSummaryPrim
> +    thing _ = "change"

This being moved from Darcs.Patch.Viewing does seem nicer
> -instance ShowPatchBasic Prim where
> -    showPatch = showPrim OldFormat

> -    Nothing -> return $ showPatch $ primFromHunk fh -- This is a weird error...
> +    Nothing -> return $ showFileHunk OldFormat fh -- This is a weird error...

We don't know anything about Prims in this module anymore.

------------------------------
simplify ShowPatch constraints
clean up some unused exports
use API function for identity patch
------------------------------

No comments

given token replacement its own module
--------------------------------------
> addfile ./src/Darcs/Patch/TokenReplace.hs
> hunk ./src/Darcs/Patch/TokenReplace.hs 1

More of the same business of sweeping together bits of code that
seem to belong together but are scattered throughout.

This new module is about token replacement (not the patches but the
low-level fiddling that makes it happen).

I suppose it could also swallow up the Darcs.Patch.RegChars module
if we wanted it to

> +tryTokInternal :: String -> B.ByteString -> B.ByteString
> +                 -> B.ByteString -> Maybe [B.ByteString]

This comes from Darcs.Patch.Prim.Core

> +forceTokReplace :: String -> String -> String
> +                -> B.ByteString -> Maybe B.ByteString

And this is from Darcs.Patch.Prim.Apply

simplify Effect instances
-------------------------
> Ganesh Sittampalam <ganesh at earth.li>**20101123074242
>  Ignore-this: 82d766c2858b91b047934092e1fa75ec
> ] hunk ./src/Darcs/Patch/V1/Commute.lhs 658
>  instance Effect Patch where
> -    effect (PP p) = effect p
> +    effect (PP p) = p :>: NilFL

That's simpler?  I guess this communicates better that hey we're just
dealing with Prims here.  No need to see how far the ball of yarn goes,
we're done.

> hunk ./src/Darcs/Patch/V2/Real.hs 786
>  instance Effect RealPatch where
>      effect (Duplicate _) = NilFL
>      effect (Etacilpud _) = NilFL
> -    effect (Normal p) = effect p
> +    effect (Normal p) = p :>: NilFL
> hunk ./src/Darcs/Patch/V2/Real.hs 791
> -    effectRL (Normal p) = effectRL p
> +    effectRL (Normal p) = p :<: NilRL

-- 
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: not available
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <http://lists.osuosl.org/pipermail/darcs-devel/attachments/20110106/b3186cbd/attachment.pgp>


More information about the darcs-devel mailing list