Hi! I'll be happy to apply this as soon as I get confirmation that I understand what's going on...
On Thu, Nov 06, 2008 at 12:58:22 +0100, Petr Rockai wrote:
> The way I have got the recursion versus syncing slurpies backwards is
> actually pretty embarassing.
I didn't understand this remark btw...
Refactor Repository.Repair.replayRepository to get rid of CanRepair.
--------------------------------------------------------------------
Looks ok to me overall. The main idea of this patch is to move the
patch-repair bits from replayRepository to the darcs repair command
> + BrokenPatches newpris _ -> do
> + brokenPristine newpris
> + putInfo $ text "Found broken patches."
> + done $ ExitFailure 1
Good idea
> -applyAndFix :: [...snip...] -> IO (FL (PatchInfoAnd p), Slurpy)
> +applyAndFix :: [...snip...] -> IO ((FL (PatchInfoAnd p)), Slurpy, Bool)
Those extra parentheses are superflous, right? Nit :-)
Also, it's probably a good idea to haddock this function now that
we're returning a mystery bool. My understanding is that we
return True if all the patches in the sequence can be applied.
> + Nothing -> return (p, True)
> Just (e,pp) -> do putStrLn e
> + return (pp, False)
For onlookers: we know a patch is broken when applyAnd*TryTo*Fix returns
a modified patch... (which is why we return False)
> hunk ./src/Darcs/Repository/Repair.hs 80
> -data RepositoryConsistency = RepositoryConsistent | RepositoryInconsistent
> Slurpy
> -data CanRepair = CanRepair | CannotRepair deriving Eq
> +data RepositoryConsistency p =
> + RepositoryConsistent
> + | BrokenPristine Slurpy
> + | BrokenPatches Slurpy (PatchSet p)
I might have named this BrokenPatchesAndOrPristine, but I guess that
would have been silly
> -replayRepository :: (RepoPatch p) => CanRepair -> Repository p ->
> [DarcsFlag] -> IO RepositoryConsistency
> -replayRepository canrepair repo opts = do
> +replayRepository :: (RepoPatch p) => Repository p -> [DarcsFlag] -> IO
> (RepositoryConsistency p)
> +replayRepository repo opts = do
So as I understand it, the idea is that you move the writing of the
revised patches out into the repair command (and just pass up the
new state), which seems like a much cleaner thing to do
> + -- TODO is the latter condition needed? Does a broken patch imply pristine
> + -- difference? Why, or why not?
> + return (if is_same && patches_ok
> + then RepositoryConsistent
> + else if patches_ok
> + then BrokenPristine s'
> + else BrokenPatches s' newpatches)
Maybe it's worthwhile to write this as
if patches_ok
then if is_same
then RepositoryConsistent
else BrokenPristine s'
else BrokenPatches s' newpatches
which is simpler (but maybe not as nice logically)
Introduce syncSlurpy, that syncs slurpy to disk as needed.
----------------------------------------------------------
I think we should try to haddock at least the functions we export, and
that your comment below would serve just fine:
> syncSlurpy takes an IO action that does the actual syncing, but only calls it
> when neccessary to prevent memory blowups. It is fairly cheap to call often
> (ie. after every applied patch).
> +unsyncedSlurpySize :: Slurpy -> Int
> +unsyncedSlurpySize (Slurpy _ (SlurpFile (_,_,size) ps))
> + | size == undefined_size = B.length ps
> + | otherwise = 0
> +unsyncedSlurpySize (Slurpy _ (SlurpDir _ ss)) =
> + sum $ map unsyncedSlurpySize (map_to_slurpies ss)
Ok, so from here I get the understanding that we know if a slurpy is synced if
we know its size. So this function adds up all the bits of slurpy for which we
do not yet know the size.
> +slurp_sync_size :: Int
> +slurp_sync_size = 100 * 1000000
> +
> +syncSlurpy :: (Slurpy -> IO Slurpy) -> Slurpy -> IO Slurpy
> +syncSlurpy put s = if (unsyncedSlurpySize s > slurp_sync_size)
> + then do
> + s' <- put s
> + return s'
> + else do
> + return s
Silly Haskell question: should this be simplified to the below?
syncSlurpy put s = if unsyncedSlurpySize s > slurp_sync_size
then put s
else return s
Fix Repair.applyAndFix.
-----------------------
> applyAndFix c opts s_ r psin =
> do beginTedious k
> tediousSize k $ lengthFL psin
> - ps <- aaf 0 s_ psin
> + ps <- aaf s_ psin
> endTedious k
> return ps
> where k = "Repairing patch" -- FIXME
> hunk ./src/Darcs/Repository/Repair.hs 65
> + aaf s NilFL = return (NilFL, s, True)
> + aaf s (p:>:ps) = do
> - let j = if ((i::Int) + 1 < 100) then i + 1 else 0
> - (ps', s'', restok) <- aaf j s' ps
> - s''' <- if j == 0 then update_slurpy r c opts s''
> - else return s''
> + s'' <- syncSlurpy (update_slurpy r c opts) s'
> + (ps', s''', restok) <- aaf s'' ps
Hmm, so rather than syncing every at 100 MB (or whatever syncSlurpy
decides) instead of every 100 patches? Is that the idea?
--
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9
signature.asc
Description: Digital signature
_______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
