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

[Petr Rockai]

Hi,

Eric Kow <kowey at darcs.net> writes:
> 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.
Ack, and thanks for review!

> 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)
Hm, true, and that's fixable. It might make sense to revert to original
behaviour, which isn't hard at all. Just a "unless (isNothing mp') $ putStrLn
e" before the recursive call and remove it from the case. I'm not sure which is
actually better, on second thought.

> 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.
Right. I have tried this, but I stumbled because apparently 'PatchInfoAnd p' is
not in Eq, and I would rather avoid anything adventurous. I tried adding
"reasonable" class constraints on the functions in question (that might also
explain the parentheses changed below) and failed.

> 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.
Well, larger -- more like that there are items that didn't get matched against
something. But yes, the impossible case is really impossible, or the code is
buggy.

>> -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)
Right, code evolution. :\

>> -       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?
The difference is in that we really want to forget psin as soon as
possible. The difference really is, that the file contents are unevaluated in
orig, while they are already evaluated in psin. So by using a new list, we let
the RTS collect the data in psin and avoid leaking memory.

Yours,
   Petr.

-- 
Peter Rockai | me()mornfall!net | prockai()redhat!com
 http://blog.mornfall.net | http://web.mornfall.net

"In My Egotistical Opinion, most people's C programs should be
 indented six feet downward and covered with dirt."
     -- Blair P. Houghton on the subject of C program indentation