[darcs-users] darcs patch: Make FileName.drop_dotdot work with abso... (and 2 more)

Dmitry Kurochkin dmitry.kurochkin at gmail.com
Fri Sep 26 11:17:26 UTC 2008


Thanks for review, Eric!

On Fri, Sep 26, 2008 at 1:53 PM, Eric Kow <kowey at darcs.net> 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.

I rely here on our tests and David :)

>
> 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

That was my first try. But in this case "where drop_dotdot'" is
visible inside the last drop_dotdot case (sorry my terminology):

drop_dotdot f@("":b) = "" : (drop_dotdot' b) -- first empty element is...
                             ^ drop_dotdot' is not defined here
drop_dotdot f        = drop_dotdot' f
  where drop_dotdot' = ....

So I rewritten it using guards. If there is a way to work around the
above error, I would like to know it! And I would send a patch to
simplify the code :)

>
> 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?

Yes. I did not test it. But is should work fine - super_name returns
'.' in this case.

Regards,
  Dmitry


More information about the darcs-users mailing list