On Tue, Oct 28, 2008 at 03:36:32PM -0700, Eric Kow wrote: > Salvo 6! > > Sun Oct 26 00:01:40 BST 2008 Don Stewart <[EMAIL PROTECTED]> > * Add LANGUAGE pragmas for explicit language extensions > > Sun Oct 26 16:40:09 GMT 2008 [EMAIL PROTECTED] > * add a get_unrecorded_in_files to check for unrecorded changes in a subset > of working directory > > Sun Oct 26 19:06:12 GMT 2008 [EMAIL PROTECTED] > * make get_unrecorded_private work with type witnesses again > > Sun Oct 26 19:46:36 GMT 2008 [EMAIL PROTECTED] > * make whatsnew use the lstat-saving functions to scan the working copy
These look very appealing, and have some nice ideas, but are also buggy. :( > ] hunk ./src/Darcs/Repository/Internal.lhs 417 > ftf <- filetype_function > Sealed pend <- read_pending repository > debugMessage "diffing dir..." > + -- the unsafeCoerceP below is necessary to be able to concatenate > + -- pend with NilFL to form dif. See http://hpaste.org/11480 > let diffs = if null files > hunk ./src/Darcs/Repository/Internal.lhs 420 > - then [smart_diff opts ftf cur work] > - else catMaybes (map (diff_at_path opts ftf cur work) (map > fn2fp files)) > - let workdiff = foldl (+>+) NilFL diffs > + then smart_diff opts ftf cur work > + else let diffsPerFile = catMaybes (map (diff_at_path opts > ftf cur work) (map fn2fp files)) > + in foldr (+>+) (unsafeCoerceP NilFL) diffsPerFile > dif = if AnyOrder `elem` opts There may be other bugs, but this one is sufficient to cause trouble. This unsafeCoerceP really is unsafe. Combining patches together in this way has serious potential for repository corruption! In particular, if a file is referenced more than once in the paths, you can get multiple copies of the same change. The get_unrecorded_in_files idea is great, and looks like the right sort of interface, but concatenating patches from separate diffs is way too fragile. A better approach would be to pass all the interesting files into a diff_at_paths. I'm right now pushing a test that demonstrates this bug in whatsnew. (Except it passes, since I'm not applying these patches just yet.) David P.S. This patch, of course, on its own couldn't cause repository corruption, since whatsnew doesn't modify the repository. But it would only be a matter of time before someone realized that we could apply the same optimization to record, and then we'd be seriously hosed. Which is why patch review is *very* important! P.P.S. It's nice to see a case where the type witnesses really did catch us a bug... Note that this code is right at the border between where the type witnesses protect us and where they don't, since Diff itself isn't protected by type witnesses. Fortunately, it was on the safe side of the border, or I might not have caught it... also fortunately, the bit on the diff side of the border is sufficiently simple so as to be easily reviewable. _______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
