Many thanks to Petr for the review and to Reinier for the patch! I am trusting Petr's review here and applying this patch only with a cursory full-bundle graphical diff to understand roughly what is going on, and a reading of Petr's comments. Note that my maintainership will probably involve a lot of trusting reviewers :-)
Comments telling me that patches are just hunk moves were useful, as were the probing into potential correctness issues and pitfalls, thanks! On Sun, Nov 09, 2008 at 10:43:57 +0100, Petr Rockai wrote: > > hunk ./src/Darcs/Patch/FileName.lhs 124 > > +-- | Split a file path at the slashes > > breakup :: String -> [String] > > breakup p = case break (=='/') p of > > (d,"") -> [d] > > Just exporting breakup that's used elsewhere. Might be worth checking if > there's a better function already exported somewhere (one'd expect it to be > used somewhere already?) doing this. In general, we should be very careful about Darcs.Patch.FileName, but I do agree with Petr that there is likely no harm in this. On the other hand, I think we could use a System.FilePath.Posix.splitPath instead, and I would welcome a patch to do that. > > get_unrecorded_no_look_for_adds :: RepoPatch p => Repository p C(r u t) -> > > IO (FL Prim C(r y)) > > -get_unrecorded_no_look_for_adds = get_unrecorded_private (filter (/= > > LookForAdds)) > > +get_unrecorded_no_look_for_adds r = get_unrecorded_private (filter (/= > > LookForAdds)) r [] > > Just adding the empty list meaning "diff everything" to get_unrecorded_private > invocation. Looks ok. Thanks for explaining the significance of the empty list here (which I also see Reinier has done in his comment) > > -get_unrecorded_private modopts repository@(Repo r oldopts _ _) = > > - withCurrentDirectory r $ do > > +get_unrecorded_private modopts repository@(Repo r oldopts _ _) files = > > + withCurrentDirectory r (do > > What's been wrong with the $? I too would like to avoid making changes that aren't motivated by anything in particular > > - ftf <- filetype_function > > - let dif = case unsafeDiff opts ftf cur work of > > - di -> if AnyOrder `elem` opts > > - then pend +>+ di > > - else sort_coalesceFL $ pend +>+ di > > + let diffs = if null files > > + then [unsafeDiff opts ftf cur work] > > + else catMaybes (map (diff_at_path opts ftf cur work) > > (map fn2fp files)) > > + let workdiff = foldl (+>+) NilFL diffs > > I suppose the order doesn't matter here, we just lump all the patches > from "diffs" together. Actually, should the order that the diffs are returned to us be a cause for concern? > > [make get_unrecorded_private work with type witnesses again > > [EMAIL PROTECTED] > > Ignore-this: 97418e6487ef9c9508473d4c65f295ca > > ] > > - then [unsafeDiff opts ftf cur work] > > - else catMaybes (map (diff_at_path opts ftf cur work) > > (map fn2fp files)) > > - let workdiff = foldl (+>+) NilFL diffs > > + then unsafeDiff opts ftf cur work > > + else let diffsPerFile = catMaybes (map (diff_at_path > > opts ftf cur work) (map fn2fp files)) > > + in foldr (+>+) (unsafeCoerceP NilFL) diffsPerFile > > This change is beyond me... Jason? It does seem functionally equivalent > though. (Ok, after referring elsewhere, I see that this is where the type > witnesses catch the concatenation problem from above and we circumvent them by > unsafeCoerceP; this will hopefully be corrected later on.) David said this was buggy (below), but it appears to go away in subsequent patches, so I'll ignore it. I do like it if patch reviewers check individual patches when it is reasonable to do so, though. > > [hopefully less buggy version of get_unrecorded_in_files > > Reinier Lamers <[EMAIL PROTECTED]>**20081031215944 > > Ignore-this: 9f4f2320a1784cf6f7546ab23eb6bf61 > > ] hunk ./src/Darcs/Commands/WhatsNew.lhs 39 This patch name should call us to attention. The bug in question is: http://lists.osuosl.org/pipermail/darcs-users/2008-October/015199.html > > \end{code} > > hunk ./src/Darcs/Commands/WhatsNew.lhs 106 > > chold <- get_unrecorded_no_look_for_adds repository (map sp2fn files) > > s <- slurp_recorded repository > > ftf <- filetype_function > > - let pre_changed_files = apply_to_filepaths (invert chold) $ map > > toFilePath files > > - select_files = choose_touching pre_changed_files > > - Sealed cho <- return $ select_files chold > > - cho_adds :> _ <- return $ partitionRL is_hunk $ reverseFL cho > > - Sealed all_fs <- return $ select_files all_changes > > - cha :> _ <- return $ partitionRL is_hunk $ reverseFL all_fs > > + cho_adds :> _ <- return $ partitionRL is_hunk $ reverseFL chold > > + cha :> _ <- return $ partitionRL is_hunk $ reverseFL all_changes > > let chn = unsafeDiff [LookForAdds,Summary] ftf > > (fromJust $ apply_to_slurpy (reverseRL > > cho_adds) s) > > (fromJust $ apply_to_slurpy (reverseRL cha) s) > > chold now only contains the interesting hunks (those touching selected files), > so we don't need to filter the diff anymore. > > > hunk ./src/Darcs/Commands/WhatsNew.lhs 111 > > - exitOnNoChanges (chn, cho) > > - putDocLn $ summarize cho > > + exitOnNoChanges (chn, chold) > > + putDocLn $ summarize chold > > Just substituting chold for cho, the former now being already filtered (cho > used to just be the interesting subset of chold). Again, thanks for explaining. > > +make_nonoverlapping_path_set :: [FilePath] -> [FilePath] > > +make_nonoverlapping_path_set = map unbreakup . delete_overlapping . map > > breakup . sort > > + where > > + delete_overlapping :: [[FilePath]] -> [[FilePath]] > > + delete_overlapping (p1:p2:ps) = if p1 `isPrefixOf` p2 > > + then delete_overlapping (p1:ps) > > + else p1 : delete_overlapping (p2:ps) > > + delete_overlapping ps = ps > > + unbreakup = concat . intersperse "/" > > #endif > > Now, this assumes that prefixes always sort first, which is hopefully true > (and > trying it in ghci seems to confirm that). Is there a possibility that the > paths > are not canonical? We need to ensure that nothing like "foo/../bar" ever gets > fed into this function (!). This might actually be a weak spot, although > there's a call to fix paths somewhere quite high in the stack (in beginning of > most commands, I believe). It might be worth adding a test checking something > like "darcs whatsnew foo/. foo/bar/..". > > (Ok, looking near the end of the patch, this seems to be taken into account in > testing. I would just like this to be double-checked, as this is what we rely > on for correctness.) I very much appreciate this sort of hard thinking about correctness and potential pitfalls :-) > > hunk ./src/Darcs/Repository/Internal.lhs 434 > > - else let diffsPerFile = catMaybes (map (diff_at_path > > opts ftf cur work) (map fn2fp files)) > > - in foldr (+>+) (unsafeCoerceP NilFL) diffsPerFile > > + else unsafeDiffAtPaths (ignoreTimes, lookForAdds, > > summary) ftf cur work changed_files > > We now use the new unsafeDiffAtPaths that produces a single FL Prim from all > the paths, instead of mapping over paths, obtaining [FL Prim] that needs to be > concatenated (which is the unsafe operation). This fixes the possible double > appearance of same Prim here (assuming unsafeDiffAtPaths is correct, see > above). And here is the bugfix for what David pointed out... > (testing and buildsystem bits seem to follow, Kowey could you take over here > please? Or you probably know whom to ask...) Already applied, thanks! -- Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow> PGP Key ID: 08AC04F9
signature.asc
Description: Digital signature
_______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
