[darcs-users] darcs patch: moved test for resolved issue1162 from b... (and 1 more)

Reinier Lamers tux_rocker at reinier.de
Tue Apr 7 20:18:03 UTC 2009


Hi all,

It's OK afaics. 

I have jotted down my thoughts while reviewing below.

On Monday 06 April 2009 20:46:45 ben.franksen at online.de wrote:
>[moved test for resolved issue1162 from bugs to tests
>ben.franksen at online.de**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
>ben.franksen at online.de**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.



-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.osuosl.org/pipermail/darcs-users/attachments/20090407/e2928d71/attachment.pgp>


More information about the darcs-users mailing list