[darcs-users] darcs patch: Formating and minor refactoring in URL.u... (and 8 more)

[Dmitry Kurochkin]

On Fri, Sep 12, 2008 at 12:51 AM, Eric Kow <kowey at darcs.net> wrote:
> 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.

Thanks for the review, Eric. Updated patch bundle attached, please
ignore the previous one.

[skip]
>> Dmitry Kurochkin <dmitry.kurochkin at gmail.com>**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 ++ ")"

Done, in a separate patch though. Could not amend-record because of
patch dependencies. Is there a walk around for this?

[skip]
>> +          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

Cool! Changed.

>
>> 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 :-)

The point of this change is to guarantee that urlNotifications is
updated before urlThread reads request from the channel. And yes, it
is fine to access notifications before download is complete - in fact
this happens in most cases.

[skip]
> 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.

Oh. Removed.

>
>> +    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?

Better wording is welcomed!

>
> 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

I do not think 'ignore' is any better than 'do not use'. So left as is
for now. But that would be great if someone provided better
documentation/description here - I do not like my wording.

>
>> 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?

Good point! Done.

[skip]
> 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
> :-)

I have obliterated this patch since now (after the above change) these
lines are not that long. But in general I do not like when lines are
more than 80 columns.

Regards,
  Dmitry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: formating-and-minor-refactoring-in-url_urlthread_.dpatch
Type: application/octet-stream
Size: 55672 bytes
Desc: not available
Url : http://lists.osuosl.org/pipermail/darcs-users/attachments/20080912/2ae57abc/attachment-0001.obj