[darcs-users] darcs patch: fix bug in file path handling. (and 2 more)

Eric Kow kowey at darcs.net
Wed Sep 10 12:50:18 UTC 2008


Thanks to Jason for the review and to Dmitry for confirming that
issue1045 is fixed by this.  I'm going to push this to stable
because Dmitry says it works and the buildbots too.

Here are my comments.

fix bug in file path handling.
------------------------------
> hunk ./src/Darcs/Arguments.lhs 322
> -                       Just (_,o) -> makeAbsolute o f
> +                       Just (_,o) -> withCurrentDirectory o $ ioAbsolute f

Here is the problem we are trying to solve:
 - the user was originally in directory 'o'
     (when he invoked darcs)
 - he passed 'f' as an argument to darcs
 - somewhere along the line, darcs may have changedired to the
   repository root

What absolute path does 'f' correspond to now that we changed
directories?  The simple answer is "if f is absolute then f, otherwise o
</> f", but for some reason, we have decided that this simple answer
will not suffice.

Instead we now use withCurrentDirectory o $ ioAbsolute f, which does
something like

 pushd o
 ioAbsolute f
 popd

(Yes, I am mixing bashisms with Haskell)

> --- | Interpret the second (relative) path wrt the first (absolute) one
> ---   N.B. makeAbsolute "\/foo" "..\/..\/..\/bar" == "\/bar"
> +-- | Interpret a possibly relative path wrt the current working directory
> +ioAbsolute :: FilePath -> IO AbsolutePath
> +ioAbsolute dir =
> +    do isdir <- doesDirectoryExist dir
> +       here <- getCurrentDirectory
> +       if isdir
> +         then bracket (setCurrentDirectory dir)
> +                      (const $ setCurrentDirectory $ toFilePath here)
> +                      (const getCurrentDirectory)
> +         else return $ makeAbsolute here dir
> +

I wonder if the isdir branch could be written simply as
  withCurrentDirectory dir getCurrentDirectory?

Unfortunately, I do not understand why
 - Why we do this.  My guess is that this is due to issue1057, which
   says that 'o' may well be a symbolic link, and that the right thing
   to do would be to get the real path for that link.
 - And why we only do it when 'f' is a directory

Maybe the issue is that 'f' may a symbolic link?

> -makeAbsolute p1 "." = p1 -- FIXME: This is not such a great way to fix this issue.
> -makeAbsolute p1 "./" = p1
> -makeAbsolute p1 rawp2 = case mkAbsolutePath rawp2 of
> -                        Just ap2 -> ap2
> -                        Nothing -> ma p1 $ fn2fp $ norm_path $ fp2fn $ map cleanup rawp2
> - where ma p ('.':'.':'/':r) = let pp = takeDirectory p
> -                              in if pp == p then AbsolutePath ('/':drop_dotdots r) else ma pp r
> -       ma p r = combine p (SubPath r)
> -       drop_dotdots ('.':'.':'/':r) = drop_dotdots r
> -       drop_dotdots r = r
> +makeAbsolute a dir = if is_absolute dir
> +                     then AbsolutePath $
> +                          slashes ++ (fn2fp $ norm_path $ fp2fn cleandir)
> +                     else ma a $ fn2fp $ norm_path $ fp2fn cleandir
> +  where
> +    cleandir  = map cleanup dir
> +    slashes = norm_slashes $ takeWhile (== '/') cleandir
> +    ma here ('.':'.':'/':r) = ma (takeDirectory here) r
> +    ma here ".." = takeDirectory here
> +    ma here "." = here
> +    ma here "" = here
> +    ma here r = here /- ('/':r)

'mkAbsolute' @a dir@ returns an absolute path that corresponds to @dir at .
If @dir@ is already absolute we just clean it up and return it,
otherwise, we interpret the relative path assuming such that @dir@ is
relative to @a@

This looks like a cleanup/refactor of makeAbsolute and mkAbsolutePath,
the former being a function which produces AbsolutePaths out of a
pre-existing absolute path and a potentially but not necessarily
relative path; the latter just being one that generates AbsolutePaths
out of absolute paths (returning Nothing otherwise).  I haven't looked
closely enough to know if anything has changed or if it's just a
refactor.

Also, I'm not sure why we call the second argument 'dir'.  Is it
necessarily a directory?

Finally for the interested, norm_slashes doesn't do anything special; it
just removes excess pre-slashes on paths under Unix ("///foo -> /foo").
We don't do this in Windows because "//foo/bar" means something like
"path bar on server foo" if I'm not mistaken.

> +(/-) :: AbsolutePath -> String -> AbsolutePath
> +x /- ('/':r) = x /- r
> +(AbsolutePath "/") /- r = AbsolutePath ('/':simpleClean r)
> +(AbsolutePath x) /- r = AbsolutePath (x++'/':simpleClean r)

This looks like it's just </> from System.FilePath, or (///) from darcs,
only with an AbsolutePath guarantee.

Roughly speaking
  (AbsolutePath "/") /- "bar"  = (AbsolutePath "/bar")
  (AbsolutePath "/") /- "/bar" = (AbsolutePath "/bar")
  (AbsolutePath "/foo") /- "/bar" = (AbsolutePath "/foo/bar")
  (AbsolutePath "/foo") /- "bar"  = (AbsolutePath "/foo/bar")

So it may be worthwhile to return a bug or error in case of the
following arguments to (/-)

  (AbsolutePath "/foo") /- ""    = (AbsolutePath "/foo/") -- bad?
  (AbsolutePath "/foo") /- "."   = (AbsolutePath "/foo/.")
  (AbsolutePath "/foo") /- ".."  = (AbsolutePath "/foo/..")

This is so that we can preserve a guarantee (?) that AbsolutePath is
clean.

> +simpleClean :: String -> String
> +simpleClean x = norm_slashes $ reverse $ dropWhile (=='/') $ reverse $
> +                map cleanup x
>  

It's good that we have this, although we may be able to score a small
refactor with the takeDirectory code below (takeDirectory'?)

> -takeDirectory :: FilePathLike a => a -> a
> -takeDirectory = withInternal (reverse .  drop 1 . dropWhile (/='/') . reverse)
> +takeDirectory :: AbsolutePath -> AbsolutePath
> +takeDirectory (AbsolutePath x) =
> +    case reverse $ drop 1 $ dropWhile (/='/') $ reverse x of
> +    "" -> AbsolutePath "/"

Shouldn't the ("" -> AbsolutePath "/") case be replaced by
rootDirectory?

> +    x' -> AbsolutePath x'

Ok, this looks the heart of the issue1045 fix.  Before, takeDirectory
"/" would result in the empty string, and now it results in the root
directory.  This sort of thing could be verified by the unit test
(takeDirectory rootDir == rootDir)

For that matter, I am somewhat concerned about rootDirectory being
(AbsolutePath "/") because I don't think Windows understands if you say
"/C:/foo". 

I notice that our implementation of toPath code for AbsolutePath makes
no attempt to strip off this leading slash for Windows... which makes
sense because annoyingly enough, on Windows "//SERVER/Foo" is used for
shared drives.

Also, I am guessing that making takeDirectory explicitly AbsolutePath
based is purely a design decision, i.e. to avoid having code that does
more than we need.

> -mkAbsolutePath :: FilePath -> Maybe AbsolutePath
> -mkAbsolutePath p | is_absolute p =
> -                     Just $ AbsolutePath $ slashes ++ (fn2fp $ norm_path $ fp2fn cleanp)
> -                 | otherwise = Nothing
> -  where
> -    cleanp  = map cleanup p
> -    slashes = norm_slashes $ takeWhile (== '/') cleanp
> -

don't inline global variables in URL.
-------------------------------------
Along with Jason, no comment.

resolve issue1057: this was fixed in the previous patch.
--------------------------------------------------------
I've sent a small tweak to this for Windows

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : http://lists.osuosl.org/pipermail/darcs-users/attachments/20080910/eaae485d/attachment.pgp 


More information about the darcs-users mailing list