OK, I think I'm almost ready to apply this (no obvious bugs or things to complain about).
But I have some design questions to ask you, so I'd like to wait for you
to tell me what is the right thing to do. (This really is largely about
me not knowing how to manage certain tradeoffs, so what I'm trying to do
is make sure that you've at least thought about them and are not
counting on me to have the right answers).
An example of the kind of trade-off to manage is "How close to the core
should X bit of code be? Do I want it in the surface-level code? Or
does it belong deeper down?" One could spend hours arguing about it,
which may not be productive. Alternatively, we could at least make sure
we've made a sort of "best effort" at thinking about it and then just
make a decision and see how things go. So please let me know what the
right thing to do is :-)
Resolve issue 1503: try local repos before remote ones.
-------------------------------------------------------
> + pullingFrom <- mapM (fixUrl opts) repos
> + withRepoLock opts $- \initialRepository ->
> + modifyCache initialRepository pullingFrom >>= \repository ->
> + fetchPatches opts' repos "pull" repository >>= applyPatches opts'
> repository
OK, we still have the two repositories here but at least it's clearer
which one you should be using at all times.
You could get rid of the extra argument by reversing the definition of
modifyCache so it lends itself more to partial application.
> +-- | Add locals repositories to cache, ensuring that locals are tried before
> any remote one.
> +modifyCache :: forall p C(r u). RepoPatch p => Repository p C(r u r) ->
> + [String] -> IO (Repository p C(r u r))
> +modifyCache (Repo dir opts rf(DarcsRepository pristine (Ca ccache))) repos =
I think modifyCache would be more useful as a higher order function
(otherwise, I would be not so inclined to have it in the
Darcs.Repository hierarchy).
Rather than being specifically about adding the local repositories to
the cache, it could just be about applying some (Cache -> Cache) on the
repository's cache.
> + do
> + let cache = Ca (addLocals ccache repos)
> + return (Repo dir opts rf (DarcsRepository pristine cache))
> + where
> + addLocals = foldr (\a b -> if isFile a
> + then Cache DarcsCache.Repo NotWritable a : b
> + else b)
OK, you've gotten rid of the explicit recursion, but perhaps something
like this formulation would be clearer?
addLocals xs ys = locals xs ++ ys
locals = map (Cache DarcsCache.Repo NotWritable) . filter isFile
Or maybe
addLocals xs ys = [ Cache DarcsCache.Repo NotWritable x | x <- xs, isFile x ]
++ ys
> +compareByLocality (Cache _ _ x) (Cache _ _ y)
> + | isFile x && (isUrl y || isSsh y) = LT
> + | (isUrl x || isSsh x) && isFile y = GT
> + | otherwise = EQ
Every top level function should have a type signature.
You can refactor this slightly with something like
where
isRemote z = isUrl z || isSSh z
Or maybe it'd make sense in the interest of clarity to define both
where
isLocal z = isFile z
isRemote z = isUrl z || isSSh z
Or maybe we want to make it clear that the two cases are mutually
exclusive (in which case you need to reason first if that's what
you really want to say)
where
isLocal = isFile
isRemote = not . isLocal
> copyFileUsingCache :: OrOnlySpeculate -> Cache -> HashedDir -> String -> IO
> ()
> -copyFileUsingCache oos (Ca cache) subdir f =
> - do debugMessage $ "I'm doing copyFileUsingCache on "++(hashedDir
> subdir)++"/"++f
> +copyFileUsingCache oos (Ca unsortedCache) subdir f =
> + do let (Ca cache) = Ca (sortBy compareByLocality unsortedCache)
> + debugMessage $ "I'm doing copyFileUsingCache on "++(hashedDir
> subdir)++"/"++f
Interesting. One question: does this sorting belong here? What if in
the future we want to sort by proximity to the repository directory?
How would we manage that? What's the right attitude to adopt here? One
possible answer, for example, is YAGNI. Is that the right one?
> fetchFileUsingCachePrivate :: FromWhere -> Cache -> HashedDir -> String ->
> IO (String, B.ByteString)
> -fetchFileUsingCachePrivate fromWhere (Ca cache) subdir f =
> +fetchFileUsingCachePrivate fromWhere (Ca unsortedCache) subdir f =
> do when (fromWhere == Anywhere) $ copyFileUsingCache ActuallyCopy (Ca
> cache) subdir f
> ffuc cache
> `catchall` debugFail ("Couldn't fetch `"++f++"'\nin subdir "++(hashedDir
> subdir)++
> hunk ./src/Darcs/Repository/Cache.hs 265
>
> ffuc [] = debugFail $ "No sources from which to fetch file
> `"++f++"'\n"++ show (Ca cache)
>
> + Ca cache = Ca (sortBy compareByLocality unsortedCache)
What's the difference between these two functions?
--
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9
signature.asc
Description: Digital signature
_______________________________________________ darcs-users mailing list darcs-users@darcs.net http://lists.osuosl.org/mailman/listinfo/darcs-users
