So, I've had a look at the first few patches and have little to add except a few annotations on pre-existing darcs code or stuff that Jason and David have already agreed on.
> I constructed this set of patches by pulling over my previous patches,
> unrecording them, fixing up conflicts, checking that they compile with/without
> type witnesses and every several files checking that the tests still pass.
I realise that this is a pretty massive pain in the rear, but I think
not only will this make David and other reviewers' job easier, it's also
the kind of thing that future-Jason will thank you for when he's poring
over ChangeLogs and trying to figure something out :-)
Being in deep must-get-job-done territory, I can understand that caring
about future-Jason is hard to do. I guess I can't do much else here but
wish you great courage!
Add Repository IO monad, RIO.
-----------------------------
So David has already reviewed this, but I glanced at it anyway out of
curiosity and desire to some day get past the
write-your-first-monad-tutorial part of my Haskell education.
For other people in my shoes, the RIO monad does one thing and one
thing only
(>>>=) :: RIO p C(r u t t1) a
-> (a -> RIO p C(r u t1 t2) b)
-> RIO p C(r u t t2) b
Given an RIO action X followed by the RIO action Y, the monad enforces a
simple constraint, namely that the tentative recorded state after X s
same as the one before Y. Of course, we're just dealing with type
witnesses here, so these states do not actually have any concrete
representation in reality. They're just little markers we jot down for
compile-time verification of our expectations. If our little markers
fail to unify the way we think they should, then we know we're in
trouble!
> +-- | Repository IO monad. This monad-like datatype is responsible for
> +-- sequencing IO actions that modify the tentative recorded state of
> +-- the repository.
> +newtype RIO p C(r u t t1) a = RIO {
> + unsafeUnRIO :: Repository p C(r u t) -> IO a -- ^ converts @RIO a@
> to @IO [EMAIL PROTECTED]
I generally like it when 'unsafe' functions tell me in what sense they
are unsafe. Here I guess it's because we stop tracking type witnesses.
> +-- | This corresponds to @>>@ from the Monad class.
> +(>>>) :: RIO p C(r u t t1) a -> RIO p C(r u t1 t2) b -> RIO p C(r u t t2) b
> +a >>> b = a >>>= (const b)
I wonder if we need to toss it one of those infixr thingies to avoid
having to write these parentheses. I guess whatever makes this behave
like >> would be good
Pleasant little read. Thanks!
Begin using RIO
---------------
In the spirit of one thing at a time, this patch lays down the
infrastructure without actually using the extra verification.
> - (map drop_dotslash . list_slurpy) `fmap` (withRepository []
> slurp_pending)
> + withRepository [] $ do
> + r <- getRepository
> + s <- rIO $ slurp_pending r
> + return $ (map drop_dotslash . list_slurpy) s
I wonder if something like a withRepositoryRIO could have helped us
here, and in the rest of the code. Oh well :-)
> hunk ./src/Darcs/Commands/Add.lhs 35
> hunk ./src/Darcs/Commands/Add.lhs 106
Snip a whole bunch of indentation hunks. Might have been a good
opportunity to sneak in any trailing whitespace cleanups that may have
been needed. Also for other reviewers, this is the kind of thing that
graphical diff tools excel at
> +withGutsOf :: Repository p C(r u r) -> RIO p C(r u r t) ()
> + -> IO ()
I'm guessing this is the focus of this patch and that the rest of it is
just the consequences
updates to Darcs.Match to support type witness refactor of various commands
---------------------------------------------------------------------------
> + case get_patches_beyond_tag (n2pia pinfo) repo of
Out of curiosity, is there any difference between pia and piap?
rename get_patches_beyond_tag and add more typeful version
----------------------------------------------------------
> - :>: reverseRL (concatRL $ unsafeUnflippedseal $
> get_patches_beyond_tag chtg patches)
> + :>: reverseRL (concatRL $ unsafeUnflippedseal $
> get_patches_beyond_taginfo chtg patches)
Hmm. Would a darcs replace not have been practical or sensible here?
> -get_patches_beyond_tag :: RepoPatch p => PatchInfo -> PatchSet p C(x) ->
> FlippedSeal (RL (RL (PatchInfoAnd p))) C(x)
> -get_patches_beyond_tag t ((hp:<:NilRL):<:_) | info hp == t = flipSeal $
> NilRL :<: NilRL
> +get_patches_beyond_tag :: RepoPatch p => (PatchInfoAnd p) C(x y) -> PatchSet
> p C(z) -> (RL (RL (PatchInfoAnd p))) C(y z)
> +get_patches_beyond_tag t ((hp:<:NilRL):<:_) | IsEq <- sloppyIdentity t
> + , IsEq <- sloppyIdentity hp
> + , info hp == info t =
> unsafeCoerceP $ NilRL :<: NilRL
No comments on Jason's code itself. I'm guessing it does exactly the
same thing but without hiding quite so many types.
By the way, this is another useful thing that darcs hackers can do for
themselves: take a patch as an opportunity to try and understand some
pre-existing bit of darcs functionality and explain it to the rest of
us. Ideally, such efforts should result in darcs code documentation.
My understanding of this function (and Jason's more typeful version) is
that it retrieves the patches that come "chronologically after" a tag.
We just walk down the entire patchset, returning each patch in sequence
/until/ we hit the tag in question. This is the case below
> get_patches_beyond_tag t patchset@((hp:<:ps):<:pps) =
> hunk ./src/Darcs/Patch/Depends.lhs 328
> + if info hp == info t
> + then if get_tags_right patchset == [info hp]
> + then unsafeCoerceP $ NilRL :<: NilRL -- special case to avoid
> looking at redundant patches
> + else case get_extra_old [info t] (concatRL patchset) of
> + FlippedSeal x -> unsafeCoerceP $ x :<: NilRL -- why does this
> make sense?
> + else hp `prepend` get_patches_beyond_tag t (ps:<:pps)
At this point, most of the patches in the set are now "chronologically
before" the tag... but not necessarily all of them. There may be a
handful of cases, where for example, I had unrelated patches in my
repository before I pulled your tag. My patches come "chronologically
after" your tag; but in the repoistory your tag just so happens to sit
on top of them. This is why we use get_extra to commute them out and
also return them.
I'm not sure if this makes sense, but it's my version of the
get_patches_beyond_tags story. If this sounds right, it may be good to
write some haddocks :-)
> + prepend :: (PatchInfoAnd p) C(a z) -> (RL (RL (PatchInfoAnd p))) C(y a) ->
> (RL (RL (PatchInfoAnd p))) C(y z)
> + prepend pp NilRL = (pp:<:NilRL) :<: NilRL
> + prepend pp (p:<:ps') = (pp:<:p) :<: ps'
Again a comment on darcs and not on Jason's code. I wonder if there is
something more efficient than (p `prepend` next) that we ought to be
doing here.
--
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9
pgpuh7c72hNEn.pgp
Description: PGP signature
_______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
