On Tue, Jun 01, 2010 at 16:28:54 +0000, Adolfo Builes wrote: > Mon May 31 20:15:34 COT 2010 [email protected] > * Resolve issue 1503
Hooray, first GSoC patch! OK, some minor things to fix at least: * the patch name * separate out unrelated patches from content patch Things to think about and discuss if needed: * design/functionality questions * stylistic changes * testing As for testing: Is there any reasonable way you can think of to test this patch automatically? See the network directory for some examples. We could set up some test repositories on darcs.net if you need. Resolve issue 1503 ------------------ This should have a slightly more descriptive title. For example, Resolve issue1503: treat pull repos as source entries You're not always going to be able to fit everything into the patch name, but just get the important points down so that people know what it is at a glance. You could always say more in the patch log if you feel the need. http://wiki.darcs.net/Development/GettingStarted > -import Darcs.Repository.Merge Cleanup changes like this (and your trailing whitespace cleanup) should be done in a separate patch so that it's clearer what the actual meat of your work is. > +import qualified Darcs.Repository.InternalTypes as DRI Importing these internal modules is a often bad sign (sometimes it's a necessary evil, because the Darcs.Repository.* layer is not very clean). Do we really need it? Can we import from Darcs.Repository instead? > pullCmd :: [DarcsFlag] -> [String] -> IO () > pullCmd opts repos = > - withRepoLock opts $- \repository -> > - fetchPatches opts' repos "pull" repository >>= applyPatches opts' > repository > + withRepoLock opts $- \repository -> do > + repository' <- addLocalReposToCache repository repos > + out <- fetchPatches opts' repos "pull" repository' > + applyPatches opts' repository' out Note that in situations like this, it's easy (for me) to accidentally use the wrong identifier (eg. repository when I mean repository'). I wonder if there's a way you can get rid of one of these points somehow. Maybe some sort of partial application thing like withRepoLock opts $- addLocalReposToCache repos >>= $ \repository -> > hunk ./src/Darcs/Commands/Pull.lhs 186 > +-- | If we are pulling from local repositories, they are appended to > +-- the list of the caches loaded from sources, the idea behind is to > +-- avoid going to remote repositories, as darcs always try first the > +-- ones read from sources, before trying the one given as arguments > +-- with the command pull. Thanks for the haddock. This is interesting because your code makes this only half-true now, in the sense that Darcs will only do this if the repos we are pulling from are remote ones. So maybe a clearer way here is not to talk about how Darcs behaves in general and just focus on the specific case. For example, "this is to ensure that local repositories will be tried first, even if there are already remote repositories in the sources." > +addLocalReposToCache :: forall p C(r u). RepoPatch p => Repository p C(r u > r) -> > + [String] -> IO (Repository p C(r u r)) > +addLocalReposToCache (DRI.Repo dir opts rf(DRI.DarcsRepository pristine > (DRC.Ca ccache))) repos = > + do > + pullingFrom <- (mapM (fixUrl opts) repos) > + let cache = DRC.Ca (appendRepos ccache pullingFrom) > + return (DRI.Repo dir opts rf (DRI.DarcsRepository pristine cache)) I wonder if it would be better to have a function on the Darcs.Repository layer like modifyCache :: forall p C(r u t). RepoPatch p => (Cache -> Cache) -> Repository p C(r u t) -> Repository p C(r u t) > + where > + appendRepos cache [] = cache > + appendRepos cache (x:xs) = if isFile x > + then > + (DRC.Cache DRC.Repo DRC.NotWritable x) : > appendRepos cache xs > + else > + appendRepos cache xs Hmm, this seems like a step in the right direction because it would ensure that when pulling from local repositories, we will not accidentally prioritise any remote repositories in the sources list. It sounds like this would solve this particular bug. But how about a more general solution which divides cache entries by locality? For example you could say something like compareByLocality x y | isLocal x && isRemote y = LT | isRemote x && isLocal y = GT | otherwise = compare x y Your approach is a bit more conservative, but I think the approach I suggest is a bit more general as it will cause Darcs to systematically prefer all local cache entries to remote ones. Stylistic issues [hlint could be helpful here]: * I think you mean prepend instead of append (I think appending xs to ys means ys ++ xs). * It seems like appendRepos would be a simpler with filter/map instead of explicit recursion. * Superfluous parentheses (mapM...) -- Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow> PGP Key ID: 08AC04F9
signature.asc
Description: Digital signature
_______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
