Eric Kow <kowey@darcs.net> writes:
> So why don't hashed repositories suffer from issue1942?
> Is it just by virtue of being able to treat the current
> repository as a source for hashed files that we'd otherwise
> have to fetch from the remote one.
I believe the IO interleaving works better in the hashed version of this
code, although it could stand a double-check. If it didn't, we could
save some time and memory by not parsing patches when they aren't
needed.
> Resolve issue1942: Fix an IO interleaving bug in old-fashioned readRepo.
> ------------------------------------------------------------------------
>> parse2 :: RepoPatch p => PatchInfo -> FilePath
>> -> IO (Sealed (PatchInfoAnd p C(x)))
>> - parse2 i fn = do ps <- gzFetchFilePS fn Cachable
>> - return $ hopefullyNoParseError i (toPath fn) (readPatch ps)
>> - hopefullyNoParseError :: PatchInfo -> String
>> - -> Maybe (Sealed (Named a1dr C(x)), b)
>> - -> Sealed (PatchInfoAnd a1dr C(x))
>> - hopefullyNoParseError i _ (Just (Sealed x, _)) =
>> - seal $ patchInfoAndPatch i $ actually x
>> - hopefullyNoParseError i s Nothing =
>> - seal $ patchInfoAndPatch i $ unavailable $ "Couldn't parse file "++s
>
>> + parse2 i fn = do ps <- unsafeInterleaveIO $ gzFetchFilePS fn Cachable
>> + Sealed p <- return $ hopefullyNoParseError (toPath fn) (readPatch ps)
>> + return $ seal $ patchInfoAndPatch i p
>> + hopefullyNoParseError :: String -> Maybe (Sealed (Named a1dr C(x)), b)
>> + -> Sealed (Hopefully (Named a1dr) C(x))
>> + hopefullyNoParseError _ (Just (Sealed x, _)) = seal $ actually x
>> + hopefullyNoParseError s Nothing = seal $ unavailable $ "Couldn't parse file "++s
>
> OK, two changes:
>
> 1. Use of unsafeInterleaveIO to defer gzFetchFilePS until the
> ByteString it produces is actually demanded (by readPatch,
> and I guess walking up to whoever is really using the output of
> readPatch, and so on). And if I understand correctly, in
> practice (darcs pull), we actually do have cases where we
> don't actually use the remote patch file in question
> (I guess where don't try to commute or present remote patches)
>
> What are the risks involved in this particular use of
> unsafeInterleaveIO, of not controlling when the file
> fetching happens? What if there are a lot of patches
> to potentially fetch?
It is as safe as the whole Hopefully concept is. Read that as "not very"
but it is baked so deep into darcs that we can't do anything about it
for now.
> 2. Shuffling the patchInfoAndPatch out of hopefullyNoParseError
> Aside from a refactor, what does this accomplish?
>
> Hmm, I still have an embarrassingly fuzzy grasp of laziness, but
> I suspect I'm due for an ah-hah-nothing-to-it moment. Or that's
> maybe too optimistic.
Because the "i" does not get shuffled out of the scope of the pattern
match on the result of the parsing by GHC (and I am not sure it could
be). So to get at "info p" (which comes from the inventory) you have to
download the patch to see if it parses correctly (to choose the branch
in hopefullyNoParseError). This was certainly not intentional.
Yours,
Petr.
|