2008/10/29 David Roundy <[EMAIL PROTECTED]>: >> ] 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 have to confess that I hadn't thought very hard about the case that a user would specify overlapping paths (which, as I understand it, is where the problem lies). But with darcs-2 semantics, two identical primitive patches just add up to one copy of that change, right? It's still heavily broken on darcs-1 repos of course. > 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.) I'll try out the test and see what it does with these patches applied. > 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! In fact, making record use this function was (and is) in my planning. I'll just fix it first. Reinier _______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
