[darcs-users] [patch159] make tentativelyRemove return the new re... (and 27 more)

Ganesh Sittampalam ganesh at earth.li
Tue Feb 23 23:49:16 UTC 2010


On Tue, 23 Feb 2010, Jason Dagit wrote:

> See below for my comments.

Thanks!

> I only made it about 40% of the way through before I had to start
> skimming.  The amount of stuff to look at is quite big.  My emacs buffer
> claims to have over 2500 lines.

I guess my hope is that the patches do stand alone reasonably well, so can 
be reviewed and applied one at a time. Please don't feel pressured into 
applying anything that you're not happy with, though.

> The introduction of the new Gap stuff is hard for me to review because
> it's new to me and I don't understand it.  That's really the only thing
> that would make me want to reject this patch.  If I understood it
> better, perhaps I wouldn't feel so uneasy about it.
>
> Maybe someone else can take a stab at it?

I think more eyes on the Gap stuff in particular would definitely be very 
helpful.

First one general comment on a recurring theme:

I believe the general change from 'recorded' to 'tentative' is a 
correctness fix, in that the witnesses were previously incorrect, but this 
wasn't picked up because the repository witnesses were not updated as 
things changed.

As you say, there's a transaction going on here. 'tentative' starts out 
the same as 'recorded', but as we modify it, all further modifications are 
also written to the tentative state, as if it were the real recorded 
state. If I recall correctly, I was simply unable to make some complicated 
multi-stage operations typecheck without this fix.

> check_unrecorded_conflicts opts _ | NoUpdateWorking `elem` opts =
> return False
> check_unrecorded_conflicts opts pc =
>     do repository <- identifyDarcs1Repository opts "."
> hunk ./src/Darcs/Repository/Internal.hs 472
>        cuc repository
>     where cuc :: Repository Patch C(r u t) -> IO Bool
> -          cuc r = do Sealed mpend <- read_pending r :: IO (Sealed (FL
> Prim C(r)))
> +          cuc r = do Sealed mpend <- read_pending r :: IO (Sealed (FL
> Prim C(t)))
>
> If the pending is to represent the current "transaction" then this
> change makes sense to me aswell.

> hunk ./src/Darcs/Repository/LowLevel.hs 38
> pendingName :: RepoType p -> String
> pendingName (DarcsRepository _ _) = darcsdir++"/patches/pending"
>
> -read_pending :: RepoPatch p => Repository p C(r u t) -> IO (Sealed (FL
> Prim C(r)))
> +read_pending :: RepoPatch p => Repository p C(r u t) -> IO (Sealed (FL
> Prim C(t)))
>
> Pending returns the tentative state.  Makes sense.
>
> read_pending (Repo r _ _ tp) =
>     withCurrentDirectory r (read_pendingfile (pendingName tp))

Hmm. Looking at the code for read_pending, this is actually fishy, because 
it refers to _darcs/patches/pending, not _darcs/patches/pending.tentative.

My witnesses work shouldn't change any behaviour, so if this really is 
wrong it isn't a new bug, but it's one that witnesses should possibly have 
caught downstream if primitive operations like this were "correct".

Reading the code some more, I'm now just confused about what's going on 
with "pending" and "pending.tentative". I think that perhaps the issue is 
that, as the comments already pointed out, we don't track pending state in 
witnesses, so it's all just a mess. I think perhaps this issue is 
something to be returned to in future - perhaps some more witnesses could 
untangle what's going on, and the cost of yet longer type signatures.

> hunk ./src/Darcs/Repository/Internal.hs 588
>
> tentativelyReplacePatches :: forall p C(r u t x). RepoPatch p =>
> Repository p C(r u t) -> [DarcsFlag]
>                           -> FL (Named p) C(x t) -> IO ()
> -tentativelyReplacePatches repository@(Repo x y z w) opts ps =
> -    -- tentativelyRemovePatches_ leaves the repository in state C(x u t)
> -    do tentativelyRemovePatches_ DontUpdatePristine repository opts ps
> -       -- Now we add the patches back so that the repo again has state
> C(r u t)
> -       sequence_ $ mapAdd ((Repo x y z w) :: Repository p C(x u t)) ps
> -  where mapAdd :: Repository p C(i l m) -> FL (Named p) C(i j) -> [IO ()]
> +tentativelyReplacePatches repository opts ps =
> +    do repository' <- tentativelyRemovePatches_ DontUpdatePristine
> repository opts ps
> +       sequence_ $ mapAdd repository' ps
> +  where mapAdd :: Repository p C(m l i) -> FL (Named p) C(i j) -> [IO ()]
>
> The reordering of witnesses (i l m) to (m l i) here looks suspicious.
> What is the reason?

The old code matched the recorded state against the initial state of the 
FL. The new code does the same with the tentative state, and otherwise 
leaves the witnesses unchanged.

Happy to use other names instead, but C(i l m) ... C(m j) looks worse to 
me than what I did do.

> Is it too bothersome to ask that you put the comments back in?

The comment as originally there is wrong for the new code because the 
witnesses have changed. I also found it confusing with respect to the old 
code because it really documented what we thought the witnesses should be, 
rather than what they actually were - the rebuilding of the Repository 
reflected the fact that they needed coercing to the 'correct' ones.

I could put something similar back, though IMO it doesn't add much given 
that the witnesses are now actually correct and don't need hacking, so the 
types explain it all.

> tentativelyReplacePatches repository opts ps =
>     do repository' <- tentativelyRemovePatches_ DontUpdatePristine
> repository opts ps
> hunk ./src/Darcs/Repository/Internal.hs 591
> -       sequence_ $ mapAdd repository' ps
> -  where mapAdd :: Repository p C(m l i) -> FL (Named p) C(i j) -> [IO ()]
> -        mapAdd _ NilFL = []
> -        mapAdd r@(Repo dir df rf dr) (a:>:as) =
> -               -- we construct a new Repository object on the recursive
> case so that the
> -               -- recordedstate of the repository can match the fact
> that we just wrote a patch
> -               tentativelyAddPatch_ DontUpdatePristine r opts (n2pia a)
> : mapAdd (Repo dir df rf dr) as
> +       mapAdd repository' ps
> +  where mapAdd :: Repository p C(m l i) -> FL (Named p) C(i j) -> IO
> (Repository p C(m l j))
> +        mapAdd r NilFL = return r
> +        mapAdd r (a:>:as) =
> +               do r' <- tentativelyAddPatch_ DontUpdatePristine r opts
> (n2pia a)
> +                  mapAdd r' as
>
> It seems that the m l i vs. i l m stuff has something to do with the r
> vs. t changes.  If I buy the later change, then it seems like I sohuld
> go for this change as well.  Hmm...

Yeah, as per my explanation above.

>
>
> finalize_pending :: RepoPatch p => Repository p C(r u t) -> IO ()
> finalize_pending (Repo dir opts _ rt)
> hunk ./src/Darcs/Repository/Merge.hs 90
>              doChanges usi
>              setTentativePending r (effect pend' +>+ pw_resolution)
>      return $ seal (effect pwprim +>+ pw_resolution)
> -  where mapAdd :: Repository p C(m l i) -> FL (PatchInfoAnd p) C(i j)
> -> [IO ()]
> -        mapAdd _ NilFL = []
> -        mapAdd r'@(Repo dir df rf dr) (a:>:as) =
> -               -- we construct a new Repository object on the recursive
> case so that the
> -               -- recordedstate of the repository can match the fact
> that we just wrote a patch
> -               tentativelyAddPatch_ DontUpdatePristine r' opts a :
> mapAdd (Repo dir df rf dr) as
> +  where mapAdd :: Repository p C(m l i) -> FL (PatchInfoAnd p) C(i j)
> -> IO (Repository p C(m l j))
> +        mapAdd repo NilFL = return repo
> +        mapAdd repo (a:>:as) =
> +               do repo' <- tentativelyAddPatch_ DontUpdatePristine repo
> opts a
> +                  mapAdd repo' as
>
> Same change as above, but any reason to not include the comment?

Again, I viewed as explaining the hack that was needed to work around the 
incorrect witnesses. The hack is now gone (or at least moved to the more 
primitive operations).

>
>         applyps :: Repository p C(m l i) -> FL (PatchInfoAnd p) C(i j)
> -> IO ()
>         applyps repo ps = do debugMessage "Adding patches to inventory..."
> hunk ./src/Darcs/Repository/Merge.hs 97
> -                             sequence_ $ mapAdd repo ps
> +                             mapAdd repo ps
>
> Looks like the sequence is now absorbed into the locally defined
> mapAdd.  Not really sure why, but I won't contest the point :)

The type of sequence_ is wrong for the new type of mapAdd. I could have 
used foldM or similar but I find explicit recursion clearer in many 
cases like this one.


>                              debugMessage "Applying patches to pristine..."
>                              applyToTentativePristine repo ps
>
> [add concept of gaps
> Ganesh Sittampalam <ganesh at earth.li>**20091211010754
> Ignore-this: afe3115fd2333f00cb5a4c8a1f7ec281
> ] hunk ./src/Darcs/Witnesses/Sealed.hs 1
> --- Copyright (C) 2007 David Roundy
> +-- Copyright (C) 2007 David Roundy, 2009 Ganesh Sittampalam
>
> While you're at it, my name could be there too :)

Probably best as a separate patch.

BTW the reason I bothered there is that I added a substantial chunk of 
code and it ties in with my BSD-dedication for my contributions. Normally 
keeping copyright notices up to date is a losing battle, I think :-)

> +newtype Poly a = Poly { unPoly :: FORALL(x) a C(x) }
>
> Polymorphic?

Yeah.


> +
> +newtype Stepped f a C(x) = Stepped { unStepped :: f (a C(x)) }
>
> What does stepped mean?

It's sort of moving things sidewise. It's not a good name, but I couldn't 
think of a better one.

> Now I'm confused :)  These wrappers confuse me.
> [...]
> What is a gap?
> [...]
> This code is very abstract :)
>
> [...]
> Makes more sense with the haddocks.  Thanks for adding those.

As I said in my submission email, I'm aware that this is a confusing area. 
If you can ask some more specific questions in light of the haddock, I can 
try to improve the docs.

> unrecordedChanges opts repo paths = do
>   (all_current, _) <- readPending repo
> hunk ./src/Darcs/Repository/State.hs 135
> -  Sealed pending <- pendingChanges repo paths
> +  Sealed (pending :: FL Prim C(t x)) <- pendingChanges repo paths
>
> Making things explicit.
>
>   relevant <- restrictSubpaths repo paths
>   let getIndex = I.updateIndex =<< (relevant <$> readIndex repo)
> hunk ./src/Darcs/Repository/State.hs 153
>                  return $ if ignoretimes then plain else plain
> `overlay` index
>
>   ft <- filetypeFunction
> -  diff <- treeDiff ft current working
> +  Sealed (diff :: FL Prim C(x y)) <- (unFreeLeft `fmap` treeDiff ft
> current working) :: IO (Sealed (FL Prim C(x)))
>
> Wow.  Hmm...Probably just making it more explicit.

Just persuading the type-checker to accept my code. I wouldn't write those 
signatures for fun!

>   Sealed (diff :: FL Prim C(x y)) <- (unFreeLeft `fmap` treeDiff ft current working) :: IO (Sealed (FL Prim C(x)))
> +  IsEq <- return (unsafeCoerceP IsEq) :: IO (EqCheck C(y u))
>
> You're unsafeCoercing an EqCheck?  You're telling the type checker
> that y = u from now on?  I'm sure you have a good reason, so please
> explain.

Yeah, this needs documenting. This is a key "assertion" step required by 
the way we model repositories and by the way I've chosen to deal with 
freshly constructed patches.

Our approach is to say that we *know* what the unrecorded state of the 
repo is when we build one - that's why the 'u' variable is existential (or 
rather that all the functions passed to withRepository etc have to be 
polymorphic in 'u', which amounts to the same thing).

I'm now also saying that a freshly constructed patch has one end sealed 
and the other end free. So if it's a diff against recorded, then we can 
say that the free end is unified with 'r'. But that leaves the other end 
sealed, but we know because of what we just diffed that it must actually 
be 'u'. So we have to assert it.

There's actually another level of dodginess here, in that the caller can 
restrict the scope of the diff by passing in a list of subpaths. So two 
successive calls to unrecordedChanges could be used to construct two diffs 
that are both C(r u), and then you could "prove" that two different states 
are equal. I can sort of see this happening by accident, but I hope it's 
unlikely, and I can't see any nice way to guard against it.

> +
> +-- This is actually unsafe if we ever commute patches and then compare them
> +-- using this function. TODO: consider adding an extra existential to
> WPatchInfo
> +-- (as with TaggedPatch in Darcs.Patch.Choices)
> +compareWPatchInfo :: WPatchInfo C(a b) -> WPatchInfo C(c d) -> EqCheck
> C((a, b) (c, d))
> +compareWPatchInfo (WPatchInfo x) (WPatchInfo y) = if x == y then
> unsafeCoerceP IsEq else NotEq
>
> Should the above TODO be in the bug tracker or somewhere else?  Does it
> need doing?

It would be nice to do it, yes. It needs some infrastructure for a new 
kind of witnesses lists though. I don't know if we want to track wishlist 
code improvements on the bugtracker or not - grepping for TODO seems 
reasonable too.

> hunk ./src/Darcs/Repository/Repair.hs 59
> -replaceInFL :: FL (PatchInfoAnd a)
> -            -> [(PatchInfo, PatchInfoAnd a)]
> -            -> FL (PatchInfoAnd a)
> +replaceInFL :: FL (PatchInfoAnd a) C(x y)
> +            -> [Sealed2 (WPatchInfo :||: PatchInfoAnd a)]
>
> When was :||: introduced?

A few months ago by my hunk editing work. I used it in doing substitution 
on TaggedPatch - there's quite a similar pattern between the two chunks of 
code, in fact. Perhaps some abstraction is possible.

Cheers,

Ganesh


More information about the darcs-users mailing list