[darcs-users] [patch421] getting rid of ComP constructor in Patch

[Ganesh Sittampalam]

On Thu, 11 Nov 2010, Florent Becker wrote:

> hunk ./src/Darcs/Repository/Internal.hs 507
>> -      writePatch tpn $ fromPrims_ newpend_
>> +      writeTentativePending repo (unsafeCoercePStart newpend_)
>>        where newpend :: FL Prim C(a b) -> FL Prim C(b c) ->
> FlippedSeal (FL Prim) C(c)
>>              newpend NilFL patch_ = flipSeal patch_
>>              newpend p     patch_ = flipSeal $ p +>+ patch_
>
> Why seal then coerce, rather than coerce directly?
>
> hunk ./src/Darcs/Repository/Internal.hs 525
>>  prepend :: forall p C(r u t x y). RepoPatch p =>
>>             Repository p C(r u t) -> FL Prim C(x y) -> IO ()
>>  prepend (Repo _ opts _ _) _ | NoUpdateWorking `elem` opts = return ()
>> -prepend (Repo _ _ _ rt) patch =
>> -    do let pn = pendingName rt ++ ".tentative"
>> -       Sealed pend <- readPrims `liftM` (gzReadFilePS pn `catchall`
> (return B.empty))
>> -       Sealed newpend_ <- return $ newpend pend patch
>> -       writePatch pn $ fromPrims_ (crudeSift newpend_)
>> +prepend repo@(Repo _ _ _ _) patch =
>> +    do
>> +       Sealed pend <- readTentativePending repo
>> +       Sealed newpend_ <- return $ newpend (unsafeCoerceP pend) patch
>> +       writeTentativePending repo (unsafeCoercePStart $ crudeSift
> newpend_)
>>        where newpend :: FL Prim C(b c) -> FL Prim C(a b) -> Sealed (FL
> Prim C(a))
>>              newpend NilFL patch_ = seal patch_
>>              newpend p     patch_ = seal $ patch_ +>+ p
>
> as above, why seal then coerce?
>
> What's more, there are unsafeCoerces because of the function's
> unsafety. On the other hand, prepend is called only once, with
> x = t. Shouldn't it be reflected in its type, and wouldn't it reduce
> the need for unsafeCoerces?

I think you're right; I think I was aiming for minimal changes and so I 
chose unsafeCoercing (which won't affect behaviour) over more significant 
refactorings. Worth a followup, though given the general witness-y mess in 
Repository doing it properly will be a lot of effort.

> hunk ./src/Darcs/Repository/LowLevel.hs 23
>>
>>  #include "gadts.h"
>>
>> -module Darcs.Repository.LowLevel ( readPending, readPendingfile,
> pendingName, readPrims ) where
>> +module Darcs.Repository.LowLevel
>> +    ( readPending, readTentativePending
>> +    , writeTentativePending
>> +    -- deprecated interface:
>> +    , readPendingfile, writePendingFile
>> +    , pendingName )
>> +    where
>>
>>  import Darcs.Repository.InternalTypes ( RepoType(..), Repository(..)
> )
>
> Would it be possible to tell hlint that these functions are deprecated?

Possibly, but I don't think it is worth it as noone will be messing with 
this stuff without looking at the code quite carefully. In fact in later 
(as yet unsubmitted) patches I've got rid of those deprecated functions, 
but I have some different ones instead (to handle "pending.new").

> darcs does not compile if we correct the type of readPending. Does
> that means (potential) bugs?

I think the same confused assumption is made through the stack, so it sort 
of works out. But yes, there may well be some lurking. I've made a couple 
of half-hearted attempts at untangling this, but it's fiddly and the mess 
in Repository.Internal makes it harder.

Ganesh