[darcs-users] [patch72] resolve issue1624 break global cache up into subdirectories

Luca Molteni luca_molteni at fastwebnet.it
Wed Dec 9 10:50:27 UTC 2009


Fixed those two whitespace things.

Bye,

L.M.



2009/11/29 Reinier Lamers <tux_rocker at reinier.de>:
> 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: 1624
Type: application/octet-stream
Size: 58254 bytes
Desc: not available
URL: <http://lists.osuosl.org/pipermail/darcs-users/attachments/20091209/96f9f4a0/attachment-0001.obj>


More information about the darcs-users mailing list