On Thu, Sep 11, 2008 at 12:34:02 -0700, Dmitry Kurochkin wrote: > Hi all!
Hi! My contribution to the review. For what it's worth, as a patch reviewer, I like this approach of starting with the cleanup patch and then changing stuff (because it makes life easier). Thanks. > When I do 'darcs send' for http://darcs.net/unstable repository target email > addresses are empty. Looks like there is something missing in prefs? Yeah. Although generally, I think we should encourage sending patches to stable (and making sure that stable stays pretty up to date) Formating and minor refactoring in URL.urlThread. ------------------------------------------------- Exactly as it says on the tin. Identation and some let expresisons > Dmitry Kurochkin <[EMAIL PROTECTED]>**20080910061227] hunk ./src/URL.hs 118 > + debugMessage $ "URL.urlThread ("++(url r)++"\n"++ > + " -> "++(file r)++")" Since we were doing this anyway, we could have removed the excess parentheses here, which helps readibility a tiny bit: ++ url r ++ "\n" ++ file r ++ ")" Do not download URL we have speculated before. ---------------------------------------------- > + d <- liftIO $ alreadyDownloaded u > + if d > + then dbg "Ignoring UrlRequest of URL that is already > downloaded." > + else do After the else, the rest of this is just indentation. Sigh, I wish indentation-only changes were somehow more self-evident as such. Right now, I rely on graphical diff tools to recognise them. > + alreadyDownloaded u = do > + n <- liftIO $ withMVar urlNotifications (return . (Map.lookup u)) > + case n of > + Just v -> isEmptyMVar v >>= return . not I think isEmptyMVar v >>= return . not is more simply expressed as not `fmap` isEmptyMVar v > hunk ./src/URL.hs 196 > - let r = UrlRequest u f c p v > - writeChan urlChan r > hunk ./src/URL.hs 198 > + let r = UrlRequest u f c p v > + writeChan urlChan r I don't really understand this change. I'm guessing we're just trying to note that we have tried to fetch a URL before we actually do it? Is it ok if we don't complete the fetch before somebody tries to look into urlNotifications? Sorry if that's a silly question. I don't get concurrency :-) Indentation fixes in configure.ac. ---------------------------------- As advertised Comment in configure.ac. ------------------------ Likewise Remove unused variable from configure.ac. ----------------------------------------- > - CURL_PIPELINING=True No comment. I'm just trusting Dmitry here. Print warning when '--http-pipelining' option is used, but darcs is compiled without HTTP pipelining support. ------------------------------------------------------------------------------------------------------------- > +#if !defined(HAVE_LIBWWW) && !defined(CURL_PIPELINING) > +import System.IO ( putStrLn ) > +#endif putStrLn is in the Prelude, so this shouldn't be necessary unless we're hiding it, which we don't seem to be. > + 1 >> (putStrLn $ "Warning: darcs is compiled without HTTP pipelining "++ > + "support, '--http-pipelining' argument is ignored.") Yay! I was worried people might get confused about this. Anybody have suggestions for wording? Resolve issue1054: --no-cache option to ignore patch caches. ------------------------------------------------------------ > +Do not use patch caches. I might say something a little bit more, maybe: Do not use patch caches even if they are defined > hunk ./src/Darcs/Arguments.lhs 1386 > - DarcsNoArgOption [] ["no-http-pipelining"] NoHTTPPipelining > no_pipelining_description]] > + DarcsNoArgOption [] ["no-http-pipelining"] NoHTTPPipelining > no_pipelining_description], > + DarcsNoArgOption [] ["no-cache"] NoCache > + "don't use patch caches"] Or maybe "ignore patch caches" Don't take me too seriously though. I'm terrible at naming or describing things > hunk ./src/Darcs/Repository/Prefs.lhs 442 > - | take 6 l == "cache:" = Just (Cache Directory Writable > (drop 6 l)) > - | take 9 l == "readonly:" = Just (Cache Directory > NotWritable (drop 9 l)) > + | take 6 l == "cache:" && NoCache `notElem` opts = Just > (Cache Directory Writable (drop 6 l)) > + | take 9 l == "readonly:" && NoCache `notElem` opts = > Just (Cache Directory NotWritable (drop 9 l)) Wouldn't it be better to start with a single case | NoCache `elem` opts = Nothing? This is another example of why reviewing patches is good for you. I didn't know there was a 'readonly' cache option until I saw this. Coding style in Darcs.Arguments.network_options. ------------------------------------------------ Just a whitespace tweak Coding style in Darcs.Repository.Prefs.getCaches. ------------------------------------------------- Same here, though I'm not so keen on this, because when everything was on one line, I could see how everything lined up more easily. Bikeshed :-) -- Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow> PGP Key ID: 08AC04F9
pgpi75vQnzohB.pgp
Description: PGP signature
_______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
