[darcs-devel] [patch482] add tests for reading of split patches (and 2 more)
Reinier Lamers
bugs at darcs.net
Sat Jan 8 12:18:14 UTC 2011
Reinier Lamers <tux_rocker at reinier.de> added the comment:
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 at 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 at earth.li>
> + * add files
> +
> +New patches:
> +
> +[add files
> +Ganesh Sittampalam <ganesh at 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 at earth.li>
> + * wibble
> +
> +Tue Nov 16 18:32:38 GMT 2010 Ganesh Sittampalam <ganesh at earth.li>
> + * 'nums'
> +
> +
> +New patches:
> +
> +[wibble
> +Ganesh Sittampalam <ganesh at earth.li>**20101116183225
> + Ignore-this: 5e5df9907f191511650c2ed66754d17
> +] {
> +addfile ./wibble
> +hunk ./wibble 1
> ++A
> ++B
> ++C
> ++D
> +}
> +['nums'
> +Ganesh Sittampalam <ganesh at 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 at 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 at . Basically, identical patches are equal and
> hunk ./src/Darcs/Patch/Prim/Core.lhs 911
> --- @Move < DP < FP < Split < Identity < ChangePref at .
> +-- @Move < DP < FP < Identity < ChangePref at .
> -- 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 at 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
__________________________________
Darcs bug tracker <bugs at darcs.net>
<http://bugs.darcs.net/patch482>
__________________________________
More information about the darcs-devel
mailing list