On Tue, Apr 21, 2009 at 11:02 PM, Benedikt Schmidt <[email protected]> wrote: > Hello, > > Dmitry Kurochkin wrote: >> >> I was looking at issue1430 and found that "darcs changes GNUmakefile" >> and "darcs changes -i GNUmakefile" produce different patch sets. The >> attached patch fixes it. > > A lazier changes -i definitly sounds like a good idea. > Some comments inline.
Thanks for the comments! > >> Actually view_changes should not do any filtering on a patchset at all >> since it is already done in changes_cmd. I plan to clean it up and >> hopefully fix issue1430. > >> Tue Apr 21 18:04:29 MSD 2009 Dmitry Kurochkin >> <[email protected]> >> * Darcs.Commands.Changes: do not do read_repo twice. > > We already noticed this during the sprint, but concluded > that it's called twice to prevent a memory leak. I think there should be a comment explaining this. Or perhaps it is better to pass repository to changelog and call read_repo there if needed? > >> - ps <- read_repo repository >> - putDocLnWith printers $ changelog opts ps $ filtered_changes >> patches >> + putDocLnWith printers $ changelog opts patches $ >> filtered_changes patches > > filtered_changes goes through _all_ the patches sequentially, > but they can't be discarded by the GC if changelog needs them > later on. Note that changelog uses the patchset only if the > number option is used, so ps is not evaluated if the flag is > not present. Makes sense. Thanks for the explanation. > >> filter_patches_by_names [] pps = (pps, [], empty) >> filter_patches_by_names fs (hp:ps) >> | Just p <- hopefullyM hp = >> - case look_touch fs (invert p) of >> + case look_touch fs p of > >> hunk ./src/Darcs/Patch/TouchesFiles.hs 90 >> _ -> case splitAt (length t) f of >> (f', '/':_) -> f' == t >> _ -> False >> - fs' = sort $ apply_to_filepaths p fs >> + fs' = sort $ apply_to_filepaths (invert p) fs > > I see that it fixes the bug, but I would not expect look_touch > to invert the given patch since going forward seems to be the > natural thing (as there is no mention of rev in the function > name). > Are you sure that the patch does not break other commands > that use one of the functions in TouchesFiles and expect > the old behaviour? Just wondering if some of the more > usefull commands like rollback <file> have also been > broken all the time with renames. Looks like I did... At least revert is broken now. Just forget about these patches :) I have found source of strictness in changes is filter_patches_by_names. Now I need to find out how to refactor it. And this wrong patch filtering code would be removed anyway. Still worth checking other commands/review select code closely for similar errors. But I am not going to do this now. Regards, Dmitry > > Best, > Benedikt > > > > _______________________________________________ > 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
