A bunch more refactorings and cleanups.

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

Mon Nov 22 06:43:20 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * split out IsHunk from Effect

Tue Nov 23 07:23:03 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * split out Darcs.Patch.{Effect,Conflict}

Tue Nov 23 07:23:06 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * remove dead code

Tue Nov 23 07:23:16 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * abstract MarkedUpFile etc over PatchInfo

Tue Nov 23 07:23:22 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * break up Summary so Prim is handled internally

Tue Nov 23 07:23:24 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * move markupFile etc out of Darcs.Patch.Apply

Tue Nov 23 07:23:25 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * move Darcs.Population[Data] into Darcs.Patch namespace

Tue Nov 23 07:23:27 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * give applyFL a better name

Tue Nov 23 07:23:28 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * abstract Population over PatchInfo

Tue Nov 23 07:23:29 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * move Population handling out of Darcs.Patch.Apply

Tue Nov 23 07:23:31 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * move Apply Prim code into separate module

Tue Nov 23 07:25:43 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * move read of Prim into its own module

Tue Nov 23 07:25:47 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * move coalescing code out of Darcs.Patch.Prim.Core

Tue Nov 23 07:25:49 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * move showPrim etc out of Darcs.Patch.Prim.Core

Tue Nov 23 07:25:50 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * extract Darcs.Patch.Prim.Commute

Tue Nov 23 07:27:23 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * move IsHunk into its own module

Tue Nov 23 07:27:41 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * hide the internals of Prim

Tue Nov 23 07:42:02 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * 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.

Tue Nov 23 07:42:27 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * move Prim instances for Show into Prim.Show

Tue Nov 23 07:42:37 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * simplify ShowPatch constraints

Tue Nov 23 07:42:38 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * clean up some unused exports

Tue Nov 23 07:42:39 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * given token replacement its own module

Tue Nov 23 07:42:41 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * use API function for identity patch

Tue Nov 23 07:42:42 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * simplify Effect instances

Attachments

• split-out-ishunk-from-effect.dpatch
• unnamed

This patch bundle (with 24 patches) was just applied to the repository http://darcs.net/screened.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_screened.html#bundle-43cf36582bde96b297c5dc87c33f3e59de27d829

On Tue, Nov 23, 2010 at 23:18:37 +0000, Ganesh Sittampalam wrote:
> A bunch more refactorings and cleanups.

I'm halfway through this.  Just marking my progress until Monday.

>   * split out IsHunk from Effect
>   * split out Darcs.Patch.{Effect,Conflict}
>   * remove dead code
>   * abstract MarkedUpFile etc over PatchInfo
>   * break up Summary so Prim is handled internally
>   * move Darcs.Population[Data] into Darcs.Patch namespace
>   * give applyFL a better name
>   * abstract Population over PatchInfo
>   * move Population handling out of Darcs.Patch.Apply
>   * move Apply Prim code into separate module
>   * move read of Prim into its own module
>   * move coalescing code out of Darcs.Patch.Prim.Core
>   * move showPrim etc out of Darcs.Patch.Prim.Core
>   * extract Darcs.Patch.Prim.Commute
>   * move IsHunk into its own module

All look good.  My only complaint is that we're missing license
boilerplate on a lot of the new modules we're creating.

My comments below are just chatter, should be perfectly fine to
ignore them.

Because these involve a lot of hunk-moving, I found it helpful to use

  darcs diff --diff-command="mv %1 %2 /tmp" --last=1
  darcs changes --summary --last=1
  cd /tmp
  meld old-patch484/src/foo.hs new-patch484/src/bar.hs

>   * 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

split out IsHunk from Effect
----------------------------
> hunk ./src/Darcs/Patch/Prim/Core.lhs 1023
>      effect = reverseRL . effectRL
>      effectRL :: p C(x y) -> RL Prim C(x y)
>      effectRL = reverseFL . effect
> +
> +class IsHunk p where
>      isHunk :: p C(x y) -> Maybe (Prim C(x y))
> hunk ./src/Darcs/Patch/Prim/Core.lhs 1026
> -    isHunk _ = Nothing
>  
>  instance Effect Prim where
>      effect p = p :>: NilFL
> hunk ./src/Darcs/Patch/Prim/Core.lhs 1030
>      effectRL p = p :<: NilRL
> +
> +instance IsHunk Prim where
>      isHunk p = if primIsHunk p then Just p else Nothing

Here's the meat of this patch.  Note the disappearance of the
isHunk default implementation which ...

> hunk ./src/Darcs/Patch/Named.lhs 44
> +instance IsHunk (Named p) where
> +    isHunk _ = Nothing

> +instance IsHunk (PatchInfoAnd p) where
> +    isHunk _ = Nothing

Shows up here and here.  Any reason not to have a default implementation
anymore?  Guess it's safer to avoid accidental fall-through.

> hunk ./src/Darcs/Patch/PatchInfoAnd.hs 39
> -instance (Apply p, Conflict p, Effect p, PatchListFormat p, ShowPatch p) => ShowPatch (PatchInfoAnd p) where
> +instance (Apply p, Conflict p, IsHunk p, PatchListFormat p, ShowPatch p) => ShowPatch (PatchInfoAnd p) where
  
> hunk ./src/Darcs/Patch/V1/Commute.lhs 657
>      effect (PP p) = effect p
> +
> +instance IsHunk Patch where
>      isHunk p = do PP p' <- return p
>                    isHunk p'
> hunk ./src/Darcs/Patch/V2/Real.hs 792
>      effectRL (InvConflictor _ e _) = reverseFL e
> +
> +instance IsHunk RealPatch where
>      isHunk rp = do Normal p <- return rp
>                     isHunk p

More instance separation

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

IsHunk used to come from Effect via Conflict

The rest of it seems fairly straightforward.

split out Darcs.Patch.{Effect,Conflict}
---------------------------------------
> Ganesh Sittampalam <ganesh@earth.li>**20101123072303
>  Ignore-this: 17fb8d3d32bc0b76e1acd47f67a7ba56
> ] hunk ./darcs.cabal 200
>                        Darcs.Patch.Bundle
>                        Darcs.Patch.Choices
>                        Darcs.Patch.Commute
> +                      Darcs.Patch.Conflict
>                        Darcs.Patch.ConflictMarking
>                        Darcs.Patch.Depends
> hunk ./darcs.cabal 203
> +                      Darcs.Patch.Effect
>                        Darcs.Patch.FileName
>                        Darcs.Patch.Format
>                        Darcs.Patch.Info

This basically says it all.

> addfile ./src/Darcs/Patch/Conflict.hs
> hunk ./src/Darcs/Patch/Conflict.hs 1

> addfile ./src/Darcs/Patch/Effect.hs
> hunk ./src/Darcs/Patch/Prim/Core.lhs 926

Hunk-moved from src/Darcs/Patch/Prim/Core.lhs with imports tidied up.
Also us slowly moving away from Literate Haskell.

remove dead code
----------------
> Ganesh Sittampalam <ganesh@earth.li>**20101123072306
>  Ignore-this: 1ed64d161324b01c3bea241ae814cf09
> ] hunk ./src/Darcs/Patch.lhs 80
> -               DirMark(..), patchChanges, applyToPop,
> +               DirMark(..), applyToPop,

> -import Darcs.Patch.Apply ( applyToPop, patchChanges, emptyMarkedupFile,
> +import Darcs.Patch.Apply ( applyToPop, emptyMarkedupFile,

> hunk ./src/Darcs/Patch/Apply.lhs 381
> -%files or directories, changed by a patch
> -%we get it solely from the patch here
> -%instead of performing patch apply on a population
> -%we !could! achieve the same by applying a patch to a cleaned population
> -%and getting modified files and dirs
> -%but this should be significantly slower when the population grows large
> -%This could be useful for just presenting a summary of what a patch does
> -%(especially useful for larger repos)
> -
> -\begin{code}
> -patchChanges :: Prim C(x y) -> [(String,DirMark)]
> -patchChanges (Move f1 f2) = [(fn2fp f1,MovedFile $ fn2fp f2),
> -                             (fn2fp f2,MovedFile $ fn2fp f1)]
> -patchChanges (DP d AddDir) = [(fn2fp d,AddedDir)]
> -patchChanges (DP d RmDir) = [(fn2fp d,RemovedDir)]
> -patchChanges (FP f AddFile) = [(fn2fp f,AddedFile)]
> -patchChanges (FP f RmFile) = [(fn2fp f,RemovedFile)]
> -patchChanges (FP f _) = [(fn2fp f,ModifiedFile)]
> -patchChanges (ChangePref _ _ _) = []
> -\end{code}

I asked on IRC if automatic unused-function detection would be useful
for us...  Turns out that's exactly what Ganesh did to make some of the
refactor code.

abstract MarkedUpFile etc over PatchInfo
----------------------------------------
Not entirely sure where this is going, but I'm imagining that it's for
the on-going work in making conflict marking report the patches
involved.

> hunk ./src/Darcs/Patch/Apply.lhs 305
> -type MarkedUpFile = [(B.ByteString, LineMark)]
> -emptyMarkedupFile :: MarkedUpFile
> +data LineMark pi
> +    = AddedLine pi | RemovedLine pi
> +    | AddedRemovedLine pi pi | None
> +   deriving (Show)
> +type MarkedUpFile pi = [(B.ByteString, LineMark pi)]
> +emptyMarkedupFile :: MarkedUpFile pi
>  emptyMarkedupFile = [(B.empty, None)]

Here's the bulk of our patch...

> hunk ./src/Darcs/Patch/Apply.lhs 313
> -markupFile :: Effect p => PatchInfo -> p C(x y)
> -            -> (FilePath, MarkedUpFile) -> (FilePath, MarkedUpFile)
> +markupFile :: forall p pi C(x y) . Effect p => pi -> p C(x y)
> +            -> (FilePath, MarkedUpFile pi) -> (FilePath, MarkedUpFile pi)
>  markupFile x p = mps (effect p)
> hunk ./src/Darcs/Patch/Apply.lhs 316
> -    where mps :: FL Prim C(a b) -> (FilePath, MarkedUpFile) -> (FilePath, MarkedUpFile)
> +    where mps :: FL Prim C(a b) -> (FilePath, MarkedUpFile pi) -> (FilePath, MarkedUpFile pi)

With either more of this generalisation
  
> -getMarkedupFile :: RepoPatch p => Repository p C(r u t) -> PatchInfo -> FilePath -> IO MarkedUpFile
> +getMarkedupFile :: RepoPatch p => Repository p C(r u t) -> PatchInfo -> FilePath -> IO (MarkedUpFile PatchInfo)

Or just propagating the use.

break up Summary so Prim is handled internally
----------------------------------------------
> +                      Darcs.Patch.Prim.Details
> +                      Darcs.Patch.SummaryData

Seems like better grouping and layering,  Darcs.Patch.Summary
very slightly refactored so that the abstract summary is generated
within Darcs.Patch.Prim.

SummaryData is to make imports work.

> +import Darcs.Patch.Prim.Details ( summarizePrim )
> addfile ./src/Darcs/Patch/Prim/Details.hs
> hunk ./src/Darcs/Patch/Prim/Details.hs 1

This is moved over from src/Darcs/Patch/Prim.Details.

I think we need to slap on some license boilerplate, but I forget the
details...

> +summarizePrim :: Prim C(x y) -> [SummDetail]
> +summarizePrim (FP f (Hunk _ o n)) = [SummFile SummMod f (length o) (length n) 0]
> +summarizePrim (FP f (Binary _ _)) = [SummFile SummMod f 0 0 0]
> +summarizePrim (FP f AddFile) = [SummFile SummAdd f 0 0 0]
> +summarizePrim (FP f RmFile) = [SummFile SummRm f 0 0 0]
> +summarizePrim (FP f (TokReplace _ _ _)) = [SummFile SummMod f 0 0 1]
> +summarizePrim (DP d AddDir) = [SummAddDir d]
> +summarizePrim (DP d RmDir) = [SummRmDir d]
> +summarizePrim (Move f1 f2) = [SummMv f1 f2]
> +summarizePrim (ChangePref _ _ _) = [SummNone]

Renamed from the genSummary helper function 's'

(also maybe an interesting case where you might still want a hunk move patch
because it says what you mean, even if it makes the patch less concise because
you end up having to edit the hunks anyway)

> addfile ./src/Darcs/Patch/SummaryData.hs
> hunk ./src/Darcs/Patch/SummaryData.hs 1

Hunk-moved Darcs.Patch.Summary

move markupFile etc out of Darcs.Patch.Apply
--------------------------------------------
Didn't pay too much attention.  Same basic theme of moving the stuff that
requires knowledge of Prim into Darcs.Patch.Prim, and also creating general
infrastructure that the Darcs.Patch.Prim stuff could import.

> addfile ./src/Darcs/Patch/Markup.hs
> hunk ./src/Darcs/Patch/Markup.hs 1
...
> +markupFile :: forall p pi C(x y) . Effect p => pi -> p C(x y)
> +            -> (FilePath, MarkedUpFile pi) -> (FilePath, MarkedUpFile pi)
> +markupFile x p = mps (effect p)
> +    where mps :: FL Prim C(a b) -> (FilePath, MarkedUpFile pi) -> (FilePath, MarkedUpFile pi)
> +          mps NilFL = id
> +          mps (pp:>:pps) = mps pps . markupPrim x pp

From Darcs.Patch.Apply...

> addfile ./src/Darcs/Patch/MarkupData.hs
> hunk ./src/Darcs/Patch/MarkupData.hs 1
...
> +data LineMark pi
> +    = AddedLine pi | RemovedLine pi
> +    | AddedRemovedLine pi pi | None
> +   deriving (Show)
> +type MarkedUpFile pi = [(B.ByteString, LineMark pi)]
> +emptyMarkedupFile :: MarkedUpFile pi
> +emptyMarkedupFile = [(B.empty, None)]

> hunk ./src/Darcs/Patch/Prim/Details.lhs 12
> +\end{code}
> +
> +%\section{Outputting interesting and useful information}
> +
> +%Just being able to manipulate patches and trees is not enough.  We also
> +%want to be able to view the patches and files.  This requires another set
> +%of functions, closely related to the patch application functions, which
> +%will give us the necessary information to browse the changes we have made.
> +%It is \emph{not} the Patch module's responsibility to add any sort of
> +%markup or formatting, but simply to provide the information necessary for an
> +%external module to do the formatting.

I'm guessing this being commented out (in the original) was due to the
LaTeX being for user documentation and not devel-stuff, and (speculation)
that David must have been intending to uncomment it someday if the library
got to some kind of clean state.  Could be worthwhile to turn this into
module-level haddock and just ditch the literate Haskell stuff.

move Darcs.Population[Data] into Darcs.Patch namespace
------------------------------------------------------
> replace ./src/Darcs/Commands/Annotate.lhs [A-Za-z_0-9\-\.] Darcs.Population Darcs.Patch.Population
> replace ./src/Darcs/Commands/Annotate.lhs [A-Za-z_0-9\-\.] Darcs.PopulationData Darcs.Patch.PopulationData

Not much to see...

>  -- | apply a patchset to a population
> -applyPatchSetPop :: RepoPatch p => PatchSet p C(Origin x) -> Population -> Population
> +applyPatchSetPop :: Effect p => PatchSet p C(Origin x) -> Population -> Population
>  applyPatchSetPop ps pop = applyPatchesPop (newset2FL ps) pop

except maybe this, which is just accounting for Population being a Darcs.Patch level concept

give applyFL a better name
--------------------------
>  applyFL (p:>:ps) = do apply p
> -                      applyFL ps
> +                          applyFL ps

> replace ./src/Darcs/Patch/Apply.lhs [A-Za-z_0-9] applyFL applyPrimFL
> replace ./src/Darcs/Patch/V1/Apply.hs [A-Za-z_0-9] applyFL applyPrimFL

Does seem better
  applyPrimFL :: WriteableDirectory m => FL Prim C(x y) -> m ()

-------------------------------------------------
abstract Population over PatchInfo
move Population handling out of Darcs.Patch.Apply
move Apply Prim code into separate module
move read of Prim into its own module
move showPrim etc out of Darcs.Patch.Prim.Core
-------------------------------------------------

Didn't read these as they just tell the same sort of story as before of
migrating prim-specific stuff into Darcs.Prim.

-------------------------------------------------
move coalescing code out of Darcs.Patch.Prim.Core
extract Darcs.Patch.Prim.Commute
-------------------------------------------------

Same sort of lack of attention, as it's basically breaking up the big
Darcs.Patch.Prim.Core (itself moved out of Darcs.Patch.Prim)

move IsHunk into its own module
-------------------------------
> +                      Darcs.Patch.FileHunk

FileHunk, eh?

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, try +44 (0)1273 64 2905 or
xmpp:kowey@jabber.fr (Jabber or Google Talk only)

On Sun, 2 Jan 2011, Eric Kow wrote:

> All look good.  My only complaint is that we're missing license
> boilerplate on a lot of the new modules we're creating.

Good point.

This is kind of annoying, because it involves figuring out a reasonably 
copyright line even though most of our other copyright lines are way out 
of date. Any idea how other projects approach this problem?

> Here's the meat of this patch.  Note the disappearance of the
> isHunk default implementation which ...
>
>> hunk ./src/Darcs/Patch/Named.lhs 44
>> +instance IsHunk (Named p) where
>> +    isHunk _ = Nothing
>
>> +instance IsHunk (PatchInfoAnd p) where
>> +    isHunk _ = Nothing
>
> Shows up here and here.  Any reason not to have a default implementation
> anymore?  Guess it's safer to avoid accidental fall-through.

I'm on another mission to remove defaults :-) Sometimes they are the right 
thing to do - e.g. MyEq where you only need to define one member - but I 
think in cases like this accidental fall-through is bad. For example if I 
was making a new patch type is it ok to leave it out? I think the answer 
for this particular method is generally no.

More widely, I've found quite a few examples where things are defaulted, 
and then for many instances the methods aren't actually ever used. I think 
I've got some refactorings of Conflict and Apply pending (perhaps not yet 
submitted) where this is the case and by teasing them apart into multiple 
type classes you can then remove some instances.

> I asked on IRC if automatic unused-function detection would be useful
> for us...  Turns out that's exactly what Ganesh did to make some of the
> refactor code.

Unfortuately the tool I wrote to do it requires various hacks so isn't 
very easy to use generally - both the darcs source code need to be changed 
to make it parseable, and haskell-src-exts needs some changes, some of 
which aren't generally correct but work ok in the darcs case. I will try 
to get it cleaned up for hackage at some point though.

> abstract MarkedUpFile etc over PatchInfo
> ----------------------------------------
> Not entirely sure where this is going, but I'm imagining that it's for
> the on-going work in making conflict marking report the patches
> involved.

Actually, not that. I needed to do it to untangle some dependencies (I 
forget the details), but I felt justified in doing so because it makes to 
logical sense; it proves the independence of the concept of marking up a 
file from the actual data you mark it up with.

> (also maybe an interesting case where you might still want a hunk move patch
> because it says what you mean, even if it makes the patch less concise because
> you end up having to edit the hunks anyway)

Yep, I'd have done that if we had hunk move. Ideally a combination of 
indentation, selective token replace and hunk move could have expressed 
the whole thing :-)

> FileHunk, eh?

It's a file patch that's a hunk. Made sense to me, anyway :-)

Ganesh

Ganesh Sittampalam writes:
 > On Sun, 2 Jan 2011, Eric Kow wrote:
 > 
 > > we're missing license boilerplate
 > 
 > Good point.
 > 
 > This is kind of annoying, because it involves figuring out a reasonably 
 > copyright line even though most of our other copyright lines are way out 
 > of date. Any idea how other projects approach this problem?

The license boilerplate is important, because as Darcs becomes more
modularized, people may want to grab pieces of it for other projects.
In the case of "GPL v# or later", at the very least this allows other
projects under GPL to do so with near-zero worry.

I discovered that there doesn't seem to be any easy place to find the
Darcs permission notice without getting source.  This is obnoxious,
and it matters, because the GPL is incompatible with itself.
Reproducing version 2 of the GPL in your manual implies that that
version is OK, but what about v3 and v1?  There are huge differences
among licensing under "GPL" (implying "any version at your option"
according to the GPL itself), under "GPL v#", and under "GPL v# or
later at your option".  At least the man page and the user manual
should reproduce it.

The copyright dates are not so important, at least in the U.S. where
copyright term is measured in centuries.  What's important is that the
copyright holder lists be accurate, because anybody who wants to
change to an incompatible license will need to contact all copyright
holders.

The author list is also important (as a backdoor for relicensing if
there are assignees who refuse to relicense, as the FSF usually does
-- most assignments allow authors broad rights to use their own code,
and the FSF's official interpretation of the FSF standard assignment
is that authors receive the right to sublicense their own code,
although I personally think that interpretation of the wording is
shaky).

For newly contributed code, you just add the author and date.  For
refactored code, you copy the copyright line from all source files.
It is better to have too many copyright holders than to be missing a
few, since it's well-known that in many cases listed copyright holders
own only a portion of the code, and in the limit that can be the null
set without doing too much harm.

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@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@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@jabber.fr (Jabber or Google Talk only)

On Thu, 6 Jan 2011, Eric Kow wrote:

> On Tue, Nov 23, 2010 at 23:18:37 +0000, Ganesh Sittampalam wrote:

>> Ganesh Sittampalam <ganesh@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

Sorry, I can't remember either. It's not the end-state. It was probably 
for dependency reasons; I was trying to make lots of small changes 
rather than one big change when moving things around, so that meant that 
sometimes some modules ended up in odd positions in the hierarchy 
temporarily.

> 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.

Yes, we have a choice between having all the Show code together and 
all the code for specific patch types together. I'm aiming for the 
latter, which I claim is clearly superior given that we'll be looking to 
add new patch types in future. I also have a vague hope at the back of my 
mind of breaking up Prim.V1 (as it becomes later in my patches) further so 
that things like FilePatchType themselves implement some of the Patch 
interfaces, further improving modularity in the sense that patches are 
defined in layers - Patch/RealPatch as a layer handling conflicts over 
Prim which might become a layer that brings together files and directories 
over more low-level patch types. [To some extent FileHunk is an example of 
the latter, but it's more of a pragmatic thing to deal with the fact that 
lots of higher-level code wants direct access to it, rather than a 
principled design.]

> simplify Effect instances
> -------------------------
>> Ganesh Sittampalam <ganesh@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.

I guess the view that it's simpler is somewhat dependent on my desire to 
remove the Effect Prim instance (which I do in a later patch), along with 
similar trivial instances like FromPrim, ToFromPrim and PrimPatchBase.

Thinking about that a bit further, I guess it's not clear-cut that said 
removal is definitely the right thing to do. You lose a bit in terms of 
not being able to overload on Patch/Prim in a few cases, but I think 
you gain in it becoming clearer what kind of patch is where - in 
situations where you want to you need to turn something to or from a Prim, 
you really ought to know whether you actually have a Prim already or not.

Ganesh

Pushing as part of a simultaneous patch481-patch484
push-and-compile-and-test (previous review + being refactoring means
it's ok to push),

EXCEPT following patch:
Tue Nov 23 07:42:41 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * use API function for identity patch

The above patch needs for patch487 to be applied (in turn depends on
patch485 or patch486) (otherwise darcs-test does not compîle).

This patch bundle (with 24 patches) was just applied to the repository http://darcs.net/.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-43cf36582bde96b297c5dc87c33f3e59de27d829

Pushing last patch "* use API function for identity patch" since
aforementioned bundles got pushed (after being checked for compilation
and tests with this patch).

This patch bundle (with 24 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-43cf36582bde96b297c5dc87c33f3e59de27d829