On Fri, Sep 26, 2008 at 03:17:26PM +0400, Dmitry Kurochkin wrote:
> Thanks for review, Eric!
>
> On Fri, Sep 26, 2008 at 1:53 PM, Eric Kow <[EMAIL PROTECTED]> wrote:
> > Providing secondary review because of file path trickiness. Thanks for
> > moving so quickly on this one!
> >
> > I'm going to push this into stable, but I have some questions about the
> > FileName code and think it would be ideal if you could answer them
> > before they go into 2.1.0. The questions are not about Dmitry's code
> > specifically, just the surrounding FileName stuff.
> >
> > Make FileName.drop_dotdot work with absolute paths.
> > ---------------------------------------------------
> >> drop_dotdot :: [String] -> [String]
> >> -drop_dotdot ("":p) = drop_dotdot p
> >> -drop_dotdot (".":p) = drop_dotdot p
> >> -drop_dotdot ("..":p) = ".." : (drop_dotdot p)
> >> -drop_dotdot (_:"..":p) = drop_dotdot p
> >> -drop_dotdot (d:p) = case drop_dotdot p of
> >> - ("..":p') -> p'
> >> - p' -> d : p'
> >> drop_dotdot [] = []
> >>
> > For the interested, drop_dotdot is used as a helper function for
> > normalising file paths.
> >
> > The input to this function is a list of path components (obtained via
> > FileName.breakup). The breakup function splits a path into parts that
> > are separated by a slash, so
> >
> > breakup "/foo/bar//baz/quux" == ["", "foo", "bar", "", "baz", "quux"]
> > breakup "toto/tata/.././" == ["toto", "tata", "..", ".", ""]
> >
> > The purpose of this function is to remove redundant path components,
> > performing the following reductions:
> >
> > convert "foo//bar" == "foo/bar"
> > convert "foo/./bar" == "foo/bar"
> > convert "../bar" == "../bar"
> > convert "foo/../bar" == "bar"
> >
> > See the code for actual details.
> >
> > By the way, this module is fully haddocked in the experimental darcs-doc
> > branch if I recall correctly. Hopefully the comments will make their
> > way in into darcs proper :-) (Florent is trying to reproduce issue1043
> > which is blocking his darcs-doc work a bit).
> >
> > hunk ./src/FileName.lhs 116
> >> +drop_dotdot f@(a:b)
> >> + | null a = "" : (drop_dotdot' b) -- first empty element is important
> >> + -- for absolute paths
> >> + | otherwise = drop_dotdot' f
> >
> > Dmitry's patch makes a small correction to the drop_dotdot logic.
> > Above, we assumed that an empty path component always corresponds to
> > a double slash. But this is not true! Another place where an empty
> > path component can appear is with the initial slash on absolute paths
> >
> > So:
> >
> > breakup "/foo/bar//baz/quux" == ["", "foo", "bar", "", "baz", "quux"]
> > old_drop_dotdot ["", "foo", "bar", "", "baz", "quux"]
> > == [ "foo", "bar", "baz", "quux"]
> > repath [ "foo", "bar", "baz", "quux"] = "foo/bar/baz/quux" -- WRONG!
> >
> > Dmitry's patch just adds a special case to re-insert the empty path
> > component if it comes from an absolute path.
> >
> > new_drop_dotdot ["", "foo", "bar", "", "baz", "quux"]
> > == ["", "foo", "bar", "baz", "quux"]
> >
> > So this /seems/ to be safe and correct, but it worries me quite a bit.
> > Why is it that we have never spotted a problem with this function until
> > now, after so many years? What has changed? Have we never been trying
> > to normalise absolute FilePaths before turning them into FileNames?
> > Are there other modules which rely on the assumption on that absolute
> > FileName lose their initial slash? We are using this norm_path function
> > a lot, sometimes in higher-level code too.
>
> That worries me too. I believe the function was never used for
> absolute paths. And I hope nothing relies on it's weird handling of
> absolute paths.
Right, it was never designed or intended to be used for absolute
paths. None of FileName was, and it troubles me that we're using
FileName outside of its intended use. As I've mentioned before, I'd
prefer to avoid doing this.
David
_______________________________________________
darcs-users mailing list
[email protected]
http://lists.osuosl.org/mailman/listinfo/darcs-users
