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 <[email protected]>**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
_______________________________________________
darcs-users mailing list
[email protected]
http://lists.osuosl.org/mailman/listinfo/darcs-users
