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.
A minor style comment: perhaps this could be rewritten without the
guards
drop_dotdot f@("":b) = "" : (drop_dotdot' b) -- first empty element is...
drop_dotdot f = drop_dotdot' f
Resolve issue1078: make ioAbsolute work with symbolic links in file paths.
--------------------------------------------------------------------------
> ] hunk ./src/Darcs/RepoPath.hs 116
> then bracket (setCurrentDirectory dir)
> (const $ setCurrentDirectory $ toFilePath here)
> (const getCurrentDirectory)
> - else return $ makeAbsolute here dir
> + else let super_dir = (fn2fp . super_name . fp2fn) dir
> + file = (fn2fp . own_name . fp2fn) dir
> + in do abs_dir <- ioAbsolute super_dir
> + return $ makeAbsolute abs_dir file
This seems quite sensible; Dmitry is merely completing the ioAbsolute
function.
The problem ioAbsolute tries to solve is that the same directory can
have different names (because of symbolic links). The simplest way to
make sure we always use the same name (even if the user does not) is to
just change to that directory and ask the operating system what the
current working directory is.
The slight oversight -- one which /should/ have been caught during
review, apologies -- is that if a directory can have different names due
to symbolic links, any files under that directory also can have multiple
names. For example, if "foo" and "quux" are the same directories, then
"foo/bar" and "quux/bar" are the same file.
Dmitry's code tells us that if we are calling ioAbsolute on a file, then
we must get its parent directories' ioAbsolute name. One question: what
happens if the file in question is "/foo"? Does super_name do what we
expect?
Move issue1078 test from bugs to tests.
---------------------------------------
> Dmitry Kurochkin <[EMAIL PROTECTED]>**20080925180103
> Ignore-this: f735ee2e36bdf8f446cab61d1f7ac334
> ] move ./bugs/issue1078_symlink.sh ./tests/issue1078_symlink.sh
Yay!
--
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9
pgpTvHlXwxiNI.pgp
Description: PGP signature
_______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
