Hi, second part of the reaction.
Eric Kow <ko...@darcs.net> writes: >> Thu Aug 12 00:36:15 CEST 2010 Petr Rockai <m...@mornfall.net> >> * Merge Darcs.Patch.FileName into Darcs.Path. >> >> Thu Aug 12 00:47:39 CEST 2010 Petr Rockai <m...@mornfall.net> >> * Remove the now-redundant sp2fn. >> >> Thu Aug 12 01:05:34 CEST 2010 Petr Rockai <m...@mornfall.net> >> * Fix announceFiles in WhatsNew (abolish unsafePathFrom*). >> >> Thu Aug 12 01:06:04 CEST 2010 Petr Rockai <m...@mornfall.net> >> * Restore the ".." check in isMaliciousPath. >> >> Thu Aug 12 01:29:40 CEST 2010 Petr Rockai <m...@mornfall.net> >> * Fix a subtle bug in onlyHunks with rather curious side-effects. > > I stopped here. > >> Thu Aug 12 01:30:27 CEST 2010 Petr Rockai <m...@mornfall.net> >> * Merge Darcs.RepoPath into Darcs.Path. > > More to go! And even more now... > Merge Darcs.Patch.FileName into Darcs.Path. > ------------------------------------------- >> - then do has_target <- treeHasDir cur (fn2fp $ superName $ fp2fn new) >> + then do has_target <- treeHasDir cur (fn2fp $ parent $ fp2fn new) > > Getting some sort of notion that we a get little renames like this > >> - result <- foldM removeOnePath >> (ftf,recorded,unrecorded, []) $ >> - map pathFromSubPath files >> + result <- foldM removeOnePath >> (ftf,recorded,unrecorded, []) files > > And less need for conversions like this. > (Presumably because we're accepting more FileName and fewer FilePath) At this point I think we work with FileName (Relative) most of the time. Although actual filesystem access is still using FilePath. That will come in later. >> hunk ./src/Darcs/Patch/Named.lhs 42 >> -import Darcs.Witnesses.Eq ( EqCheck(..) ) >> -import Darcs.Witnesses.Ordered ( FL(..), RL(..), mapFL, concatFL >> - , mapFL_FL ) >> -import Darcs.Patch.Prim ( Prim(..), FromPrim(..), Effect(effect, effectRL), >> nFn ) >> -#include "impossible.h" >> - >> -data Patch C(x y) where >> - PP :: Prim C(x y) -> Patch C(x y) >> - ComP :: FL Patch C(x y) -> Patch C(x y) >> - Merger :: Patch C(x y) >> - -> RL Patch C(x b) >> - -> Patch C(c b) >> - -> Patch C(c d) >> - -> Patch C(x y) >> - Regrem :: Patch C(x y) >> - -> RL Patch C(x b) >> - -> Patch C(c b) >> - -> Patch C(c a) >> - -> Patch C(y x) >> - >> -instance FromPrim Patch where >> - fromPrim = PP >> +import Darcs.Witnesses.Ordered ( concatFL ) >> +import Darcs.Patch.Prim ( Effect(effect, effectRL), nFn ) >> hunk ./src/Darcs/Patch/Named.lhs 42 >> -import Darcs.Witnesses.Ordered ( concatFL ) >> hunk ./src/Darcs/Patch/Named.lhs 40 >> -import Darcs.Patch.Info ( PatchInfo, patchinfo, makeFilename ) >> -import Darcs.Patch.Patchy ( Patchy ) >> -import Darcs.Patch.Prim ( Effect(effect, effectRL), nFn ) >> +import Darcs.Patch.Info ( PatchInfo, patchinfo, makeFilename, invertName, >> idpatchinfo ) >> +import Darcs.Patch.Patchy ( Patchy, Commute(..), Invert(..) ) >> +import Darcs.Patch.Prim ( Effect(effect, effectRL), nFn, Conflict(..) ) >> +import Darcs.Witnesses.Eq ( MyEq(..) ) >> +import Darcs.Witnesses.Ordered ( (:>)(..), (:\/:)(..), (:/\:)(..) ) >> hunk ./src/Darcs/Patch/Named.lhs 41 >> -import Darcs.Patch.Patchy ( Patchy, Commute(..), Invert(..) ) >> +import Darcs.Patch.Patchy ( Patchy, Commute(..), Merge(..), >> PatchInspect(..), Invert(..) ) >> ] >> : >> hunk ./src/Darcs/Patch/Named.lhs 45 >> -import Darcs.Patch.Prim ( Prim(..), FromPrim(..), Effect(effect, effectRL), >> nFn ) >> +import Darcs.Patch.Prim ( Prim(..), FromPrim(..), Effect(effect, effectRL) ) > >> hunk ./src/Darcs/Patch/FileName.hs 1 > > Module is nuked. I'm assuming that the code moved in Darcs.Path is only > hunk-moved. Yes, should be the case. > I'll just restate my reservations about the future stability of > Darcs.Path, that we'll need to make it sure that should Darcs.Path > change in the future our behaviour with respect to pre-existing patches > does not also change. Our of the challenges in the Darcs hacking > context is that correctness for Darcs is partly defined by historical > precedent, ie. "does not behave like it used to" can be considered a > bug. :-/ [I guess folks working on browser etc have similar problems, > cf. quirks mode, just with somewhat lower stakes] I keep that in mind, don't worry. Fortunately, darcs is not writing out particularly stupid filenames into patches, so it should not really matter much. If the path code breaks on paths as canonic as those in darcs patches, we will see that right away. > Remove the now-redundant sp2fn. > ------------------------------- > Skipping this and just assuming that it's a natural consequence of > the new consolidated FileName == SubPath == Storage.Hashed.Relative Indeed. > Fix announceFiles in WhatsNew (abolish unsafePathFrom*). > -------------------------------------------------------- >> - let paths = map toFilePath files >> - check = virtualTreeIO (mapM exists $ map unsafePathFromString >> paths) >> + let check = virtualTreeIO (mapM exists files) > > Seems nice, but it's not clear to me what the fix actually is. Because toFilePath was producing a non-canonic path, which then broke the Path invariants (which is what is unsafe about unsafePathFromString -- it allows you to build an invariant-breaking path). The type of "files" and argument type of exists is now the same, so this was a redundant roundtrip, too. > Restore the ".." check in isMaliciousPath. > ------------------------------------------ >> +isMaliciousPath = forbidden [ "_darcs", ".." ] >> + where forbidden bad (directory -> dir :/: rest) = dir `elem` bad || >> forbidden bad rest >> + forbidden _ _ = False > > This is one of those functions where you don't need to check old > behaviour, just the new one. > > forbidden: Nice use of English for clarity. > > Unless I misunderstand the (foo -> pattern) syntax, this is just > recursively walking the path elements and checking to see if any of the > elements are forbidden. Why have explicit recursion when we can just > convert to a list and use any? For now, I am not allowing a Path to be converted to a list. It will need a bit of thought whether this is desirable and whether it could open a loophole in the API. > Fix a subtle bug in onlyHunks with rather curious side-effects. > --------------------------------------------------------------- >> onlyHunks :: [Sealed (FL Prim C(x))] -> Bool >> onlyHunks [] = False >> -onlyHunks pss = fn2fp f /= "" && all oh pss >> +onlyHunks pss = fn2fp f /= "." && all oh pss >> where f = getAFilename pss >> oh :: Sealed (FL Prim C(x)) -> Bool >> oh (Sealed (p:>:ps)) = primIsHunk p && > > So we have a problem with assumptions about the representation of "." > Perhaps we need to introduce some sort of "isRelativeRoot" function and > abolish explicit checking on "."/"" instead? We actually have isRoot which we should probably use here. Any use of fn2fp is unsafe when it comes to path representation (as opposed to actually looking up files in the filesystem). Yours, Petr. _______________________________________________ darcs-users mailing list darcs-users@darcs.net http://lists.osuosl.org/mailman/listinfo/darcs-users
