[darcs-users] darcs patch: partial type witnesses in Unrevert (and 10 more)

Jason Dagit dagit at codersbase.com
Wed Aug 27 16:34:05 UTC 2008


On Wed, Aug 27, 2008 at 9:17 AM, David Roundy <droundy at darcs.net> wrote:
> On Tue, Aug 26, 2008 at 10:30:41PM -0700, Jason Dagit wrote:
>> David,
>>
>> Below is a big important set of patches.  Sorry if it's a bit overwhelming.
>>
>> If you've already commented on my 'fix accidental reversal in
>> tentativelyAddToPending' patch then I missed it.  Basically, if you look at
>> my patch, 'Make Darcs.Repository.Internal compile with type witnesses.',
>> you see that the hunk around line 577 swaps the order of concatenation.  I've
>> fixed it, but the patch doesn't seem to be in darcs.net yet that's why I'm
>> resending.
>>
>> That's the most critical patch in the bunch.  The rest do some refactoring
>> and commenting, and confilct resolving, as well as adding a test case and
>> fix for the bug we found in unrevert.
>>
>> The exciting patch is 'extensive type witnesses refactor for commands'.  That's
>> a big monumental patch.  I've tried to document all my uses of unsafeCoerceP
>> as to how I undertand why they are needed.  More work to be done on the
>> type witness front, but this makes all the modules compile with type witnesses!
>> Although, I didn't record a patch to the makefile to make that so.  I'm a bit
>> tired at the moment to think of the best way to make that change.
>>
>> Also, note that I didn't bother folding view_changes in Changes into the type
>> witnesses yet.  I worked on that quite a bit, but in the end it seemed like
>> a waste of time right now.  So that means diff and changes both have some
>> unsafe bits.
>>
>> Let me know what you think!
>>
>> Thanks!
>> Jason
>>
>> Tue Aug 12 22:38:37 PDT 2008  Jason Dagit <dagit at codersbase.com>
>>   * partial type witnesses in Unrevert
>>
>> Mon Aug 25 11:32:35 PDT 2008  Jason Dagit <dagit at codersbase.com>
>>   * add double-unrevert.sh test
>>
>> Mon Aug 25 11:59:07 PDT 2008  Jason Dagit <dagit at codersbase.com>
>>   * Finish refactor of Unrevert as well as making it pass double-unrevert.sh
>
> Applied up to here!
>
>> Tue Aug 26 20:49:29 PDT 2008  Jason Dagit <dagit at codersbase.com>
>>   * extensive type witnesses refactor for commands
>>
>> Mon Aug 25 17:36:05 PDT 2008  Jason Dagit <dagit at codersbase.com>
>>   * fix accidental reversal in tentativelyAddToPending
>>
>> Tue Aug 26 20:57:03 PDT 2008  Jason Dagit <dagit at codersbase.com>
>>   * Fix conflict in Internal
>>
>> Sun Aug 24 18:18:10 PDT 2008  Jason Dagit <dagit at codersbase.com>
>>   * improve reporting for bug in get_extra
>>
>> Tue Aug 26 21:13:21 PDT 2008  Jason Dagit <dagit at codersbase.com>
>>   * fix conflicts with get_extra changes
>>
>> Tue Aug 26 21:13:44 PDT 2008  Jason Dagit <dagit at codersbase.com>
>>   * fix minor type witness compile error with new commuteFL
>>
>> Tue Aug 26 21:40:25 PDT 2008  Jason Dagit <dagit at codersbase.com>
>>   * fix error in Properties due to new commuteFL
>>
>> Tue Aug 26 22:00:06 PDT 2008  Jason Dagit <dagit at codersbase.com>
>>   * fix minor warnings in Changes.lhs when using type witnesses
>
> Any chance you could simply unrecord all of these and then record them
> in smaller chunks? Every file in the big patch is involved in a
> conflict, which means I can't usefully apply less than the entire
> batch, so we may as well group them together in such a way that I can
> review them individually.

Sure, as long as you don't mind that the individual resulting patches
may not compile.  One reason why it's a big lump is because I wanted a
patch that would compile for sure and I hate running the test suite
once for each file in that patch (just running the tests that many
times would take several hours).

> It would also be great if you could darcs send with a more recent
> build of darcs, so I can see the context of these changes.  Or perhaps
> there's a bug in the most recent darcs that I'm not aware of, which
> breaks the showing of context?

Weird.  As of when I sent it I was completely up to date with
darcs.net.  But maybe you've pushed something since last night?  I can
see what happens locally when I pull again.

>
>> hunk ./src/Darcs/Commands/Add.lhs 134
>> -addp :: AddMessages -> [DarcsFlag] -> String -> Slurpy -> [FilePath] -> IO (FL Prim)
>> +maybeToFL :: Maybe (p C(x y)) -> FL p C(x y)
>> +maybeToFL Nothing = unsafeCoerceP NilFL
>> +maybeToFL (Just x) = x:>:NilFL
>
> Note that this maybeToFL is equivalent to unsafeCoerceP in terms of
> unsafety.  But I'm sure you're aware of that.

Well, no I'm not aware of that.  On the other hand, I defined that
function local to Add.lhs and didn't export it because I guessed
(correctly) that it would make you nervous.  On the other hand, where
it's used in addp, we need something that can take freshly constructed
patches and build a sequence.  Here we're trusting that addp
constructs the patches in a valid sequence.  I'm not really sure what
else to do about it.  It's sort of like reading patch files where we
just have to put them in a sequence and run with it.  And it's also a
drop in replacement for maybeToList.

But, getting back to the unsafeCoerceP issue.  Is your concern the
Nothing case?  The other case looks quite normal to me.  Note that,
due to the way GADTs type check, the unsafeCoerceP is the first case
is needed.  The only other option I see is inserting the body of
maybeToFL into the already huge foldr in addp.  A solution I would
like to avoiding for maintenance issues.  You'll notice I added the
type signatures in that foldr block.  That's because I spent an entire
morning one day just trying to figure out how that foldr works.  I was
trying not to make the situation any worse :)

> That's as far as I got in reviewing things.

Thanks!

Jason


More information about the darcs-users mailing list