There is an implicit point behind this patch, which is that while it is very worthwhile to be having a discussion on what kind of technology we need to be passing flags around the right way, sometimes all you to do is just stop doing it :-)
Also, I just want to point out to darcs-users that these are exactly the
kind of patches you can review with just rudimentary Haskell, and no
special darcs knowledge.
Refactor Cache's handling of hashed paths. No functional change.
----------------------------------------------------------------
> +hashedFilePath :: CacheLoc -> HashedDir -> String -> String
> +hashedFilePath (Cache Directory _ d) s f = d ++ "/" ++ (hashedDir s) ++ "/"
> ++ f
> +hashedFilePath (Cache Repo _ r) s f =
> + r ++ "/"++darcsdir++"/" ++ (hashedDir s) ++ "/" ++ f
Another minor note: This could return FilePath, for documentation
purposes :-)
> + cacheHasIt (c:cs) | not $ writable c = cacheHasIt cs
> + | otherwise = do ex <- doesFileExist $ fn c
> + if ex then return True
> + else cacheHasIt cs
> + fn c = hashedFilePath c subdir f
Perhaps how I might have written this is is
cacheHasIt cs = or `fmap` mapM fileExists (filter writable cs)
fileExists c = doesFileExist $ hashedFilePath c subdir f
I like how Haskell makes it easy for me to separate things out into
single-purpose loops. But I guess it depends on what you consider
to be a readable. Sometimes the explicit recursion is friendlier.
> speculateFileUsingCache :: Cache -> HashedDir -> String -> IO ()
> speculateFileUsingCache c sd h = do debugMessage $ "Speculating on "++h
Did you ever figure out what it means to 'speculate' on a file? It may
be worth haddocking if you have the time :-)
> hunk ./src/Darcs/Repository/Cache.lhs 143
> sfuc cache stickItHere
> `catchall` return ()
> where cacheLoc [] = return Nothing
> - cacheLoc (Cache _ NotWritable _:cs) = cacheLoc cs
> - cacheLoc (Cache t Writable d:cs) =
> - do ex <- doesFileExist (fn t d)
> + cacheLoc (c:cs) | not $ writable c = cacheLoc cs
> + | otherwise =
> + do ex <- doesFileExist $ fn c
> if ex then fail "Bug in darcs: This exception should be
> caught in speculateFileUsingCache"
> else do othercache <- cacheLoc cs
> case othercache of Just x -> return $ Just x
This again might be another place where it could be useful to split the
recursion out from the business code
> + where ffuc (c:cs)
> + | not (writable c) && (Anywhere == fromWhere || is_file (fn c)) =
I haven't looked at this refactor closesly, but it seems ok.
Add data Compression to Darcs.Flags.
------------------------------------
> +data Compression = NoCompression | GzipCompression
> +compression :: [DarcsFlag] -> Compression
> +compression f | NoCompress `elem` f = NoCompression
> + | otherwise = GzipCompression
Looks good.
Replace [DarcsFlag] with Compression in HashedIO Slurpy.
--------------------------------------------------------
I'm a fan of this kind of work.
One of the things that makes darcs's code needlessly scary is that you
never know how all those [DarcsFlag] are going to get used. The
downside is that we have a bit less flexibility, but I think it's also
a good thing to force us to change these types if we change functionality
accordingly.
This also looks safe in that I didn't see any default [DarcsFlag] being
passed in to functions that want them. In other words, it's looks like
it's all Compression.
> wfuc (c:cs) | not $ writable c = wfuc cs
> | otherwise = do createCache c subdir
> - -- FIXME: create links in caches
> - if NoCompress `elem` opts
> - then writeAtomicFilePS (fn c) ps
> - else gzWriteAtomicFilePS (fn c) ps
> + write compr (fn c) ps -- FIXME:
> create links in caches
Nice.
> slurpHashed :: Cache -> [DarcsFlag] -> String -> IO Slurpy
> slurpHashed c opts h = fst `fmap` runStateT slh
> (HashDir { permissions = RO, cache = c,
> - options = opts, rootHash = h })
> + compress = (compression opts),
> rootHash = h })
Avoidable parentheses.
> -syncHashed c s r = do runStateT sh $ HashDir {permissions=RW, cache=c,
> options=[], rootHash=r }
> +syncHashed c s r = do runStateT sh $ HashDir { permissions=RW, cache=c,
> + compress=compression
> [], rootHash=r }
Just nodding my head: compression [] lets us change the default
compression options later
> +listHashedContents :: String -> Cache -> String -> IO [String]
> +listHashedContents k c root =
...
> - x <- fst `fmap` runStateT (lhc (D,fp2fn ".",root)) (HashDir RO c opts
> root)
> + x <- fst `fmap` runStateT (lhc (D,fp2fn ".",root)) (HashDir RO c
> NoCompression root)
I'm guessing it is reasonable to assume NoCompression here because we
never write to the cache? May be worth a quick little comment.
Remove a few unused [DarcsFlag] parameters from HashedRepo and
Repository.Internal.
------------------------------------------------------------------------------------
Looks good. I didn't notice any gotchas.
Remove [DarcsFlag] use in (most of) HashedIO, HashedRepo API.
-------------------------------------------------------------
No shockers here. Hopefully we can revisit this code and make it tidier
when we have a clearer idea what are all the opts being used in Repository
--
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9
pgpIzNRKbcdgI.pgp
Description: PGP signature
_______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
