[darcs-users] darcs patch: resolve issue1131

Dmitry Kurochkin dmitry.kurochkin at gmail.com
Thu Oct 9 10:45:34 UTC 2008


On Wed, Oct 8, 2008 at 7:55 PM, Eric Kow <kowey at darcs.net> wrote:
> On Wed, Oct 08, 2008 at 15:55:36 +0400, Dmitry Kurochkin wrote:
>> I am not able to reproduce the issue. So it is not tested. But it is
>> pretty simple so I do not think it introduces any new bugs (and
>> hopefully fixes one :)).
>
> Secondary review.  I think I'm happy to have this in stable.
>
> Resolve issue1131: accept download requests for different files.
> ----------------------------------------------------------------
>> +data UrlState = UrlState { inProgress :: Map String ( FilePath
>> +                                                    , [FilePath]
>> +                                                    , Cachable
>> +                                                    , (MVar String) )
>
> Before we mapped from a URL to the FilePath it should be saved as when
> downloaded.  Now we map to that and a list of possible FilePaths in
> case we ask for it with different local names.  (We still don't know
> why it would do this yet).  In principle, I suppose we could just
> replace FilePath, [FilePath] by [FilePath]

I wanted to put it into a single list first. But in this case we have to:
1. original file is first, append extra files to tail.
2. original file is last, append extra files to head.

I did not want to append from tail or keep original file last. So I
decided to store original file separately, afterall it is treated
specially.

>
>> hunk ./src/URL.hs 150
>>                   case Map.lookup u p of
>
> Are we already in the process of or planning to download this file?
> If not, stick it in the queuue (put new_nst), otherwise...
>
> BEFORE
>> -                   Just (f', c', _) ->
>> -                     if f == f' && c == c'
>> -                     then if u `elemQ` w && priority r == High
>> -                          then do put $ st { waitToStart = pushQ u (deleteQ u w) }
>>                                    dbg $ "Moving "++u++" to head of download queue."
>> -                          else dbg "Ignoring UrlRequest of file that's already queued."
>> -                     else bug $ "URL.urlThread: same URLs with different parameters "++u++" "++f++ show (f,f',c,c')
>
> AFTER
>> +                   Just (f', fs', c', v) ->
>> +                     if c /= c'
>> +                     then bug $ "URL.urlThread: same URLs with different parameters "++u++" "++show (c,c')
>> +                     else do when (u `elemQ` w && priority r == High) $ do
>> +                                  modify (\s -> s { waitToStart = pushQ u (deleteQ u w) })
>>                                    dbg $ "Moving "++u++" to head of download queue."
>> +                             if f `notElem` (f':fs')
>> +                                then let new_p = Map.insert u (f', f:fs', c', v) p
>> +                                     in do modify (\s -> s { inProgress = new_p })
>> +                                           dbg "Adding new file to existing UrlRequest."
>> +                                else dbg "Ignoring UrlRequest of file that's already queued."
>
> Are you happy with this summary, Dmitry?
>
> 'muzak' here is my shorthand for 'print the ignoring urlrequest message'
>
> -------------------------------------------+--------+--------------------
>                                           | BEFORE | AFTER
> ---------+--------------+------------------+--------+--------------------
> filename | cacheability | waiting+priority | hurry  | hurry; muzak
> filename | cacheability |                  | muzak  |        muzak
> filename |              | waiting+priority | bug!   | bug!
> filename |              |                  | bug!   | bug!
> ---------+--------------+------------------+--------+--------------------
>         | cacheability | waiting+priority | bug!   | hurry; extra name
>         | cacheability |                  | bug!   |        extra name
>         |              | waiting+priority | bug!   | bug!
>         |              |                  | bug!   | bug!
> ---------+--------------+------------------+--------+--------------------
>         |              | waiting+priority | bug!   | bug!
> ---------+--------------+------------------+--------+--------------------

I am not sure what the last row is for - it is the same as two row
above. Otherwise this looks correct.

Thanks for the review!

Regards,
  Dmitry

>
>> hunk ./src/URL.hs 224
>> +                                  mapM_ (copyFile f) fs
>
> When we download a file, make sure that it gets copied to all its new
> names.
>
> Pitfall check
> -------------
>> - put $ st { waitToStart = pushQ u (deleteQ u w) }
>> + modify (\s -> s { waitToStart = pushQ u (deleteQ u w) })
>
> * Possibility that the current state is not the same st?
>  Safe - verified that there are no intervening puts or modifies
> * Interaction between f' and fs
>  Safe - we always check f' along with fs
> * Copying over the file we just downloaded
>  Safe - f' doesn't go in fs (unless there is some kind of crazy link)
>
> --
> Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
> PGP Key ID: 08AC04F9
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.6 (GNU/Linux)
>
> iD8DBQFI7NfxBUrOwgisBPkRAiyXAKCTwghNdy6plakB/52lfJiAvFzbowCeM249
> qNWdNqI7ZzMhtpzTQ9GYmBw=
> =Gjdj
> -----END PGP SIGNATURE-----
>
>


More information about the darcs-users mailing list