Getting rid of the dreaded Split patch type (well, you might dread
it if you've dug around in the patch code as much as I have.)

I suspect there are few/no repositories in the wild with this,
but better safe than sorry. I've written tests for the things
that I think used to work.

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

Mon Nov 22 07:30:43 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * add tests for reading of split patches

Mon Nov 22 06:43:04 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * get rid of Split

Tue Nov 23 07:01:42 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * read legacy Split patch format
  This now gets flattened out at the FL Patch/FL RealPatch level.

Attachments

• add-tests-for-reading-of-split-patches.dpatch
• unnamed

This patch bundle (with 3 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-be26b9d8de4a934a5748515f79d9aa3066329d0d

Hi Reinier, do you have any patch reviewing time?  I'm slowly trying to 
hand out patches so we can wear this backlog away.  It's a bit random at 
the moment, I'm afraid.

Hi,

I have some questions, but overall it looks fine. Disclaimer: I don't
understand where it is important that reading/showing should round-trip and
where it isn't.

Let's start the reviewing by looking up what split patches are. Somewhere in the hunks, I see:

> -\paragraph{Split patch [OBSOLETE!]}
> -A split patch is similar to a composite patch but rather than being
> -composed of several patches grouped together, it is created from one
> -patch that has been split apart, typically through a merge or
> -commutation.
> -\begin{verbatim}
> -(
> -  <put patches here> (indented two)
> -)
> -\end{verbatim}

So a split patch is a kind of primitive patch that is not primitive at all. It contains a list (FL) of patches that together form the split patch. In the code, split patches are made using the Split constructor of the Prim type.

The comment says split patches are obsolete, and the whole concept looks inconsistent with what Prims are supposed to be. So removing them seems a good idea.

> [add tests for reading of split patches
> Ganesh Sittampalam <ganesh@earth.li>**20101122073043
>  Ignore-this: bc2cebc9257f2982d15e590351f2d97f
> ] addfile ./tests/data/split--darcs-1.dpatch
> hunk ./tests/data/split--darcs-1.dpatch 1
> +1 patch for repository /home/ganesh/temp/empty-old:
> +
> +Tue Nov 16 07:15:41 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
> +  * add files
> +
> +New patches:
> +
> +[add files
> +Ganesh Sittampalam <ganesh@earth.li>**20101116071541
> + Ignore-this: 2a86fa7deae02f1c98d578077ee5f8a9
> +] {
> +addfile ./file1
> +(
> +addfile ./file2
> +addfile ./file3
> +)
> +addfile ./file4
> +}
> +
> +Context:
> +
> +Patch bundle hash:
> +947100f54fce0fb3cb5debeca8702b119ee17d8a

Here we add a patch file that contains a split patch.

> addfile ./tests/data/split--darcs-2.tgz
> addfile ./tests/data/split--hashed.tgz
> addfile ./tests/data/split--old-fashioned-inventory.tgz

Here we add tarballs that I presume contain repositories with the above patch bundle applied. Was the darcs-2 tarball created by applying the bundle or by converting the darcs-1 repo created by applying the bundle?

> addfile ./tests/data/split2--darcs-1.dpatch
> hunk ./tests/data/split2--darcs-1.dpatch 1
> +2 patches for repository /home/ganesh/temp/empty-old:
> +
> +Tue Nov 16 18:32:25 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
> +  * wibble
> +
> +Tue Nov 16 18:32:38 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
> +  * 'nums'
> +
> +
> +New patches:
> +
> +[wibble
> +Ganesh Sittampalam <ganesh@earth.li>**20101116183225
> + Ignore-this: 5e5df9907f191511650c2ed66754d17
> +] {
> +addfile ./wibble
> +hunk ./wibble 1
> ++A
> ++B
> ++C
> ++D
> +}
> +['nums'
> +Ganesh Sittampalam <ganesh@earth.li>**20101116183238
> + Ignore-this: e865de7aa7e896c759dabccf510c59bf
> +] {
> +hunk ./wibble 1
> ++1
> + A
> + B
> + C
> +(
> +hunk ./wibble 3
> ++2
> +hunk ./wibble 5
> ++3
> +)
> +hunk ./wibble 7
> + B
> + 3
> + C
> ++4
> + D
> +hunk ./wibble 9
> ++5
> +}
> +
> +Context:
> +
> +Patch bundle hash:
> +cdfc4f7087525b0298d4f08e6e497f925f85e406

Add another patch bundle containing a split patch. Now there are two patches, the second of which contains the split patch. I wonder what functionality this bundle requires that the previous one doesn't. 

> addfile ./tests/split-patches.sh
> hunk ./tests/split-patches.sh 1
> +#!/usr/bin/env bash
> hunk ./tests/split-patches.sh 3
> +## Test for correct handling of Darcs v1 patches with nested { }
> +##
> +## Copyright (C) 2010 Ganesh Sittampalam
> +##
> +## Permission is hereby granted, free of charge, to any person
> +## obtaining a copy of this software and associated documentation
> +## files (the "Software"), to deal in the Software without
> +## restriction, including without limitation the rights to use, copy,
> +## modify, merge, publish, distribute, sublicense, and/or sell copies
> +## of the Software, and to permit persons to whom the Software is
> +## furnished to do so, subject to the following conditions:
> +##
> +## The above copyright notice and this permission notice shall be
> +## included in all copies or substantial portions of the Software.
> +##
> +## THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> +## EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> +## MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> +## NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> +## BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> +## ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> +## CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> +## SOFTWARE.
> +
> +. lib                  # Load some portability helpers.
> +
> +if grep old-fashioned .darcs/defaults; then
> +format=old-fashioned-inventory
> +patchtype=darcs-1
> +elif grep darcs-2 .darcs/defaults; then
> +format=darcs-2
> +patchtype=darcs-2
> +elif grep hashed .darcs/defaults; then
> +format=hashed
> +patchtype=darcs-1
> +else exit 200; fi
> +
> +rm -rf split--${format}
> +gunzip -c $TESTDATA/split--${format}.tgz | tar xf -
> +cd split--${format}
> +darcs check
> +cd ..

Here we check if repositories containing split patches can still be checked.

> +# .dpatch tests: .dpatch files for darcs 2 split patches were
> +# broken in darcs 2.5 (and probably always broken) so we don't
> +# bother to test them.
> +
> +if [ ${patchtype} != darcs-1 ] ; then exit 0 ; fi
> +
> +rm -rf temp
> +mkdir temp
> +cd temp
> +darcs init
> +darcs apply $TESTDATA/split--${patchtype}.dpatch
> +cd ..
> +
> +rm -rf temp2
> +mkdir temp2
> +cd temp2
> +darcs init
> +darcs apply $TESTDATA/split2--${patchtype}.dpatch
> +cd ..

And here we check if bundles containing split patches can be applied. Shouldn't we also check the internal representation of the patch in the resulting repo? We do want to make sure that darcs does something meaningful with the split patch. It is not enough if it just accepts the syntax. OTOH, I realize it may be hard to test this.

> [get rid of Split
> Ganesh Sittampalam <ganesh@earth.li>**20101122064304
>  Ignore-this: 5473000393dab8828eb56f182af20ab5
> ] hunk ./src/Darcs/Patch/Apply.lhs 151
>  applyToFilepaths pa fs = withFilePaths fs (apply pa)
>  
>  instance Apply Prim where
> -    apply (Split ps) = applyFL ps
>      apply Identity = return ()
>      apply (FP f RmFile) = mRemoveFile f
>      apply (FP f AddFile) = mCreateFile f
> hunk ./src/Darcs/Patch/Apply.lhs 322
>  
>  markupPrim :: PatchInfo -> Prim C(x y)
>              -> (FilePath, MarkedUpFile) -> (FilePath, MarkedUpFile)
> -markupPrim _ (Split NilFL) (f, mk) = (f, mk)
> -markupPrim n (Split (p:>:ps)) (f, mk) = markupPrim n (Split ps) $
> -                                       markupPrim n p (f, mk)
>  markupPrim _ (FP _ AddFile) (f, mk) = (f, mk)
>  markupPrim _ (FP _ RmFile) (f, mk) = (f, mk)
>  markupPrim n (FP f' (Hunk line old new)) (f, mk)
> hunk ./src/Darcs/Patch/Apply.lhs 401
>  patchChanges (FP f AddFile) = [(fn2fp f,AddedFile)]
>  patchChanges (FP f RmFile) = [(fn2fp f,RemovedFile)]
>  patchChanges (FP f _) = [(fn2fp f,ModifiedFile)]
> -patchChanges (Split ps) = concat $ mapFL patchChanges ps
>  patchChanges (ChangePref _ _ _) = []
>  patchChanges Identity = []
>  \end{code}
> hunk ./src/Darcs/Patch/Apply.lhs 418
>   = Pop pi (applyToPopTree patch tree)
>     -- ``pi'' is global below!
>   where applyToPopTree :: Prim C(x y) -> PopTree -> PopTree
> -       applyToPopTree (Split ps) tr =
> -        foldlFL (\t p -> applyToPopTree p t) tr ps
>         applyToPopTree p@(FP f AddFile) tr =
>             let xxx = BC.split '/' (fn2ps  f) in
>                 popChange xxx p $ fst $ breakP xxx tr

So here we remove the references to the Prim constructor in Apply.lhs.

> hunk ./src/Darcs/Patch/Prim/Core.lhs 81
>      Move :: !FileName -> !FileName -> Prim C(x y)
>      DP :: !FileName -> !(DirPatchType C(x y)) -> Prim C(x y)
>      FP :: !FileName -> !(FilePatchType C(x y)) -> Prim C(x y)
> -    Split :: FL Prim C(x y) -> Prim C(x y)
>      Identity :: Prim C(x x)
>      ChangePref :: !String -> !String -> !String -> Prim C(x y)

And here we go: remove the Split constructor of Prim.

> hunk ./src/Darcs/Patch/Prim/Core.lhs 173
>      invert (DP d AddDir) = DP d RmDir
>      invert (Move f f') = Move f' f
>      invert (ChangePref p f t) = ChangePref p t f
> -    invert (Split ps) = Split $ invert ps
>      identity = Identity
>      sloppyIdentity Identity = IsEq
>      sloppyIdentity _ = NotEq
> hunk ./src/Darcs/Patch/Prim/Core.lhs 190
>      showsPrec d (FP fn fp) = showParen (d > appPrec) $ showString "FP " .
>                               showsPrec (appPrec + 1) fn . showString " " .
>                               showsPrec (appPrec + 1) fp
> -    showsPrec d (Split l) = showParen (d > appPrec) $ showString "Split " .
> -                            showsPrec (appPrec + 1) l
>      showsPrec _ Identity = showString "Identity"
>      showsPrec d (ChangePref p f t) = showParen (d > appPrec) $ showString "ChangePref " .
>                                       showsPrec (appPrec + 1) p . showString " " .
> hunk ./src/Darcs/Patch/Prim/Core.lhs 249
>  showPrim x (DP d RmDir)  = showRmDir x d
>  showPrim x (Move f f') = showMove x f f'
>  showPrim _ (ChangePref p f t) = showChangePref p f t
> -showPrim x (Split ps)  = showSplit x ps
>  showPrim _ Identity = blueText "{}"
>  
>  showPrimFL :: FileNameFormat -> FL Prim C(a b) -> Doc
> hunk ./src/Darcs/Patch/Prim/Core.lhs 381
>                   | otherwise = B.take n ps : breakEvery n (B.drop n ps)
>  \end{code}
>  
> -\paragraph{Split patch [OBSOLETE!]}
> -A split patch is similar to a composite patch but rather than being
> -composed of several patches grouped together, it is created from one
> -patch that has been split apart, typically through a merge or
> -commutation.
> -\begin{verbatim}
> -(
> -  <put patches here> (indented two)
> -)
> -\end{verbatim}
>  \begin{code}
> hunk ./src/Darcs/Patch/Prim/Core.lhs 382
> -showSplit :: FileNameFormat -> FL Prim C(x y) -> Doc
> -showSplit x ps = blueText "("
> -            $$ vcat (mapFL (showPrim x) ps)
> -            $$ blueText ")"
> -
> -Split :: CommuteFunction
> -commuteSplit (Split patches :< patch) =
> -    toPerhaps $ cs (patches :< patch) >>= sc
> -    where cs :: ((FL Prim) :< Prim) C(x y) -> Maybe ((Prim :< (FL Prim)) C(x y))
> -          cs (NilFL :< p1) = return (p1 :< NilFL)
> -          cs (p:>:ps :< p1) = do p' :> p1' <- commute (p1 :> p)
> -                                 p1'' :< ps' <- cs (ps :< p1')
> -                                 return (p1'' :< p':>:ps')
> -          sc :: (Prim :< (FL Prim)) C(x y) -> Maybe ((Prim :< Prim) C(x y))
> -          sc (p1 :< ps) = scFL $ p1 :< (sortCoalesceFL ps)
> -            where scFL :: (Prim :< (FL Prim)) C(x y)
> -                       -> Maybe ((Prim :< Prim) C(x y))
> -                  scFL (p1' :< (p :>: NilFL)) = return (p1' :< p)
> -                  scFL (p1' :< ps') = return (p1' :< Split ps')
> -commuteSplit _ = Unknown
> -
>  tryToShrink :: FL Prim C(x y) -> FL Prim C(x y)
>  tryToShrink = mapPrimFL tryHarderToShrink
>  
> hunk ./src/Darcs/Patch/Prim/Core.lhs 415
>  toSimple (DP a AddDir) = Just (a, SDP AddDir)
>  toSimple (DP _ RmDir) = Nothing -- ordering is trickier with rmdir present
>  toSimple (Move _ _) = Nothing
> -toSimple (Split _) = Nothing
>  toSimple Identity = Nothing
>  toSimple (ChangePref a b c) = Just (fp2fn "_darcs/prefs/prefs", SCP a b c)
>  
> hunk ./src/Darcs/Patch/Prim/Core.lhs 594
>      eec (p2 :<ChangePref p f t) = Succeeded (ChangePref p f t :< unsafeCoerceP p2)
>      eec (Identity :< p1) = Succeeded (p1 :< Identity)
>      eec (p2 :< Identity) = Succeeded (Identity :< p2)
> -    eec xx =
> -        msum [
> -              cleverCommute commuteFiledir                xx
> -             ,cleverCommute commuteSplit                  xx
> -             ]
> +    eec xx = cleverCommute commuteFiledir                xx
>  
>  {-
>  Note that it must be true that
> hunk ./src/Darcs/Patch/Prim/Core.lhs 615
>  instance PatchInspect Prim where
>      -- Recurse on everything, these are potentially spoofed patches
>      listTouchedFiles (Move f1 f2) = map fn2fp [f1, f2]
> -    listTouchedFiles (Split ps) = nubsort $ concat $ mapFL listTouchedFiles ps
>      listTouchedFiles (FP f _) = [fn2fp f]
>      listTouchedFiles (DP d _) = [fn2fp d]
>      listTouchedFiles (ChangePref _ _ _) = []
> hunk ./src/Darcs/Patch/Prim/Core.lhs 623
>      hunkMatches f (FP _ (Hunk _ remove add)) = anyMatches remove || anyMatches add
>          where anyMatches = foldr ((||) . f) False
>      hunkMatches _ (FP _ _) = False
> -    hunkMatches f (Split ps) = or $ mapFL (hunkMatches f) ps
>      hunkMatches _ (DP _ _) = False
>      hunkMatches _ (ChangePref _ _ _) = False
>      hunkMatches _ Identity = False
> hunk ./src/Darcs/Patch/Prim/Core.lhs 690
>  old and new version of a file.
>  \begin{code}
>  canonize :: Prim C(x y) -> FL Prim C(x y)
> -canonize (Split ps) = sortCoalesceFL ps
>  canonize p | IsEq <- isIdentity p = NilFL
>  canonize (FP f (Hunk line old new)) = unseal unsafeCoercePEnd $ unFreeLeft $ canonizeHunk f line old new
>  canonize p = p :>: NilFL
> hunk ./src/Darcs/Patch/Prim/Core.lhs 717
>  coalesce (FP f1 p1 :< FP _ p2) = coalesceFilePrim f1 (p1 :< p2) -- f1 = f2
>  coalesce (Identity :< p) = Just p
>  coalesce (p :< Identity) = Just p
> -coalesce (Split NilFL :< p) = Just p
> -coalesce (p :< Split NilFL) = Just p
>  coalesce (Move a b :< Move b' a') | a == a' = Just $ Move b' b
>  coalesce (Move a b :< FP f AddFile) | f == a = Just $ FP b AddFile
>  coalesce (Move a b :< DP f AddDir) | f == a = Just $ DP b AddDir

So with all the above hunks, a lot of operations defined on Prims are stripped
of their Prim case.

> hunk ./src/Darcs/Patch/Prim/Core.lhs 901
>          = d1 == d2 && p1 `unsafeCompare` p2
>      unsafeCompare (FP f1 fp1) (FP f2 fp2)
>          = f1 == f2 && fp1 `unsafeCompare` fp2
> -    unsafeCompare (Split ps1) (Split ps2)
> -        = eqFL unsafeCompare ps1 ps2
>      unsafeCompare (ChangePref a1 b1 c1) (ChangePref a2 b2 c2)
>          = c1 == c2 && b1 == b2 && a1 == a2
>      unsafeCompare Identity Identity = True
> hunk ./src/Darcs/Patch/Prim/Core.lhs 909
>  instance Eq (Prim C(x y)) where
>      (==) = unsafeCompare
>  
> -mergeOrders :: Ordering -> Ordering -> Ordering
> -mergeOrders EQ x = x
> -mergeOrders LT _ = LT
> -mergeOrders GT _ = GT
> -
>  -- | 'comparePrim' @p1 p2@ is used to provide an arbitrary ordering between
>  --   @p1@ and @p2@.  Basically, identical patches are equal and
> hunk ./src/Darcs/Patch/Prim/Core.lhs 911
> ---   @Move < DP < FP < Split < Identity < ChangePref@.
> +--   @Move < DP < FP < Identity < ChangePref@.
>  --   Everything else is compared in dictionary order of its arguments.
>  comparePrim :: Prim C(x y) -> Prim C(w z) -> Ordering
>  comparePrim (Move a b) (Move c d) = compare (a, b) (c, d)
> hunk ./src/Darcs/Patch/Prim/Core.lhs 923
>  comparePrim (FP f1 fp1) (FP f2 fp2) = compare (f1, fp1) $ unsafeCoerceP (f2, fp2)
>  comparePrim (FP _ _) _ = LT
>  comparePrim _ (FP _ _) = GT
> -comparePrim (Split ps1) (Split ps2) = compareFL comparePrim ps1 $ unsafeCoerceP ps2
> -comparePrim (Split _) _ = LT
> -comparePrim _ (Split _) = GT
>  comparePrim Identity Identity = EQ
>  comparePrim Identity _ = LT
>  comparePrim _ Identity = GT
> hunk ./src/Darcs/Patch/Prim/Core.lhs 929
>  comparePrim (ChangePref a1 b1 c1) (ChangePref a2 b2 c2)
>   = compare (c1, b1, a1) (c2, b2, a2)
>  
> -eqFL :: (FORALL(b c d e) a C(b c) -> a C(d e) -> Bool)
> -      -> FL a C(x y) -> FL a C(w z) -> Bool
> -eqFL _ NilFL NilFL = True
> -eqFL f (x:>:xs) (y:>:ys) = f x y && eqFL f xs ys
> -eqFL _ _ _ = False
> -
> -compareFL :: (FORALL(b c d e) a C(b c) -> a C(d e) -> Ordering)
> -           -> FL a C(x y) -> FL a C(w z) -> Ordering
> -compareFL _ NilFL NilFL = EQ
> -compareFL _ NilFL _     = LT
> -compareFL _ _     NilFL = GT
> -compareFL f (x:>:xs) (y:>:ys) = f x y `mergeOrders` compareFL f xs ys
> -
> -

And thus we get rid of the code to compare two Split patches. Why did we need
that unsafeCoerceP in the first removed line of comparePrim? compareFL doesn't
require its arguments to have the same witnesses, does it?

> hunk ./src/Darcs/Patch/Read.hs 131
>       , return' $ readTok x
>       , return' $ readBinary x
>       , return'   readIdentity
> -     , readSplit x
>       , return' $ readChangePref
>       ]
>    where

This is in a function readPrim, in a list of alternative parsers for a Prim.
For some reason the old code did the sealing inside readSplit for Prim, and
in this function for the other primitive patch types.

> hunk ./src/Darcs/Patch/Read.hs 168
>  changepref :: B.ByteString
>  changepref = BC.pack "changepref"
>  
> -readPatches :: ParserM m => FileNameFormat -> Char
> -            -> m (Sealed (FL Prim C(x )))
> -readPatches x c =
> -  do mp <- (Just <$> readPrim x) <|> return Nothing
> -     case mp of
> -       Nothing -> do lexChar c
> -                     return $ seal NilFL
> -       Just (Sealed p) -> do Sealed ps <- readPatches x c
> -                             return $ seal (p:>:ps)
> -
> -readSplit :: ParserM m => FileNameFormat -> m (Sealed (Prim C(x )))
> -readSplit x = do
> -  char '('
> -  ps <- readPatches x ')'
> -  return $ Split `mapSeal` ps
> -
>  readFileName :: FileNameFormat -> B.ByteString -> FileName
>  readFileName OldFormat = ps2fn
>  readFileName NewFormat = fp2fn . decodeWhite . BC.unpack

Give the boot to some more split patch parsing code. We have to make sure that
patch files with this syntax can still be parsed by code somewhere else
though.

> hunk ./src/Darcs/Patch/Summary.hs 10
>                            Effect, IsConflictedPrim(IsC), ConflictState(..),
>                            DirPatchType(..), FilePatchType(..) )
>  
> -import Darcs.Witnesses.Ordered ( mapFL )
> -
>  import Printer ( Doc, empty, vcat,
>                   text,
>                   minus, plus, ($$), (<+>), (<>),
> hunk ./src/Darcs/Patch/Summary.hs 67
>            s (FP f (TokReplace _ _ _)) = [SummFile SummMod f 0 0 1]
>            s (DP d AddDir) = [SummAddDir d]
>            s (DP d RmDir) = [SummRmDir d]
> -          s (Split xs) = concat $ mapFL s xs
>            s (Move f1 f2) = [SummMv f1 f2]
>            s (ChangePref _ _ _) = [SummNone]
>            s Identity = [SummNone]
> hunk ./src/Darcs/Patch/Viewing.hs 38
>  import Darcs.Patch.FileName ( fn2fp )
>  import Printer ( Doc, empty, vcat,
>                   text, blueText, Color(Cyan,Magenta), lineColor,
> -                 ($$), (<+>), (<>),
> +                 ($$), (<+>),
>                   prefix,
>                   userchunkPS,
>                 )
> hunk ./src/Darcs/Patch/Viewing.hs 59
>  
>  instance ShowPatch Prim where
>      showContextPatch p@(FP _ (Hunk _ _ _)) = showContextHunk p
> -    showContextPatch (Split ps) =
> -        do x <- showContextSeries ps
> -           return $ blueText "(" $$ x <> blueText ")"
>      showContextPatch p = return $ showPatch p
>      summary = plainSummaryPrim
>      thing _ = "change"

Remove code to display split patches.

> hunk ./src/Darcs/Test/Patch/Test.hs 369
>  
>  instance Check Prim where
>     checkPatch Identity = isValid
> -   checkPatch (Split ps) = checkPatch ps
>  
>     checkPatch (FP f RmFile) = removeFile $ fn2fp f
>     checkPatch (FP f AddFile) =  createFile $ fn2fp f

Remove code to check split patches in the unit tests.

> [read legacy Split patch format
> Ganesh Sittampalam <ganesh@earth.li>**20101123070142
>  Ignore-this: 3fd0ccf9e668d133bbea222364823b35
>  This now gets flattened out at the FL Patch/FL RealPatch level.
> ] move ./src/Darcs/Patch/Braced ./src/Darcs/Patch/Bracketed
> move ./src/Darcs/Patch/Braced.hs ./src/Darcs/Patch/Bracketed.hs
> replace ./darcs.cabal [A-Za-z_0-9] Braced Bracketed
> hunk ./src/Darcs/Patch/Bracketed.hs 1
> -module Darcs.Patch.Braced
> -    ( Braced(..), mapBraced, unBraced
> +module Darcs.Patch.Bracketed
> +    ( Bracketed(..), mapBraced, unBraced
>      , BracedFL, mapBracedFL_FL, unBracedFL
>      ) where
>  
> hunk ./src/Darcs/Patch/Bracketed.hs 11
>  import Darcs.Patch.Format ( PatchListFormat )
>  import Darcs.Witnesses.Ordered ( FL(..), mapFL_FL, concatFL )
>  
> --- |This type exists for legacy support of the V1 patch on-disk format only.
> --- It is a wrapper type that explicitly tracks the nesting of braces
> +-- |This type exists for legacy support of on-disk format patch formats.
> +-- It is a wrapper type that explicitly tracks the nesting of braces and parens
>  -- in the on-disk representation of such patches. It is used as an intermediate
>  -- form when reading such patches normally, and also for round-tripping such
> hunk ./src/Darcs/Patch/Bracketed.hs 15
> --- patches when checking the hash in bundles. It shouldn't be used for anything
> --- else.
> -data Braced p C(x y) where
> -  Singleton :: p C(x y) -> Braced p C(x y)         -- A single patch, not wrapped in braces
> -  Braced :: BracedFL p C(x y) -> Braced p C(x y)   -- A list of patches, wrapped in braces
> +-- patches when checking the hash in bundles.
> +-- It shouldn't be used for anything else.
> +data Bracketed p C(x y) where
> +  Singleton :: p C(x y) -> Bracketed p C(x y)            -- A single patch, not wrapped in anything
> +  Braced :: BracedFL p C(x y) -> Bracketed p C(x y)   -- A list of patches, wrapped in {}
> +  Parens :: BracedFL p C(x y) -> Bracketed p C(x y)   -- A list of patches, wrapped in ()
>

Now we're at the point where we're making sure that we can still handle the
syntax for split patches in patch files. Apparently, there was a way to
handle legacy patch files with patch lists delimited by braces ('{' and '}').
Here we extend that code to handle patch lists delimited by parentheses as
well, which is the split patch format.

It's inconsistent to have the one constructor named "Parens" and the other
"Braced" (as opposed to "Parens" and "Braces"), but I can see "Braced" is
there for historical reasons and "Parenthesized" is a bit long.

> hunk ./src/Darcs/Patch/Bracketed.hs 22
> -type BracedFL p C(x y) = FL (Braced p) C(x y)
> +type BracedFL p C(x y) = FL (Bracketed p) C(x y)

Adapt this type definition to the new name of the Bracketed type. The name "BracedFL"
will be changed to "BracketedFL" later on.

> hunk ./src/Darcs/Patch/Bracketed.hs 24
> -unBraced :: Braced p C(x y) -> FL p C(x y)
> +unBraced :: Bracketed p C(x y) -> FL p C(x y)
>  unBraced (Singleton p) = p :>: NilFL
>  unBraced (Braced ps) = unBracedFL ps
> hunk ./src/Darcs/Patch/Bracketed.hs 27
> +unBraced (Parens ps) = unBracedFL ps
>  
>  unBracedFL :: BracedFL p C(x y) -> FL p C(x y)
>  unBracedFL = concatFL . mapFL_FL unBraced

Adjust type of unBraced to the new name "Bracketed", and add a case for the
new Parens constructor. I suppose this, too, will be renamed to unBracketed.

> hunk ./src/Darcs/Patch/Bracketed.hs 32
>  
> -mapBraced :: (FORALL(a b) p C(a b) -> q C(a b)) -> Braced p C(x y) -> Braced q C(x y)
> +mapBraced :: (FORALL(a b) p C(a b) -> q C(a b)) -> Bracketed p C(x y) -> Bracketed q C(x y)
>  mapBraced f (Singleton p) = Singleton (f p)
>  mapBraced f (Braced ps) = Braced (mapBracedFL_FL f ps)
> hunk ./src/Darcs/Patch/Bracketed.hs 35
> +mapBraced f (Parens ps) = Parens (mapBracedFL_FL f ps)
>  
>  mapBracedFL_FL :: (FORALL(a b) p C(a b) -> q C(a b)) -> BracedFL p C(x y) -> BracedFL q C(x y)
>  mapBracedFL_FL f ps = mapFL_FL (mapBraced f) ps

Make similar adjustments to mapBraced...

> hunk ./src/Darcs/Patch/Bracketed.hs 40
>  
> -instance PatchListFormat (Braced p)
> +instance PatchListFormat (Bracketed p)
> replace ./src/Darcs/Patch/Bracketed.hs [A-Za-z_0-9] BracedFL BracketedFL
> replace ./src/Darcs/Patch/Bracketed.hs [A-Za-z_0-9] mapBraced mapBracketed
> replace ./src/Darcs/Patch/Bracketed.hs [A-Za-z_0-9] mapBracedFL_FL mapBracketedFL_FL
> replace ./src/Darcs/Patch/Bracketed.hs [A-Za-z_0-9] unBraced unBracketed
> replace ./src/Darcs/Patch/Bracketed.hs [A-Za-z_0-9] unBracedFL unBracketedFL

And these are the rest of the changes from "Braced" to "Bracketed".

> hunk ./src/Darcs/Patch/Bracketed/Instances.hs 2
>  {-# OPTIONS_GHC -fno-warn-orphans #-}
> -module Darcs.Patch.Braced.Instances () where
> +module Darcs.Patch.Bracketed.Instances () where
>  
> hunk ./src/Darcs/Patch/Bracketed/Instances.hs 4
> -import Darcs.Patch.Braced ( Braced(..) )
> +import Darcs.Patch.Bracketed ( Bracketed(..) )
>  import Darcs.Patch.Show ( ShowPatchBasic(..) )
>  
>  import Darcs.Witnesses.Ordered ( FL(NilFL), mapFL )
> hunk ./src/Darcs/Patch/Bracketed/Instances.hs 13
>  
>  #include "gadts.h"
>  
> -instance ShowPatchBasic p => ShowPatchBasic (Braced p) where
> +instance ShowPatchBasic p => ShowPatchBasic (Bracketed p) where
>      showPatch (Singleton p) = showPatch p
>      showPatch (Braced NilFL) = blueText "{" $$ blueText "}"
>      showPatch (Braced ps) = blueText "{" $$ vcat (mapFL showPatch ps) $$ blueText "}"
> hunk ./src/Darcs/Patch/Bracketed/Instances.hs 17
> +    showPatch (Parens ps) = blueText "(" $$ vcat (mapFL showPatch ps) $$ blueText ")"
>  
>  -- the ReadPatch instance is defined in Darcs.Patch.Read as it is
>  -- used as an intermediate form during reading of lists of patches

So here we specify how patches of the Bracketed type should be shown.

What is that special case for (Braced NilFL) good for? Nothing apparently if
we don't do such a thing for Parens. Or does it have something to do with the
way vcat renders an empty list?

> hunk ./src/Darcs/Patch/Bracketed/Instances.hs 21
> --- that are specified as ListFormatV1.
> +-- that are specified as ListFormatV1 or ListFormatV2.
> replace ./src/Darcs/Patch/Bracketed/Instances.hs [A-Za-z_0-9] unBracedFL unBracketedFL
> replace ./src/Darcs/Patch/Bundle.hs [A-Za-z_0-9] Braced Bracketed
> replace ./src/Darcs/Patch/Bundle.hs [A-Za-z_0-9] unBracedFL unBracketedFL

And do some more s/Braced/Bracketed/, now in Instances.hs

> hunk ./src/Darcs/Patch/Format.hs 21
>  
>  -- | This type is used to tweak the way that lists of 'p' are shown for a
>  -- given 'Patch' type 'p'. It is needed to maintain backwards compatibility
> --- for V1 patches.
> +-- for V1 and V2 patches.
>  data ListFormat (p :: PATCHKIND)
>    = ListFormatDefault -- ^Show and read lists without braces.
>    | ListFormatV1      -- ^Show lists with a single layer of braces around the outside,
> hunk ./src/Darcs/Patch/Format.hs 26
>                        -- except for singletons which have no braces.
> -                      -- Read with arbitrary nested braces and flatten them out.
> +                      -- 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.

Apparently there is a ListFormat type that specifies the kind of lists that can
occur in a patch serialization. I find it confusing that the coment for the
type says that is is (implicitly only) for *showing* patches, but the comments
for the constructors also refer to *reading* patches. In practice it's used
for both showing and reading.

A list format is associated with a certain patch type using a type class
called PatchListFormat.

We add a ListFormatV2, which means a format in which parenthesized lists can
occur but braced lists can't. Fine, let's see how that's used.

> hunk ./src/Darcs/Patch/Read.hs 33
>  import ByteStringUtils ( fromHex2PS, dropSpace )
>  import qualified Data.ByteString       as B  (ByteString, init, tail, concat, null)
>  
> -import Darcs.Patch.Braced ( Braced(..), unBracedFL )
> +import Darcs.Patch.Bracketed ( Bracketed(..), unBracedFL )
>  import Darcs.Patch.FileName ( FileName, fn2fp, fp2fn, ps2fn, decodeWhite )
>  import Darcs.Patch.Format ( PatchListFormat(..), ListFormat(..) )
>  import Darcs.Patch.Prim ( Prim(..), FileNameFormat(..),
> hunk ./src/Darcs/Patch/Read.hs 67
>           Just (p, ps') | B.null (dropSpace ps') -> Just p
>           _ -> Nothing
>  
> -instance ReadPatch p => ReadPatch (Braced p) where
> +instance ReadPatch p => ReadPatch (Bracketed p) where
>      readPatch' = mapSeal Braced <$> bracketedFL readPatch' '{' '}'
>                     <|>

Some renaming again...

> hunk ./src/Darcs/Patch/Read.hs 70
> +                 mapSeal Parens <$> bracketedFL readPatch' '(' ')'
> +                   <|>
>                   mapSeal Singleton <$> readPatch'

And add the case for reading a parenthesized list to the ReadPatch instance
for Bracketed.

>  instance (ReadPatch p, PatchListFormat p) => ReadPatch (FL p) where
> hunk ./src/Darcs/Patch/Read.hs 78
>      readPatch'
>          | ListFormatV1 <- patchListFormat :: ListFormat p
>              = mapSeal unBracedFL <$> readPatch'
> +        -- in the V2 format case, we only need to support () on reading, not {}
> +        -- for simplicity we just go through the same code path.
> +        | ListFormatV2 <- patchListFormat :: ListFormat p
> +            = mapSeal unBracedFL <$> readPatch'
>          | otherwise
>              = read_patches
>       where read_patches :: ParserM m => m (Sealed (FL p C(x )))

Hmmm. So here we really parse the V2 legacy patches the same way we parse the
V1 legacy patches. If that's the easiest, why not. We have to distinguish
ListFormatV1 and ListFormatV2 for showing however, as will appear later on.

> replace ./src/Darcs/Patch/Read.hs [A-Za-z_0-9] unBracedFL unBracketedFL
> hunk ./src/Darcs/Patch/V2/Real.hs 33
>  import Darcs.Patch.Apply ( mapMaybeSnd )
>  import Darcs.Patch.Commute ( commuteFLorComplain, commuteRL, commuteRLFL )
>  import Darcs.Patch.ConflictMarking ( mangleUnravelled )
> -import Darcs.Patch.Format ( PatchListFormat )
> +import Darcs.Patch.Format ( PatchListFormat(..), ListFormat(..) )
>  import Darcs.Patch.Invert ( invertFL, invertRL )
>  import Darcs.Patch.Merge ( Merge(..) )
>  import Darcs.Patch.Prim ( Prim, FromPrim(..), ToFromPrim(..), Conflict(..), Effect(..),
> hunk ./src/Darcs/Patch/V2/Real.hs 706
>      applyAndTryToFixFL (Normal p) = mapMaybeSnd (mapFL_FL Normal) `liftM` applyAndTryToFixFL p
>      applyAndTryToFixFL x = do apply x; return Nothing
>  
> -instance PatchListFormat RealPatch
> +instance PatchListFormat RealPatch where
> +    patchListFormat = ListFormatV2
>  

In my copy of screened there is a comment here saying that talks about V1 prim
patches while it really appears to mean V2 patches. The comment looks copied
from the instance f PatchListFormat for Patch.

>  instance ShowPatchBasic RealPatch where
>      showPatch (Duplicate d) = blueText "duplicate" $$ showNon d
> hunk ./src/Darcs/Patch/Viewing.hs 128
>              showPatchInternal ListFormatV1      (p :>: NilFL) = showPatch p
>              showPatchInternal ListFormatV1      NilFL         = blueText "{" $$ blueText "}"
>              showPatchInternal ListFormatV1      ps            = blueText "{" $$ vcat (mapFL showPatch ps) $$ blueText "}"
> +            showPatchInternal ListFormatV2      ps            = vcat (mapFL showPatch ps)
>              showPatchInternal ListFormatDefault ps            = vcat (mapFL showPatch ps)

Here we add a case for showing an FL with context in the ListFormatV2. This
function does not have to do anything special, because in ListFormatV2 FLs are
not shown in any special way.

>  
>  instance (Apply p, Effect p, PatchListFormat p, ShowPatch p) => ShowPatch (FL p) where
> hunk ./src/Darcs/Patch/Viewing.hs 138
>              showContextPatchInternal ListFormatV1      NilFL         = return $ blueText "{" $$ blueText "}"
>              showContextPatchInternal ListFormatV1      ps            = do x <- showContextSeries ps
>                                                                            return $ blueText "{" $$ x $$ blueText "}"
> +            showContextPatchInternal ListFormatV2      ps            = showContextSeries ps
>              showContextPatchInternal ListFormatDefault ps            = showContextSeries ps
>  
>      description = vcat . mapFL description

And here is the code for showing an FL of ListFormatV2 patches without a
context. Nothing special here either.

Bye,
Reinier

Hi Reinier,

Thanks very much for the careful review.

On Sat, 8 Jan 2011, Reinier Lamers wrote:

> Here we add tarballs that I presume contain repositories with the above 
> patch bundle applied. Was the darcs-2 tarball created by applying the 
> bundle or by converting the darcs-1 repo created by applying the bundle?

I'm afraid I can't actually remember. I have a vague feeling I tried both 
and the end result was the same, but I'm not certain.

> Add another patch bundle containing a split patch. Now there are two 
> patches, the second of which contains the split patch. I wonder what 
> functionality this bundle requires that the previous one doesn't.

Again, hazy memory, but I think at one point I discovered a 
difference in the behaviour for patches that take a single line, like 
addfile, and ones that use multiple lines, like hunk. So I wanted 
to test both. Testing hunks may also be connected to checking the handling 
of the context lines.

> And here we check if bundles containing split patches can be applied. 
> Shouldn't we also check the internal representation of the patch in the 
> resulting repo? We do want to make sure that darcs does something 
> meaningful with the split patch. It is not enough if it just accepts the 
> syntax. OTOH, I realize it may be hard to test this.

apply is actually a pretty good check in itself, because it (should) check 
the bundle hash, which in turn involves round-tripping the patch through 
the datatype into the ShowPatch representation without any hunk context. 
So if apply succeeds, then either it's broken enough to skip the hash 
check or the right internal datatype was constructed.

Of course you're right that it would be better to actually do a test of 
the resulting repo. My inclination would be to test it with darcs 
commands, rather than actually checking the internal representation. 
Checking internal representations is worthwhile because it guarantees 
backwards compatibility, but doesn't port well to new repo formats.

> And thus we get rid of the code to compare two Split patches. Why did we need
> that unsafeCoerceP in the first removed line of comparePrim? compareFL doesn't
> require its arguments to have the same witnesses, does it?

No idea, I'm afraid. I vaguely remember wondering the same thing myself.

> It's inconsistent to have the one constructor named "Parens" and the other
> "Braced" (as opposed to "Parens" and "Braces"), but I can see "Braced" is
> there for historical reasons and "Parenthesized" is a bit long.

Yes, you're right. The historical reasons are actually also down to me, as 
I introduced "Braced" when I dropped ComP and only later realised it 
needed to be a bit more general purpose. A follow-up renaming Braced to 
Braces wouldn't hurt.

> What is that special case for (Braced NilFL) good for? Nothing apparently if
> we don't do such a thing for Parens. Or does it have something to do with the
> way vcat renders an empty list?

I duplicated it from the original code when I removed ComP. I think it is 
indeed to do with the way vcat renders an empty list - I have a vague 
feelign that something does actually go wrong without it. Split should 
never have an empty list, I believe, so it wasn't needed for that.

> Apparently there is a ListFormat type that specifies the kind of lists that can
> occur in a patch serialization. I find it confusing that the coment for the
> type says that is is (implicitly only) for *showing* patches, but the comments
> for the constructors also refer to *reading* patches. In practice it's used
> for both showing and reading.

OK, will update in a follow-up.

> In my copy of screened there is a comment here saying that talks about V1 prim
> patches while it really appears to mean V2 patches. The comment looks copied
> from the instance f PatchListFormat for Patch.

The comment is actually correct - there's a distinction between the 
numbering of darcs patches and of the underlying prim patches. Both V1 and 
V2 darcs patches (Patch/RealPatch) are built on V1 Prim patches.

Clearly this is confusing, but I couldn't think of a good alternative for 
the Prim numbering. Suggestions welcome!

Cheers,

Ganesh

Pushing as part of a simultaneous patch481-patch484
push-and-compile-and-test (previous review it's ok to push).

This patch bundle (with 3 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-be26b9d8de4a934a5748515f79d9aa3066329d0d

This patch bundle (with 3 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-be26b9d8de4a934a5748515f79d9aa3066329d0d