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 <[email protected]> wrote: > Hi, > > Eric Kow <[email protected]> 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 > [email protected] > http://lists.osuosl.org/mailman/listinfo/darcs-users > _______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
