[darcs-users] darcs patch: resolve issue1131

[Eric Kow]

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]

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

> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : http://lists.osuosl.org/pipermail/darcs-users/attachments/20081008/c171dd42/attachment.pgp