3 patches for repository http://darcs.net/screened:

This set takes the possibly slightly controversial step of removing
the explicit Identity from Prim. The rationale is that

(a) it's not actually being used except in tests, which suggests we
    shouldn't have it
(b) it meant that there were redundant ways of expressing
    "doing nothing" - e.g. NilFL and Identity :>: NilFL
(c) Now that Split and ComP are gone, it makes sense for Prim to
    represent precisely one primitive change, and Identity can be
    seen as zero primitive changes. We can always use NilFL as an
    identity at the FL Prim level.

Mon Nov 22 06:43:08 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * drop identity member of Invert

Mon Nov 22 06:43:12 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * move MyEq superclass from Invert to Patchy

Mon Nov 22 06:43:15 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * get rid of Identity constructor from Prim

Attachments

• drop-identity-member-of-invert.dpatch
• unnamed

This patch bundle (with 3 patches) was just applied to the repository http://darcs.net/screened.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_screened.html#bundle-3ea246f03dc7c7306812aa1c73bee99d17f86664

On Tue, Nov 23, 2010 at 23:12:55 +0000, Ganesh Sittampalam wrote:
> This set takes the possibly slightly controversial step of removing
> the explicit Identity from Prim. The rationale is that
> 
> (a) it's not actually being used except in tests, which suggests we
>     shouldn't have it

This sounds convincing enough.

So I guess the my main objective as a reviewer is to confirm that it
actually isn't being used... I think I have a story below that says so

I haven't pushed the patches yet though, partly because I ran into
trouble getting this to build (probably something on my end) and ran
out of time.  Feel free to do it yourself if I seem to have
understood enough of what's going on...

> (b) it meant that there were redundant ways of expressing
>     "doing nothing" - e.g. NilFL and Identity :>: NilFL
> (c) Now that Split and ComP are gone, it makes sense for Prim to
>     represent precisely one primitive change, and Identity can be
>     seen as zero primitive changes. We can always use NilFL as an
>     identity at the FL Prim level.

As somebody who hopes to understand more of Darcs.Patch.*, I would
certainly be happier with the simpler story of Prim just being one
change.

> Mon Nov 22 06:43:08 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
>   * drop identity member of Invert
> 
> Mon Nov 22 06:43:12 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
>   * move MyEq superclass from Invert to Patchy
> 
> Mon Nov 22 06:43:15 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
>   * get rid of Identity constructor from Prim

Let's have an initial look.  I'll try to preface the more potentially
useful bits of commentary with QUESTION.

This is an example of recent refactoring work.  What would advocates for
greater testing have us do here?  I'm pretty happy to accept that we
have some battery of unit tests running over Darcs.Patch and that this
act of getting rid of unused code needs no further evidence besides
thinking and review. Am I wrong?

drop identity member of Invert
------------------------------
> hunk ./src/Darcs/Patch/Invert.hs 13
>  
>  class MyEq p => Invert p where
>      invert :: p C(x y) -> p C(y x)
> -    identity :: p C(x x)
> -    sloppyIdentity :: p C(x y) -> EqCheck C(x y)
> -    sloppyIdentity p = identity =\/= p

Meat.

'identity' (maybe we need a convention to refer to functions to avoid
collisions with English) allowed Invert patch type to supply its variant
of an identity patch. For example, identity for lists of patches would
just be the empty list.

'sloppyIdentity' checks that a patch is equal (according to that
patches's instance of MyEq) to that 'identity' and unifies your
witnesses if so.

I asked myself if it really safe to get rid of sloppyIdentity and
decided that it would be if sloppyIdentity p == True implies that p
was 'identity'... since all the definitions of sloppyIdentity above
seem to boil down to that, I guess it is.

> hunk ./src/Darcs/Patch/Prim/Core.lhs 170
>      invert (DP d AddDir) = DP d RmDir
>      invert (Move f f') = Move f' f
>      invert (ChangePref p f t) = ChangePref p t f
> -    identity = Identity
> -    sloppyIdentity Identity = IsEq
> -    sloppyIdentity _ = NotEq

This is the main instance we want to worry about.
Since Prim isn't built out of anything, it needs Identity to be able
to define this (well, it could have used ComP [] maybe?, which I'm
glad is going away...).  Getting rid of 'identity' lets us get rid of
'Identity'

> hunk ./src/Darcs/Patch/PatchInfoAnd.hs 170
> -    identity = PIAP idpatchinfo (actually identity)
>      invert (PIAP i p) = PIAP i (invert `fmapH` p)

>  instance (Invert p, Commute p) => Invert (FL p) where
>      invert = reverseRL . invertFL
> -    identity = NilFL
> -    sloppyIdentity NilFL = IsEq
> -    sloppyIdentity (x:>:xs) | IsEq <- sloppyIdentity x = sloppyIdentity xs
> -    sloppyIdentity _ = NotEq

> hunk ./src/Darcs/Patch/Permutations.hs 240
>  instance (Commute p, Invert p) => Invert (RL p) where
>      invert = reverseFL . invertRL
> -    identity = NilRL
> -    sloppyIdentity NilRL = IsEq
> -    sloppyIdentity (x:<:xs) | IsEq <- sloppyIdentity x = sloppyIdentity xs
> -    sloppyIdentity _ = NotEq

Key instances to keep in mind.

> hunk ./src/Darcs/Patch/V1/Commute.lhs 682
> -    identity = PP identity

> hunk ./src/Darcs/Patch/V2/Real.hs 462
> -    identity = Normal identity

> hunk ./src/Darcs/Patch/Choices.hs 69
>  instance Invert p => Invert (TaggedPatch p) where
>      invert = liftTP invert
> -    identity = TP (TG Nothing (-1)) identity

Sometimes the instances involve some making stuff up, here taking the
convention that Patch.Choices tags for identity are -1.

> hunk ./src/Darcs/Patch/Named.lhs 40
>      invert (NamedP n d p)  = NamedP (invertName n) (map invertName d) (invert p)
> -    identity = NamedP idpatchinfo [] identity

Looks like Darcs.Patch.Info.idpatchinfo is now hanging by a thread.
Perhaps we'll be able to kill it off once and for all with Petr's
annotate work from adventure?


> hunk ./src/Darcs/Commands/Changes.lhs 130
> -                  `catch` \_ -> return (Sealed identity) -- this is triggered when repository is remote
> +                  `catch` \_ -> return (Sealed NilFL) -- this is triggered when repository is remote

> hunk ./src/Darcs/Commands/Tag.lhs 92
> -    let mypatch = infopatch myinfo identity
> +    let mypatch = infopatch myinfo NilFL

Just replacing 'identity' with its implementation.

> -nullP :: Prim C(x y) -> EqCheck C(x y)
> -nullP = sloppyIdentity

Seems like a useless synonym in any case, maybe just there for concision

>  sortCoalesceFL2 :: FL Prim C(x y) -> FL Prim C(x y)
>  sortCoalesceFL2 NilFL = NilFL
> -sortCoalesceFL2 (x:>:xs) | IsEq <- nullP x = sortCoalesceFL2 xs
>  sortCoalesceFL2 (x:>:xs) | IsEq <- isIdentity x = sortCoalesceFL2 xs
>  sortCoalesceFL2 (x:>:xs) = either id id $ pushCoalescePatch x $ sortCoalesceFL2 xs

This particular case seems useless anyway.  Wouldn't isIdentity be true
for Identity anyway?

> hunk ./src/Darcs/Patch/Prim/Core.lhs 510
> -      Just new' | IsEq <- nullP new' -> Right ps'
> -                | otherwise -> Right $ either id id $ pushCoalescePatch new' ps'
> +      Just Identity -> Right ps'
> +      Just new' -> Right $ either id id $ pushCoalescePatch new' ps'

>  instance Effect Prim where
> -    effect p | IsEq <- sloppyIdentity p = NilFL
> -             | otherwise = p :>: NilFL
> -    effectRL p | IsEq <- sloppyIdentity p = NilRL
> -               | otherwise = p :<: NilRL
> +    effect Identity = NilFL
> +    effect p = p :>: NilFL
> +    effectRL Identity = NilRL
> +    effectRL p = p :<: NilRL

> hunk ./src/Darcs/Patch/V1/Commute.lhs 636
> -    where fake_p = Merger identity NilRL p1 p2
> +    where fake_p = Merger NilFL NilRL p1 p2

> hunk ./src/Darcs/Patch/V1/Commute.lhs 638
> -          p = Merger identity unwindings p1 p2
> +          p = Merger NilFL unwindings p1 p2

Just code substitution.

>  showContextSeries :: (Apply p, ShowPatch p, Effect p) => FL p C(x y) -> TreeIO Doc
> -showContextSeries patches = scs identity patches
> -    where scs :: (Apply p, ShowPatch p, Effect p) => Prim C(w x) -> FL p C(x y) -> TreeIO Doc
> +showContextSeries patches = scs Nothing patches
> +    where scs :: (Apply p, ShowPatch p, Effect p) => Maybe (Prim C(w x)) -> FL p C(x y) -> TreeIO Doc
>            scs pold (p:>:ps) = do

> hunk ./src/Darcs/Patch/Viewing.hs 74
> -                  NilFL -> coolContextHunk pold hp identity
> -                  (p2:>:_) ->
> -                      case isHunk p2 of
> -                      Nothing -> do a <- coolContextHunk pold hp identity
> -                                    b <- liftIO $ virtualTreeIO (scs hp ps) s'
> -                                    return $ a $$ fst b
> -                      Just hp2 -> do a <- coolContextHunk pold hp hp2
> -                                     b <- liftIO $ virtualTreeIO (scs hp ps) s'
> -                                     return $ a $$ fst b
> +                  NilFL -> coolContextHunk pold hp Nothing
> +                  (p2:>:_) -> do a <- coolContextHunk pold hp (isHunk p2)
> +                                 b <- liftIO $ virtualTreeIO (scs (Just hp) ps) s'
> +                                 return $ a $$ fst b

We get a bit of code simplification out of this, two basically identical
branches that collapse down if you observe that coolContextHunk already
pays attention to the distinction between hunk and everything else.

> -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.

> hunk ./src/Darcs/Test/Patch.hs 463
> -                      unsafeCoerceP identity),
> +                      unsafeCoerceP NilFL),

> -\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?

> -joinInverses :: (FORALL(x y) (Prim :> Prim) C(x y) -> Maybe (Prim C(x y)))
> -              -> Prim C(a b) -> Maybe Doc
> -joinInverses j p = case j (invert p :> p) of
> -                    Just Identity -> Nothing
> -                    Just p' -> Just $ redText "joinInverses gave just" $$ showPatch p'
> -                    Nothing -> Just $ redText "joinInverses failed"

Test seems like it only does something interesting with Identity.
I *think* this probably belongs in the third patch instead of this
one, but not a big deal if I'm right, of course

>  joinCommute :: (FORALL(x y) (Prim :> Prim) C(x y) -> Maybe (Prim C(x y)))
>  joinCommute j (a :> b :> c) =
> hunk ./src/Darcs/Test/Patch/QuickCheck.hs 278
> -                           make_identity_identity p | IsEq <- isIdentity p = identity
> +                           make_identity_identity p | IsEq <- isIdentity p = Identity

> hunk ./src/Darcs/Test/Patch/QuickCheck.hs 538
> -              last_triple NilFL = seal2 $ identity :> identity :> identity
> -              last_triple (a :>: NilFL) = seal2 $ a :> invert a :> identity
> +              last_triple NilFL = seal2 $ Identity :> Identity :> Identity
> +              last_triple (a :>: NilFL) = seal2 $ a :> invert a :> Identity

Just code substitution

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

> -class MyEq p => Invert p where
> +class Invert p where

> +import Darcs.Witnesses.Eq ( MyEq )
>  
> hunk ./src/Darcs/Patch/Patchy.hs 37
> -class (Apply p, Commute p, PatchInspect p, ShowPatch p, ReadPatch p, Invert p) => Patchy p
> +class (MyEq p, Apply p, Commute p, PatchInspect p, ShowPatch p, ReadPatch p, Invert p) => Patchy p
  
> -class (Invert p, Commute p, Effect p) => Conflict p where
> +class (MyEq p, Invert p, Commute p, Effect p) => Conflict p where

> -doesCommute :: (Invert p, Commute p, Check p) => p C(x y) -> p C(y z) -> Bool
> +doesCommute :: (MyEq p, Invert p, Commute p, Check p) => p C(x y) -> p C(y z) -> Bool
>  doesCommute p1 p2 =

get rid of Identity constructor from Prim
-----------------------------------------
[snipped a lot of straightforward looking removals]

> hunk ./src/Darcs/Patch/Prim/Core.lhs 445
>  tryOne _ _ NilFL = Nothing
>  tryOne sofar p (p1:>:ps) =
>      case coalesce (p1 :< p) of
> -    Just p' -> Just (reverseRL sofar +>+ p':>:NilFL +>+ ps)
> +    Just p' -> Just (reverseRL sofar +>+ p' +>+ ps)

Huh? I guess the cons was useless anyway...

> 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?

> -coalesce (Identity :< p) = Just p
> -coalesce (p :< Identity) = Just p

This goes away

> -coalesce (Move a b :< Move b' a') | a == a' = Just $ Move b' b
> -coalesce (Move a b :< FP f AddFile) | f == a = Just $ FP b AddFile
> -coalesce (Move a b :< DP f AddDir) | f == a = Just $ DP b AddDir
> -coalesce (FP f RmFile :< Move a b) | b == f = Just $ FP a RmFile
> -coalesce (DP f RmDir :< Move a b) | b == f = Just $ DP a RmDir
> -coalesce (ChangePref p f1 t1 :< ChangePref p2 f2 t2) | p == p2 && t2 == f1 = Just $ ChangePref p f2 t1
> +coalesce (FP f1 p1 :< FP _ p2) = fmap (:>: NilFL) $ coalesceFilePrim f1 (p1 :< p2) -- f1 = f2
> +coalesce (Move a b :< Move b' a') | a == a' = Just $ Move b' b :>: NilFL
> +coalesce (Move a b :< FP f AddFile) | f == a = Just $ FP b AddFile :>: NilFL
> +coalesce (Move a b :< DP f AddDir) | f == a = Just $ DP b AddDir :>: NilFL
> +coalesce (FP f RmFile :< Move a b) | b == f = Just $ FP a RmFile :>: NilFL
> +coalesce (DP f RmDir :< Move a b) | b == f = Just $ DP a RmDir :>: NilFL
> +coalesce (ChangePref p f1 t1 :< ChangePref p2 f2 t2) | p == p2 && t2 == f1 = Just $ ChangePref p f2 t1 :>: NilFL

The rest of these are just straightforwardly making the non p == inverse p
case Just

>  coalesce _ = Nothing
>  
> hunk ./src/Darcs/Patch/Prim/Core.lhs 704
> -join :: (Prim :> Prim) C(x y) -> Maybe (Prim C(x y))
> +join :: (Prim :> Prim) C(x y) -> Maybe (FL Prim C(x y))
>  join (x :> y) = coalesce (y :< x)

>  -- | 'comparePrim' @p1 p2@ is used to provide an arbitrary ordering between
>  --   @p1@ and @p2@.  Basically, identical patches are equal and
> ---   @Move < DP < FP < Identity < ChangePref@.
> +--   @Move < DP < FP < ChangePref@.
>  --   Everything else is compared in dictionary order of its arguments.

For a bit I wondered if I should be concerned about Ordering of Prim
containers (eg. if ordering of some [Identity,foo]
due to it becoming [foo]), but I guess being concerned about this sort
of thing would only make sense if Identity really was being used in the
code.

> 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?

> hunk ./src/Darcs/Patch/V1/Commute.lhs 591
> -  resolveConflicts patch = rcs NilFL $ reverseFL $ flattenFL patch
> +  resolveConflicts patch = rcs NilFL (patch :<: NilRL)
>  
> --- TODO this is a relic from the days in which Patch had a ComP constructor
> --- for nesting lists. It is likely completely useless now but is still used
> --- in a couple of places which need to be checked before removig it.
> -{- INLINE flattenFL -}
> -flattenFL :: Patch C(x y) -> FL Patch C(x y)
> -flattenFL (PP Identity) = NilFL
> -flattenFL p = p :>: NilFL

Always nice to get rid of this sort of "first we put the thing in the
box, then we take it out of the box" code.

>  show_read :: (Show2 p, Patchy p) => p C(a b) -> Maybe Doc
> hunk ./src/Darcs/Test/Patch/QuickCheck.hs 278
> -                           make_identity_identity p | IsEq <- isIdentity p = Identity
> +                           make_identity_identity p | IsEq <- isIdentity p = localIdentity

> +-- a hack introduced after Identity was removed from Prim
> +localIdentity :: Prim C(x x)
> +localIdentity = let fn = fp2fn "./dummy" in Move fn fn

> -smartFP _ (Hunk _ [] []) | avoidEmptyHunks = unsafeCoerceP Identity
> +smartFP _ (Hunk _ [] []) | avoidEmptyHunks = unsafeCoerceP localIdentity

Bits of test where we test not with literally Identity but some
do-nothing patch instead.

I didn't really look too carefully at the rest of this code, which
removes Identity-related tests.

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, try +44 (0)1273 64 2905 or
xmpp:kowey@jabber.fr (Jabber or Google Talk only)

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

Hi,

Looks like we're good to go, except for dependencies.

I've reinstalled GHC on my end (Cabal trouble, messed up package DB from
unknown/past shenanigans?) and now got this to build and pass tests.

I'm going to abuse accepted-pending-tests to mark that this is ready.
It looks like we have the following dependencies:

Sun Oct 17 16:41:31 BST 2010  Florent Becker <florent.becker@ens-lyon.org>
  * patch426 - resolve issue114: allow hunk-splitting in revert
  
Mon Nov 22 06:42:50 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * patch479 - split a basic class out of ShowPatch that requires fewer deps
  * patch481 - disentangle Darcs.Patch.Named from Darcs.Patch.Viewing
  * patch481 - break out Summary code into separate module
  * patch482 - get rid of Split

This might actually be not much worse than the previous situation where we
just had lots of duplication between patch bundles that people send.  Hmm!
Anyway patch426 looks like the one to unstick because patch479 also depends
on it.  Sorry for the logjam.

On Wed, Dec 29, 2010 at 00:18:12 +0000, Ganesh Sittampalam wrote:
> >>-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 _ _))
> >
> >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.

Hmm, I can't recall what I meant.  Perhaps I thought that showContextHunk
actually meant identity and that we were using Nothing here as a stand-in.
But maybe I was looking at it backwards, ie. that the original code was
actually just abusing identity to mean Nothing.

Truth be told, I'm not entirely clear on how coolContextHunk is using the
idea of previous/next patch to change what it displays.  I only get the
vague impression that it's somehow sensible for the amount of context we
show to be related to the "previous" and "next" patches

> >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.

Nice. I'm definitely liking the drive for clarity.

> >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.

> 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.

Sounds good to me!

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, try +44 (0)1273 64 2905 or
xmpp:kowey@jabber.fr (Jabber or Google Talk only)

Pushing as part of a simultaneous patch481-patch484
push-and-compile-and-test (previous review means it's ok to push).

This patch bundle (with 3 patches) was just applied to the repository http://darcs.net/.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-3ea246f03dc7c7306812aa1c73bee99d17f86664

This patch bundle (with 3 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-3ea246f03dc7c7306812aa1c73bee99d17f86664