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

[David Roundy]

On Wed, Aug 27, 2008 at 2:12 PM, Jason Dagit <dagit at codersbase.com> wrote:
> On Wed, Aug 27, 2008 at 10:35 AM, David Roundy <droundy at darcs.net> wrote:
>> On Wed, Aug 27, 2008 at 09:56:32AM -0700, Jason Dagit wrote:
>>> David,
>>>
>>> This is the resend you requested.
>>>
>>> I can't be sure that the individal patches below will compile by themselves, but
>>> I know they result in a working darcs when applied together.  Hopefully,
>>> sending like this will make it easier for you to review them!
>>
>> I know it's a lot of work, but breaking these into managable chunks
>> that *will* compile may be the only way to get this actually
>> reviewed.  And this really is important, because I'm pretty confident
>> that in a change of this magnitude, there *will* be bugs present.  And
>> the type witness don't really help us in this, because most of the
>> code involved uses functions with wrong type witnesses--which is to
>> say, the functions in Darcs.Repository, which cannot be written with
>> correct type witnesses until we've got something that can handle
>> mutable state.  Which means that the code need to be reviewed, and
>> therefore needs to be presented in a comprehensible format.
>
> Even if I split the changes into ones that compile for individual
> files I'm pretty sure you need to review them in compilation order so
> you can check that they are valid if you're doing it one file at a
> time.

Yes, that's what I've been doing, reordering your changes into chunks
that I can compile (and check).  It would be easier if you did this for me,
so that I wouldn't have to go searching for patches that I need.  e.g.
you moved a function from one module to another in two patches:

Wed Aug 27 12:54:45 EDT 2008  Jason Dagit <dagit at codersbase.com>
  * updates to Darcs.Patch.Unit for type witnesses
    hunk ./src/Darcs/Patch/Unit.lhs 307
    -unsafeUnseal2 :: Sealed2 p -> p
    -unsafeUnseal2 (Sealed2 p) = p
    -

and

 Wed Aug 27 12:53:57 EDT 2008  Jason Dagit <dagit at codersbase.com>
  * updates to Sealed.lhs to support type witness refactor in commands
    hunk ./src/Darcs/Sealed.lhs 23
    -                      unsafeUnseal, unsafeUnflippedseal,
    +                      unsafeUnseal, unsafeUnflippedseal, unsafeUnseal2,
    hunk ./src/Darcs/Sealed.lhs 57
    +
    +unsafeUnseal2 :: Sealed2 a -> a
    +unsafeUnseal2 (Sealed2 a) = a

This is obviously just one change, although you recorded separately.  I had
to figure this out for myself.  You could have done so, but didn't.  This takes
time, but it probably takes me more time than it took you, since you have
some memory of actually making these changes.

> Would it be easier if I gave you a URL to my repository so you could
> pull from it instead?

No, that wouldn't really help.

> Also, so are you saying your strategy is going to be, apply them
> incrementally?  There should only be a few places that really need
> review.  I've inserted a few unsafeCoerceP and documented why I think
> it's there (to assist the review process).  Most of the changes were
> of the tedious variety once Repository.Internal was updated again.

Of course, I'll need to apply them incrementally, or I won't be able to check
them for correctness.  It needs to be me and the compiler together
checking that you haven't introduced any regressions.

> Also, I seem unable to get in sync with what you've accepted recently.
>  Some patches are not appearing at http://darcs.net.

They always take a while, since the tests need to run.  And when the tests
fail, I don't always let you know, but sometimes I just try to figure out what
went wrong (as in the above-mentioned pair of patches).  Right now,
http://darcs.net is in sync with what I've got (since I've done no darcs work
today).

David