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

dmitry.kurochkin at gmail.com dmitry.kurochkin at gmail.com
Mon Dec 29 20:55:48 UTC 2008


Hi Petr, Eric.

My laptop died last week and I was not able to do the review. And I
will not be able to review patches until it is repaired.

Sorry for such delay,  I did not have internet access :(

Regards,
  Dmitry


On 12/26/08, Petr Rockai <me at mornfall.net> wrote:
> 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
> _______________________________________________
> darcs-users mailing list
> darcs-users at darcs.net
> http://lists.osuosl.org/mailman/listinfo/darcs-users
>


More information about the darcs-users mailing list