On Wed, Sep 10, 2008 at 01:50:18PM +0100, Eric Kow wrote:
> 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)
Right.
> > --- | 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?
It could, except that it would lead to a recursive import, and I
didn't think it would be worthwhile to move withCurrentDirectory to
this module.
> 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.
Right. This is the only way to get an unambiguously correct answer as
to what this directory is called, and it's also obviously correct,
which I see as a large advantage.
> - And why we only do it when 'f' is a directory
We can't do it if the directory 'f' doesn't exist. That's not quite
true, and I just realized (independently) this morning that we could
do the trick on the parent of 'f' (and so on), which I think might be
a better solution than using makeAbsolute in this case.
> > -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 @[EMAIL
> PROTECTED]
> 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.
It's changed, because the previous code was buggy.
> Also, I'm not sure why we call the second argument 'dir'. Is it
> necessarily a directory?
No, it's not necessarily a directory.
> > +(/-) :: 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.
I think a nicer solution to this issue would be to make the second
argument a SubPath (which is guaranteed not to contain ..).
> > +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?
I don't see a good reason to do so.
> > + 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)
Except that (takeDirectory rootDir == rootDir) is really more of a bug
than a goal. The bug fix is that (takeDirectory "/foo" == "/").
> For that matter, I am somewhat concerned about rootDirectory being
> (AbsolutePath "/") because I don't think Windows understands if you say
> "/C:/foo".
It depends, of course, what we do with rootDirectory, but "/" is a
perfectly valid root directory on windows, it's the root of the
current drive.
> 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.
Indeed, it's an attempt to avoid writing buggy code. It's bad enough
that takeDirectory rootDirectory == rootDirectory, it'd be worse if we
had to deal with questions like what does (takeDirectory "../..")
equal, or even (takeDirectory "."). It's just not a well-posed
question, as far as I can tell. For absolute paths, we have the
advantage that there is only one error case, and in that case there's
a moderately reasonable answer to give. It'd probably be better to
check for the error case and fail, which is part of why this function
isn't exported--it's not really a nice function.
David
_______________________________________________
darcs-users mailing list
[email protected]
http://lists.osuosl.org/mailman/listinfo/darcs-users
