[darcs-devel] [patch483] drop identity member of Invert (and 2 more)

[Ganesh Sittampalam]

Hi Eric,

Thanks as always for the careful review.

On Fri, 24 Dec 2010, Eric Kow wrote:

>> -showContextHunk h = coolContextHunk identity h identity
>> +showContextHunk h = coolContextHunk Nothing h Nothing
>
>> hunk ./src/Darcs/Patch/Viewing.hs 84
>> -coolContextHunk :: Prim C(a b) -> Prim C(b c) -> Prim C(c d) -> TreeIO Doc
>> +coolContextHunk :: Maybe (Prim C(a b)) -> Prim C(b c) -> Maybe (Prim C(c d)) -> TreeIO Doc
>> hunk ./src/Darcs/Patch/Viewing.hs 93
>> -                     (FP f' (Hunk lprev _ nprev))
>> +                     Just (FP f' (Hunk lprev _ nprev))
>> hunk ./src/Darcs/Patch/Viewing.hs 101
>> -                      (FP f' (Hunk lnext _ _))
>> +                      Just (FP f' (Hunk lnext _ _))
>
> Just as before the identity case gets swept up with _
>
> QUESTION: Does this still need to be Maybe after Identity goes away?
> I imagine not.

I think it does - Nothing is used e.g. from showContextHunk.

Perhaps I'm misunderstanding what you mean.


>> hunk ./src/Darcs/Test/Patch.hs 463

>> -\begin{prp}[Identity commutes trivially]
>> -  The identity patch must commute with any patch without modifying said patch.
>> -\end{prp}
>
>> -\begin{prp}[Inverse doesn't commute]
>> -  A patch and its inverse will always commute, unless that patch is an
>> -  identity patch (or an identity-like patch that has no effect).
>> -\end{prp}
>
> QUESTION: are these properties we ought to be testing for anyway using
> a different kind of do-nothing patch?

Perhaps we ought to test "Identity commutes trivially" for NilFL/NilRL 
etc, but honestly I think that's obvious from the code anyway.

I thought about "Inverse doesn't commute" a bit, but I think the law is 
actually not a useful one anyway. (I think the text above should say "a 
patch and its inverse will never commute"). I can imagine more 
sophisticated patch types where a patch and its inverse would actually 
commute - for example suppose we maintain a counter and have a patch that 
adds 1 to the counter and one that subtracts 1 from it. Those would be 
inverses and should also commute.


> move MyEq superclass from Invert to Patchy
> ------------------------------------------
> QUESTION: What's the motivation behind this patch?
> It seems pretty harmless in any case.

Ah - I should have been more explicit about my thinking here :-)

I'm actually on a bit of a mission to minimise use of MyEq. The reason is 
that it's not actually obvious from its name what it actually does. There 
are various possible ways of defining equality over patches - for example 
syntactic equality ("are these two patches exactly the same data 
values?"), equality up to commutation ("can we make one patch the same as 
the other by commuting the internals?"). It's actually the case that MyEq 
implements the latter, and my suspicion is that it's used in cases where 
this isn't actually what was wanted.

So, I'd like to cut down its use and then perhaps make two or more 
different classes to express the different kinds of equality more 
precisely. From that point of view, this change is just a step towards 
that.

Also, I don't really see much justification for having the superclass, 
especially once identity is gone. What does general equality have to do 
with inverting patches?

>> hunk ./src/Darcs/Patch/Prim/Core.lhs 501
>>  pushCoalescePatch new NilFL = Left (new:>:NilFL)
>>  pushCoalescePatch new ps@(p:>:ps')
>>      = case coalesce (p :< new) of
>> -      Just Identity -> Right ps'
>> -      Just new' -> Right $ either id id $ pushCoalescePatch new' ps'
>> +      Just (new' :>: NilFL) -> Right $ either id id $ pushCoalescePatch new' ps'
>> +      Just NilFL -> Right ps'
>> +      Just _ -> impossible -- coalesce either returns a singleton or empty
>
>> -coalesce :: (Prim :< Prim) C(x y) -> Maybe (Prim C(x y))
>> +coalesce :: (Prim :< Prim) C(x y) -> Maybe (FL Prim C(x y))
>>  coalesce (FP f1 _ :< FP f2 _) | f1 /= f2 = Nothing
>> -coalesce (p2 :< p1) | IsEq <- p2 =\/= invert p1 = Just null_patch
>> +coalesce (p2 :< p1) | IsEq <- p2 =\/= invert p1 = Just NilFL
>
> QUESTION: Would Maybe (Maybe Prim) or a ternary type be safer/better
> than using Maybe FL?

I don't think Maybe (Maybe a) as an explicit representation type is ever 
safe, too much risk of confusion :-)

I'd like to clean up coalesce though and no doubt improvements could be 
made in this area.

>> hunk ./src/Darcs/Patch/Read.hs 136
>>       , return' $ readRmDir x
>>       , return' $ readTok x
>>       , return' $ readBinary x
>> -     , return'   readIdentity
>>       , return' $ readChangePref
>>       ]
>>    where
>> hunk ./src/Darcs/Patch/Read.hs 139
>> -  readIdentity = do string emptyBraces
>> -                    return Identity
>>
>> hunk ./src/Darcs/Patch/Read.hs 141
>> -emptyBraces :: B.ByteString
>> -emptyBraces = BC.pack "{}"
>
> QUESTION: OK this is the only thing that really gives me pause about
> this work. How do we know we aren't actually using Identity?
> After all we have the ability to read it from real patches.  I guess in
> reading this code, I didn't *see* any bits of the Darcs code where we
> were creating Identity (there was always the occasional identity, but
> those could generally be explained away as just NilFL or in some cases
> explicitly accounted by going up to Maybe Prim)... so if this patch
> reading code is actually returning something from real repositories,
> it was from old code, and it's very very very unlikely we would have
> just changed behaviour in that way in old code.
>
> QUESTION: is it possible that other bits of code would generate {} that
> we then read as Identity?  I guess if that happens we have bigger
> problems anyway?

Sorry, I should have gone into this in more detail.

Historically, when reading V1 patches, I believe {} would actually have 
been read as ComP NilFL before we even got into the Prim reading code. 
Since I've removed ComP already, instead it would just drop the patch 
completely - i.e. {} is accepted but ignored. I believe that this is a 
reasonable thing to do.

For V2 patches, {} would now be an error where it wasn't before, but I'm 
much more confident that darcs 2.0+ has never written out an Identity 
patch so I think this is ok.

Ganesh