[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