On Tue, Dec 23, 2008 at 18:21:28 +0100, Petr Rockai wrote: > this I consider the final (to be applied) version of the patch. I am deferring > the witness issue, since the Repository.Repair module uses unsafe operations > elsewhere and therefore does not compile with witnesses no matter the > changes. It'll likely be quite laborious to convert it to witnesses and I > would > like to see the fix applied in a timely manner. I will try to address the > witness question post-release.
I'm going to apply this patch, although I have some questions from Petr and would still like a review from Dmitry, in case he catches anything I miss. Note: I think that one mild side-effect of this is that patch repair descriptions are now printed in the reverse order (most recent first) Fix a memory leak in Darcs.Repository.Repair. --------------------------------------------- > +replaceInFL :: FL (PatchInfoAnd a) > + -> [(PatchInfo, PatchInfoAnd a)] > + -> FL (PatchInfoAnd a) > +replaceInFL orig [] = orig > +replaceInFL NilFL _ = impossible > +replaceInFL (o:>:orig) ch@((o',c):ch_rest) > + | info o == o' = c:>:replaceInFL orig ch_rest > + | otherwise = o:>:replaceInFL orig ch Petr: small question. My original idea was to return both the original patch and the repaired patch (PatchInfoAnd a, PatchInfoAnd a) so that we can just check (o == o2)... would that not work and also avoid the issue of requiring that PatchInfo uniquely identify a patch? It could also simplify. Sorry if you have already answered this and I missed it. Otherwise, this appears correct to me. The impossible case is if the list of repaired patches is larger than the original list, which cannot happen unless we change darcs repair to generate new patches or to split named patches up. It may be useful to haddock this requirement that the second list be smaller than the first one in case we ever decide to re-use this for other fancier things. > -applyAndFix :: RepoPatch p => Cache -> [DarcsFlag] -> Slurpy -> Repository p > -> FL (PatchInfoAnd p) -> IO ((FL (PatchInfoAnd p)), Slurpy, Bool) > +applyAndFix :: RepoPatch p => Cache -> [DarcsFlag] -> Slurpy -> Repository p > -> FL (PatchInfoAnd p) -> IO (FL (PatchInfoAnd p), Slurpy, Bool) This changed type signature appears to be just removing superfluous parentheses (and if so, reviewing the patch might have been slightly easier if it was not included) > - ps <- aaf s_ psin > + (repaired, slurpy, ok) <- aaf s_ psin > - return ps > + orig <- (reverseRL . concatRL) `fmap` read_repo r > + return (replaceInFL orig repaired, slurpy, ok) Rather than expect aaf to return the whole patch sequence, we just let it return the repaired patches and then zip them into the original patch sequence (which we get by doing a read_repo). Petr: how is psin diferrent from orig? > hunk ./src/Darcs/Repository/Repair.hs 84 > - (p', ourok) <- case mp' of > - Nothing -> return (p, True) > - Just (e,pp) -> do putStrLn e > - return (pp, False) > - p'' <- makePatchLazy r p' > s'' <- syncSlurpy (update_slurpy r c opts) s' > (ps', s''', restok) <- aaf s'' ps > hunk ./src/Darcs/Repository/Repair.hs 86 > - return ((p'':>:ps'), s''', restok && ourok) > + case mp' of > + Nothing -> return (ps', s''', restok) > + Just (e,pp) -> do putStrLn e > + p' <- makePatchLazy r pp > + return ((info p, p'):ps', s''', False) Context: (1) The helper (case mp') code just checks the status of applyAndTryToFix, which seems to return Just if something was wrong with the patch (and an error string), or Nothing if it was just fine. If there were modifications needed, we put putStrLn and discard their description. (2) We follow up on applyAndTryToFix of a single patch with a recursive call to aap on the rest of the patches. We then (3) return a combined result: the repaired patch sequence, slurpy and a Boolean which says if the original patches are correct. In Petr's patch, the code that prints out descriptions of patch modifications (1) is moved to the very end, and combined with the code that collates results (3). At first glance, this may appear to only be a refactor or simplification (note that one mild side-effect is that patch repair descriptions are now in reverse), but if we look closely, the result of this is that we no longer return a patch if it is untouched! And that is the point of the patch. Holding on these constitutes a memory leak. Otherwise, we also make a slight change which is instead of returning the repaired patch (p'), to return both (info p, and p') Petr: see my question above where I suggest returning (p, p') -- Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow> PGP Key ID: 08AC04F9
pgpY6DKS8L6Hz.pgp
Description: PGP signature
_______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
