Hi all, It's OK afaics.
I have jotted down my thoughts while reviewing below. On Monday 06 April 2009 20:46:45 [email protected] wrote: >[moved test for resolved issue1162 from bugs to tests >[email protected]**20090329223730 > Ignore-this: 34f0fc91b57c017e101cb068537b0c79 >] move ./bugs/issue1162_add_nonexistent_slash.sh > ./tests/issue1162_add_nonexistent_slash.sh OK, 1162 is really resolved according to bugs.darcs.net. > [added more docs, plus some > renamings and simplifications >[email protected]**20090401190250 > Ignore-this: 626996efc928cb7afef82c5d2ddfe4a1 >] hunk ./src/Darcs/RepoPath.hs 21 > -- the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, > -- Boston, MA 02110-1301, USA. > >-module Darcs.RepoPath ( AbsolutePath, makeAbsolute, ioAbsolute, > rootDirectory, - SubPath, makeSubPathOf, > simpleSubPath, >- AbsolutePathOrStd, >- makeAbsoluteOrStd, ioAbsoluteOrStd, > useAbsoluteOrStd, - AbsoluteOrRemotePath, > ioAbsoluteOrRemote, isRemote, - sp2fn, >- FilePathOrURL(..), FilePathLike(toFilePath), >- getCurrentDirectory, setCurrentDirectory >- ) where >+-- | Various abstractions for dealing with paths. >+module Darcs.RepoPath ( >+ -- * AbsolutePath >+ AbsolutePath, >+ makeAbsolute, >+ ioAbsolute, >+ rootDirectory, >+ -- * AbsolutePathOrStd >+ AbsolutePathOrStd, >+ makeAbsoluteOrStd, >+ ioAbsoluteOrStd, >+ useAbsoluteOrStd, >+ -- * AbsoluteOrRemotePath >+ AbsoluteOrRemotePath, >+ ioAbsoluteOrRemote, >+ isRemote, >+ -- * SubPath >+ SubPath, >+ makeSubPathOf, >+ simpleSubPath, >+ -- * Miscellaneous >+ sp2fn, >+ FilePathOrURL(..), >+ FilePathLike(toFilePath), >+ getCurrentDirectory, >+ setCurrentDirectory >+) where Yay for nice haddock formatting. Good. >--- | Relative to the local darcs repository and normalized >--- Note: these are understood not to have the dot in front >+-- | Paths which are relative to the local darcs repository and normalized. >+-- Note: These are understood not to have the dot in front. Is this usage of whitespace in line with The Style? Anyway, it's not a reason to reject a patch. >hunk ./src/Darcs/RepoPath.hs 71 >+ > newtype AbsolutePath = AbsolutePath FilePath deriving (Eq, Ord) This makes clear that the above comment is only about SubPath, not about AbsolutePath or any other types that follow. >hunk ./src/Darcs/RepoPath.hs 73 >+ >+-- | This is for situations where a string (e.g. a command line argument) >+-- may take the value \"-\" to mean stdin or stdout (which one depends on >+-- context) instead of a normal file path. > data AbsolutePathOrStd = AP AbsolutePath | APStd deriving (Eq, Ord) > data AbsoluteOrRemotePath = AbsP AbsolutePath | RmtP String deriving (Eq, > Ord) Good. >hunk ./src/Darcs/RepoPath.hs 121 > > simpleSubPath :: FilePath -> Maybe SubPath > simpleSubPath x | null x = bug "simpleSubPath called with empty path" >- | is_relative x = Just $ SubPath $ FilePath.normalise $ map > cleanup x + | is_relative x = Just $ SubPath $ > FilePath.normalise $ pathToPosix x Rename cleanup to pathToPosix, see later on. >hunk ./src/Darcs/RepoPath.hs 140 >+-- | Take an absolute path and a string representing a (possibly relative) >+-- path and combine them into an absolute path. If the second argument is >+-- already absolute, then the first argument gets ignored. This function > also +-- takes care that the result is converted to Posix convention and > +-- normalized. Also, parent directories (\"..\") at the front of the > string +-- argument get canceled out against trailing directory parts of > the +-- absolute path argument. >+-- >+-- Regarding the last point, someone more familiar with how these functions >+-- are used should verify that this is indeed necessary or at least useful. > makeAbsolute :: AbsolutePath -> FilePath -> AbsolutePath Another yay for comments! >hunk ./src/Darcs/RepoPath.hs 152 >- then AbsolutePath $ >- slashes ++ FilePath.normalise cleandir >- else ma a $ FilePath.normalise cleandir >+ then AbsolutePath (norm_slashes dir') >+ else ma a dir' > where >hunk ./src/Darcs/RepoPath.hs 155 >- cleandir = map cleanup dir >- slashes = norm_slashes $ takeWhile (== '/') cleandir >+ dir' = FilePath.normalise $ pathToPosix dir >+ -- Why do we care to reduce ".." here? >+ -- Why not do this throughout the whole path, i.e. "x/y/../z" -> "x/z" Cutting duplicate code. >hunk ./src/Darcs/RepoPath.hs 172 >-simpleClean x = norm_slashes $ reverse $ dropWhile (=='/') $ reverse $ >- map cleanup x >+simpleClean = norm_slashes . reverse . dropWhile (=='/') . reverse . > pathToPosix Make shorter with point-free and use pathToPosix. >hunk ./src/Darcs/RepoPath.hs 223 >--- | When mapped over characters in a path, this function normalizes >--- the path separator to Unix style (slash, not backslash). This only >--- affects Windows systems. >-cleanup :: Char -> Char >+-- | Normalize the path separator to Posix style (slash, not backslash). >+-- This only affects Windows systems. >+pathToPosix :: FilePath -> FilePath >+pathToPosix = map convert where > #ifdef WIN32 >hunk ./src/Darcs/RepoPath.hs 228 >-cleanup '\\' = '/' >+ convert '\\' = '/' > #endif >hunk ./src/Darcs/RepoPath.hs 230 >-cleanup c = c >+ convert c = c This gives 'cleanup' a more descriptive name and makes it work on FilePaths instead of single Chars. Good.
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
