[darcs-users] [patch72] resolve issue1624 break global cache up into subdirectories
Reinier Lamers
tux_rocker at reinier.de
Sun Nov 29 17:58:25 UTC 2009
Hi all,
Op zondag 15 november 2009 17:17 schreef Luca Molteni:
> New submission from Luca Molteni <volothamp at gmail.com>:
Looks like it's about time for a review. I have a lot of questions about it.
In a large part, that is because the existing cache code seems a bit dense
to read through.
>[issue1624 break global cache up into subdirectories
>Luca Molteni <volothamp at gmail.com>**20091115160511
> Ignore-this: ae4555fd3172a53549d90e42993dbce4
>] hunk ./src/Darcs/Repository/Cache.hs 23
> import System.Directory ( removeFile, doesFileExist, getDirectoryContents )
> import System.Posix.Files ( linkCount, getSymbolicLinkStatus )
> import System.IO ( hPutStrLn, stderr )
>+import System.FilePath (takeDirectory)
Nitpick: as you can see, most of darcs source leaves spaces between the
names of the imported functions and the parenthesis.
> hunk ./src/Darcs/Repository/Cache.hs 113
> -- | @hashedFilePath cachelocation subdir hash@ returns the physical filename of
> -- hash @hash@ in the @subdir@ section of @cachelocation at .
> hashedFilePath :: CacheLoc -> HashedDir -> String -> String
> -hashedFilePath (Cache Directory _ d) s f = d ++ "/" ++ (hashedDir s) ++ "/" ++ f
> +hashedFilePath (Cache Directory _ d) s f = d ++ "/" ++ (hashedDir s) ++ "/" ++ (hashedBucket f) ++ "/" ++ f
> + where hashedBucket f = take 2 $ case dropWhile (/='-') f of
> + [] -> f
> + s -> drop 1 s
I initally assumed that f was a filename because of its name, but it is a
hash. But that is not your mistake.
Where does the dash ('-') in the hash come from? In what situation won't it
be present, invoking the [] case in hashedBucket?
What happens with old caches that still use the older names? Aren't they made useless by this change?
> hunk ./src/Darcs/Repository/Cache.hs 146
> copyFileUsingCache oos (Ca cache) subdir f =
> do debugMessage $ "I'm doing copyFileUsingCache on "++(hashedDir subdir)++"/"++f
> Just stickItHere <- cacheLoc cache
> - createDirectoryIfMissing False (reverse $ dropWhile (/='/') $ reverse stickItHere)
> + createBucket stickItHere
> sfuc cache stickItHere
> `catchall` return ()
> where cacheLoc [] = return Nothing
I'm willing to believe that this change does the right thing, but the
copyFileUsingCache function is hermetic poetry to me.
> hunk ./src/Darcs/Repository/Cache.hs 168
>
> data FromWhere = LocalOnly | Anywhere deriving ( Eq )
>
> +createBucket :: String -> IO ()
> +createBucket s = createDirectoryIfMissing True (takeDirectory s)
> +
Why do you change the first argument of createDirectoryIfMissing to True?
> hunk ./src/Darcs/Repository/Cache.hs 206
> return (fn c, x)
> `catchall` do (fname,x) <- ffuc cs
> do createCache c subdir
> + createBucket (fn c)
> createLink fname (fn c)
> return (fn c, x)
> `catchall`
Why is this necessary?
> hunk ./src/Darcs/Repository/Cache.hs 247
> where hash = cacheHash ps
> wfuc (c:cs) | not $ writable c = wfuc cs
> | otherwise = do createCache c subdir
> + createBucket (fn c)
> write compr (fn c) ps -- FIXME: create links in caches
> return hash
> wfuc [] = debugFail $ "No location to write file `" ++ (hashedDir subdir) ++"/"++hash ++ "'"
And this?
> hunk ./src/Darcs/Repository/Cache.hs 252
> fn c = hashedFilePath c subdir hash
> +
This looks like a spurious whitespace change. They may cause unnecessary
conflicts later on, hence they are not so popular.
Bye,
Reinier
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.osuosl.org/pipermail/darcs-users/attachments/20091129/c11a5f0f/attachment.pgp>
More information about the darcs-users
mailing list