> The last patch is a huge one, in which I define a Prim patch type, > which is only truly primitive patches. I change pending-related code > to accept FL Prim, so the type system enforces that we don't get any > Mergers in there.
Pushing everything in. No meaningful comments
> It's still a refactor, I believe, although I did make the change
> that darcs whatsnew now just prints out the sequence of changes,
> without surrounding braces.
I still see surrounding braces...
======================================================================
Make HopefullyPrivate and Cancellation support gadts
======================================================================
Yay. Eagerly anticipating the day when I can flip the switch and just
compile everything with GADTs.
======================================================================
define actual merge algorithm (and retype remerge)
======================================================================
Going to have to just trust you on this one.
> +hokey_merge :: FL Patch C(a b) -> FL Patch C(a c) -> Sealed (FL Patch C(a))
> +hokey_merge (x:>:xs) p2 | Just (p2':>_) <- commuteFL (invert x :>: NilFL :>
> p2)
> + = (x:>:) `mapSeal` hokey_merge xs p2'
> + | otherwise = case conflict_to_end (x :> xs) of
> + xxs' :> _ -> hokey_merge xxs' p2
> +hokey_merge NilFL p2 = seal p2
hokey_merge seems like a slightly hokey name
======================================================================
Move patch manipulations into classes.
Move ReadPatch stuff into yet another class.
Define class for apply.
make merger code use RL/FL.
======================================================================
already reviewed
======================================================================
use showPatch instance of FL Patch.
======================================================================
seems fine
======================================================================
move merging into Commute class
======================================================================
There does not seem to be any changes in behaviour (really just moving
things around).
> - Nothing -> impossible
And I definitely like it when code like this goes away.
Here is some more informal documentation along the lines of
Eric-learns-darcs. The basic idea behind merging is that given
x :> y
x :> z <== you are here
You want to pull in patch y, adjusting it to fit the new context
x :> z :> y' <== you want to be here
In the simple case (which is the only one I know), the approach consists
in doing
x :> z :> z^
x :> z :> z^ :> y
x :> z :> y' :> z^'
x :> z :> y'
We can invert this scenario, starting from the first repository and
pulling z. This gives us:
x :> y :> y^ :> z
x :> y :> z' :> y^'
x :> y :> z'
Nice and symmetrical. We write this as merge (y :\/: z) = z' :/\: y'
> + case actual_merge (y:\/:z) of
> + y' -> case commutex (y' :< z) of
> + Nothing -> bugDoc $ text "merge_patches bug"
> + $$ showPatch y
> + $$ showPatch z
> + $$ showPatch y'
> + Just (z' :< _) -> z' :/\: y'
David seems to say that the same z' can be obtained by commuting the
original z with the adjusted y'
x :> z :> z^
x :> z :> z^ :> y
x :> z :> y' :> z^'
x :> z :> y'
x :> y'' :> z' -- (does y'' always == y?)
Which makes a sort of intuitive sense... I just wouldn't be able to
prove it or do anything of the sort. Note that in any case, this code
is in an #ifdef and will likely be going away in darcs 2.0.
> + Just (ip2':<p1') -> case commutex (p1' :< p2) of
> + Nothing -> Nothing -- should be a redundant check
> + Just (_:<p1o) -> if really_eq_patches p1o p1
> + then Just (invert ip2' :/\: p1')
> + else Nothing
Also, in some cases, we can obtain z' by just inverting z^' (z^'^ == z').
x :> z :> z^
x :> z :> z^ :> y
x :> z :> y' :> z^'
x :> z :> y'
x :> y'' :> z'
if (y'' == y) then Just (z^'^ :/\: y')
else Nothing
Don't really understand what these case are exactly, perhaps the ones
where (y'' /= y) are ones with conflicts?
> + merge ((x:>:xs) :\/: ys) = fromJust $ do ys' :/\: x' <- return $ mergeFL
> (x :\/: ys)
> + xs' :/\: ys'' <- return $ merge
> (ys' :\/: xs)
> + return (ys'' :/\: (x' :>: xs'))
I don't understand why this is not just
let ys' :/\: x' = mergeFL (x :\/: ys)
xs' :/\: ys'' = merge (ys' :\/: xs)
in (ys'' :/\: (x' :>: xs'))
Perhaps you're preparing to replace some bits above with actual Maybe
code? I haven't really thought about the merge itself, but it seems
to make sense.
======================================================================
make patchset_complement slightly more readable.
======================================================================
> - pushing_add = liftM2 (++) pushing $ liftM2 (flip (:)) (return []) ep
> + pushing_add = do ps <- pushing
> + thep <- ep
> + return (ps ++ [thep])
Yeah, thanks!
======================================================================
make PatchSet a RL RL.
======================================================================
Clearly the right thing to do. For the interested, this is part of the
continuing effort to make more aggressive use of types to prevent us
from making silly mistakes and to make the code easier to understand.
> - | tinfo `elem` deps = (reverse $ (tinfo,thisp) : sofar) : xs'
> + | tinfo `elem` deps = (reverseFL $ (tinfo,thisp) :>: sofar)
> :<: xs'
Just pointing this out as an example of why we want this. The new code
may be a bit harder to read at first, but easier to understand overall
once you get used to the conventions.
David:
I'm trusting you on the FL/RL directions.
Some common patterns, maybe worth putting in Darcs.Patch.Ordered
headRL (*)
concatRL . reverseFL
dropWhileNilRL (ok, not common, but maybe worth moving)
dropWhileRL
head . mapRL fst . concatR
NilRL :<: NilRL - maybe emptyPatchSet, although that wouldn't be
too useful for pattern matching
Also makes me wonder if some sort of List typeclass would be useful for
us, but we've probably already discussed this.
(*) headRL could even go into impossible.h so that we get __LINE__ and
__FILE__ in the error messages).
> hunk ./src/Darcs/Patch/Depends.lhs 438
> - case cmt_by (fir :< hopefully hp) of
> + case commuteFL (hopefully hp :> fir) of
> Nothing -> bug "patches to segregate_patches does not commutex
> (1)"
> hunk ./src/Darcs/Patch/Depends.lhs 440
> - Just (p' :< fir') -> ctt (p':las) fir' ps
> - cmt_by :: [Patch] :< Patch -> Maybe (Patch :< [Patch])
> - cmt_by (ps :< a) = do (a' :< jps') <- commutex (join_patches ps :< a)
> - return (a' :< flatten jps')
> + Just (fir' :> p') -> ctt (p':>:las) fir' ps
Accepting this on faith
> +type PatchSet C(x) = RL (RL PatchInfoAndPatch) C(() y)
C(() y)? Seems slightly supicious...
======================================================================
clean up ambiguous [Patch] in Depends.
fix accidentally-recorded change in makefile.
simplify code using FL instance of Commute.
clean up Depends a bit more (part 1 of 2).
clean up Depends a bit more (part 2 of 2 -- makes it more efficient).
move showContextPatch into ShowPatch class.
======================================================================
these patches seem ok. Nice and bite-sized too.
======================================================================
make Named patches a distinct type.
make SelectPatches be more type-specific.
======================================================================
seems ok
It's really nice to see all those fromJust's go away
> +anonymous :: Patch C(x y) -> NamedPatch C(x y)
> +anonymous p = namepatch "today" "anonymous" "unknown" ["anonymous"] p
> infopatch pi p = NamedP pi [] p
> adddeps (NamedP pi ds p) ds' = NamedP pi (ds++ds') p
Would it be useful to have an AnonymousP?
> + list_touched_files :: p C(x y) -> [FilePath]
the Commute class seems like a weird place to put this
> hunk ./src/Darcs/Patch/Patchy.lhs 61
> + showNicely :: p C(x y) -> Doc
> + showNicely = showPatch
what does Nicely mean?
> +instance (Apply p, ShowPatch p) => ShowPatch (FL p) where
> + thing x = thing (helperx x) ++ "s"
> + where helperx :: FL a C(x y) -> a C(x y)
> + helperx _ = undefined
eh?
> hunk ./src/Darcs/Repository/DarcsRepo.lhs 183
> - where hasChanged :: FilePath -> Patch -> IO Bool
> - hasChanged na pa =
> - do old <- gzReadFilePS na
> - `catch` (\_ -> return $ packString "")
> - case readPatch old of
> - Nothing -> return True -- new patch
> - Just (Sealed oldp,_) -> return $ not (oldp
> `really_eq_patches` pa)
Was hasChanged supposed to be some sort of optimisation?
I'm all for simpler code, so I guess I'm glad to see it go.
> +readPatchCarefully :: Stringalike s => s -> Maybe (Sealed (Patch C(x)), s)
> +readPatchCarefully s = case readPatch s of
> + Just (p, s') -> Just (mapSeal patchcontents p, s')
> + _ -> readPatch s
what does this do?
> - $ filter (is_similar (tp_patch tp) . tp_patch)
> + $ filter ((== touched_files) . list_touched_files .
> tp_patch)
does the order of the touched files matter?
======================================================================
remove Cancel/Cancellation/Marked patch types.
move Conflicted into separate patch type.
======================================================================
Too tired to review these. As far I as I can see, the bulk of the new
Darcs.Patch.Conflicted module comes from Darcs.Patch.Commute.
Conflicted patches come in two flavours, Normal (no conflict) and
Conflicted (real conflict), sort of like how named patches can be
anonymous...
--
Eric Kow http://www.loria.fr/~kow
PGP Key ID: 08AC04F9 Merci de corriger mon français.
pgpdpqePKWMig.pgp
Description: PGP signature
_______________________________________________ darcs-devel mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-devel
