[darcs-users] darcs patch: Add Repository IO monad, RIO. (and 1 more)

Jason Dagit dagit at codersbase.com
Wed Sep 3 16:43:16 UTC 2008


On Wed, Sep 3, 2008 at 3:40 AM, David Roundy <droundy at darcs.net> wrote:
> On Tue, Sep 02, 2008 at 04:31:52PM -0700, Jason Dagit wrote:
>> David,
>>
>> I apologize for throwing such large patches your way but I don't think
>> I can break them down anymore than this.  Also, most of the hunks are
>> just changes to indentation.  I've tried to be the least invasive I can
>> be.  Which brings me to another point, this is just the first pass at
>> using RIO.  Eventually we could replace 'Repository p' in functions
>> with RIO and not even export getRepository.
>
> Yes, that would be nice (eventually).
>
> Note that you can often avoid changes to indentation by schemes like
>
> - do foo
> -    bar
> + rIO $
> + do foo
> +    bar

I might be wrong, but I thought I tried that and found that everything
under rIO had to be indented.

>> Tue Sep  2 16:23:31 PDT 2008  Jason Dagit <dagit at codersbase.com>
>>   * Add Repository IO monad, RIO.
>>
>> Tue Sep  2 16:24:58 PDT 2008  Jason Dagit <dagit at codersbase.com>
>>   * Begin using RIO
>
> Note that I'm not commenting on the good stuff, which is most of this
> code.
>
>> +instance Functor (RIO p C(r u t t)) where
>> +    fmap f m = RIO $ \r -> fmap f (unsafeUnRIO m r)
>
> I think we could safely make this
>
> instance Functor (RIO p C(r u t t1)) where
>    fmap f m = RIO $ \r -> fmap f (unsafeUnRIO m r)

Okay.  I'm not sure if I should amend record or send a new patch.  The
latter approach sounds safer since I'm not sure if you've applied yet.

>> +-- | Similar to the @ask@ function of the MonadReader class.
>> +-- This allows actions in the RIO monad to get the current
>> +-- repository.
>> +-- FIXME: Don't export this.  If we don't export this
>> +-- it makes it harder for arbitrary IO actions to access
>> +-- the repository and hence our code is easier to audit.
>> +getRepository :: RIO p C(r u t t) (Repository p C(r u t))
>> +getRepository = RIO return
>
> Yay for this fixme.

And haddocks too! :)

> So this is your general plan, to leave most of the code in IO and only
> gradually convert? (Sounds reasonable)

Right, baby steps.

>> -add_cmd opts args = withRepoLock opts $- \repository ->
>> - do cur <- slurp_pending repository
>> +add_cmd opts args = withRepoLock opts $ do
>> + repository <- getRepository
>> + rIO $ do
>> +    cur <- slurp_pending repository
>>      origfiles <- map toFilePath `fmap` fixSubPaths opts args
>
> Do you know why we don't seem any longer to need the $- operator? Is this
> something that's going to bite our ghc 6.4 users?

It's possible.  It looks like ($) became impredicative in 6.6, but I
couldn't tell you by looking at the code if we're relying on that.
Are we still actively supporting 6.4?  Haven't the major linux
distributions moved on to 6.6 now?  It looks like 6.6 was released a
full 2 years ago (the last release of the 6.4 series was in oct of
2006).  Also, unless someone is testing our builds on 6.4 it seems bad
to claim support for 6.4.  And finally, I think I've removed other
uses of ($-) and no one has complained yet?

> I wonder if we could avoid the excessive indentation here by something like
>
> withRepoLock opts $ getRepository >>= $ \repository ->
>    do files <- sort `fmap` fixSubPaths opts args
>
> I'm not sure of precedence...

I recall trying this because I discovered ">>= $" is a parse error,
but I don't recall why I didn't do it this way.  I think it was
because then I had to lift all the IO actions into RIO.

> I guess that's all.  I only paged throught the rest of the changes, as it
> looked like more of the same (and with all the indentation, it would be
> hard to spot any real changes).

Yeah, we need a diff option that can ignore leading whitespace to
really look at the hunks.  On the other hand, I was trying not to edit
anything that didn't create a type error.

> When I get to work I'll see how cleanly it applies with the other patches
> in the queue, and hopefully apply.

I hope it applies cleanly.  I've almost finished my amending of making
all the commands type check.  Given the amount of work it takes and
the time I've spent on it thus far, I really do not want to do it a
4th time.  It's just too much time to keep throwing away.

Thanks,
Jason


More information about the darcs-users mailing list