On Tue, Aug 12, 2008 at 10:37:42PM -0700, Jason Dagit wrote:
> David,
>
> This is a big patch bundle, but most of the important changes are in
> SelectChanges. I found that module particularly tricky. I think
> the current types are right and I believe I didn't break anything
> during the refactor. And in fact, at least twice I tried to introduce
> bugs but the enhanced type safety caught it.
>
> Thanks,
> Jason
>
> Fri Aug 8 17:02:37 PDT 2008 Jason Dagit <[EMAIL PROTECTED]>
> * add type witnesses to Patch/Choices.lhs
>
> Sat Aug 9 23:34:03 PDT 2008 Jason Dagit <[EMAIL PROTECTED]>
> * add type witnesses to TouchesFiles
>
> Tue Aug 12 20:16:25 PDT 2008 Jason Dagit <[EMAIL PROTECTED]>
> * major refactor of SelectChanges to work with type witnesses
>
> Tue Aug 12 21:43:54 PDT 2008 Jason Dagit <[EMAIL PROTECTED]>
> * make WhatsNew work with type witnesses
>
> Tue Aug 12 22:04:25 PDT 2008 Jason Dagit <[EMAIL PROTECTED]>
> * refine type witnesses in SelectChanges
> [major refactor of SelectChanges to work with type witnesses
> Jason Dagit <[EMAIL PROTECTED]>**20080813031625] hunk
> ./src/Darcs/Commands/AmendRecord.lhs 120
> - with_selected_patch_from_repo "amend" repository opts True $ \ (oldp, _)
> -> do
> + with_selected_patch_from_repo "amend" repository opts True $ \ (_ :>
> oldp) -> do
> hunk ./src/Darcs/SelectChanges.lhs 82
> - -> FL p -- patches to select among
> - -> (FL p :> FL p -> IO a) -- job
> + -> FL p C(x y) -- patches to select among
> + -> (FORALL(m n) (FL p :> FL p) C(m n) -> IO a) -- job
It seems to me that the "job" here should be written without
second-rank polymorphism, i.e.
hunk ./src/Darcs/SelectChanges.lhs 82
- -> FL p -- patches to select among
- -> (FL p :> FL p -> IO a) -- job
+ -> FL p C(x y) -- patches to select among
+ -> ((FL p :> FL p) C(x y) -> IO a) -- job
because we really do know that when we select patches from a sequence,
the resulting unselected + selected patches must add up to the total!
This ought to make the code both easier to write and to debug--and
more errors can be caught by the type checker.
> hunk ./src/Darcs/SelectChanges.lhs 92
> - -> FL p -- patches to select among
> - -> (FL p :> FL p -> IO a) -- job
> + -> FL p C(x y) -- patches to select among
> + -> (FORALL(m n) (FL p :> FL p) C(m n) -> IO a) -- job
Same here, of course.
> hunk ./src/Darcs/SelectChanges.lhs 193
> -with_selected_patch_from_repo :: RepoPatch p => String -> Repository p ->
> [DarcsFlag] -> Bool
> - -> ((PatchInfoAnd p,[PatchInfoAnd p]) -> IO
> ()) -> IO ()
> +with_selected_patch_from_repo :: RepoPatch p => String -> Repository p C(r u
> t) -> [DarcsFlag] -> Bool
> + -> (FORALL(a b) ((FL (PatchInfoAnd p) :>
> (PatchInfoAnd p)) C(a b)) -> IO ()) -> IO ()
Once again, the FORALL here is not right, but in this case the result
is trickier, as we need to have:
with_selected_patch_from_repo :: RepoPatch p => String -> Repository p C(r u t)
-> [DarcsFlag] -> Bool
-> (FORALL(a) ((FL (PatchInfoAnd p) :>
(PatchInfoAnd p)) C(a r)) -> IO ()) -> IO ()
since with_selected_patch_from_repo returns the set of patches that
had to be commuted to select this patch, which involves an unknown
initial state.
> hunk ./src/Darcs/SelectChanges.lhs 214
> -wspfr :: RepoPatch p => String -> (PatchInfoAnd p -> Bool)
> - -> RL (PatchInfoAnd p) -> [PatchInfoAnd p]
> - -> IO (Maybe (PatchInfoAnd p, [PatchInfoAnd p]))
> +wspfr :: RepoPatch p => String -> (FORALL(a b) (PatchInfoAnd p) C(a b) ->
> Bool)
> + -> RL (PatchInfoAnd p) C(x y) -> FL (PatchInfoAnd p) C(y z)
> + -> IO (Maybe (FlippedSeal (FL (PatchInfoAnd p) :> (PatchInfoAnd p))
> C(z)))
Interestingly enough, this one looks correct, so the above change to
with_selected_patch_from_repo ought to be trivial.
> hunk ./src/Darcs/SelectChanges.lhs 249
> -
> -commute_by :: Patchy p => [p] :< p -> Maybe (p :< [p])
> -commute_by ([] :< a) = Just (a :< [])
> -commute_by (p:ps :< a) =
> - case commute (a :> p) of
> - Nothing -> Nothing
> - Just (p':>a') -> case commute_by (ps :< a') of
> - Nothing -> Nothing
> - Just (a'' :< ps') -> Just (a'' :< p':ps')
Yay for eliminating commute_by! (the sort of dangerous code
duplication we really don't want...)
This is as far as I'll get today. Do you think you could make the
above type changes and see if they require much of a rewrite? If much
changes, it'd be nice if you could amend-record and resend.
Otherwise, just send with a new patch changing the above-mentioned
types, and mention in the email that it was a small change. Thanks!
David
_______________________________________________
darcs-users mailing list
[email protected]
http://lists.osuosl.org/mailman/listinfo/darcs-users
