Hi all, Op zondag 15 november 2009 17:17 schreef Luca Molteni: > New submission from Luca Molteni <[email protected]>:
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 <[email protected]>**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 @cachelocat...@. > 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
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
