[darcs-users] darcs patch: Fix a memory leak in Darcs.Repository.Repair.

[Eric Kow]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 194 bytes
Desc: not available
Url : http://lists.osuosl.org/pipermail/darcs-users/attachments/20081226/2fdfaa4b/attachment.pgp