Paranoid is good. Thanks for the careful review (and for breaking up your comments by patch!)
Also, it may be useful to start with a quick summary of your review (to
make sure I don't miss anything)
Anyway, I'll be pushing this soon and sending the next bundle.
On Wed, Nov 12, 2008 at 15:23:13 +0100, Petr Rockai wrote:
> > hunk ./src/Darcs/Commands/Put.lhs 77
> > do
> > repodir <- fixUrl opts unfixedrepodir
> > -- Test to make sure we aren't trying to push to the current repo
> > - cur_absolute_repo_dir <- absolute_dir "."
> > - req_absolute_repo_dir <- absolute_dir repodir
> > + t_cur_absolute_repo_dir <- ioAbsoluteOrRemote "."
> > + t_req_absolute_repo_dir <- ioAbsoluteOrRemote repodir
> > + let cur_absolute_repo_dir = toPath t_cur_absolute_repo_dir
> > + req_absolute_repo_dir = toPath t_req_absolute_repo_dir
>
> This replaces occurances of absolute_dir with ioAbsoluteOrRemote, immediately
> untyping it to preserve the rest of the code unchanged.
Yeah. Hopefully one day we will start replacing type signatures in our
lower-level functions, for example, to require AbsoluteOrRemotePath or
AbsolutePath instead of FilePath. Maybe in the long run these typed
paths will either (a) serve as self-documentation or (b) actually catch
an error or two.
> Does just what the name says. I'm wondering if this gives us backslashes on
> Windows though, which *might* pose an issue? Maybe keeping to FilePath.Posix
> might be better for now, changing over everything at once when we know it is
> safe to do so?
Thanks for worrying! :-)
> This seems to do the right thing, however, basename and takeDirectory are not
> equivalent:
>
> Prelude System.FilePath> reverse . dropWhile ('/' /=) . dropWhile ('/' ==) .
> reverse $ "a/b/"
> "a/"
>
> Prelude System.FilePath> takeDirectory "a/b/"
> "a/b"
...and for being scrupulous about the corner cases
> Prelude UglyFileName Test.QuickCheck System.FilePath> quickCheck (\ p ->
> normalise p == (fn2fp $ norm_path $ fp2fn p))
> *** Failed! Falsifiable (after 98 tests and 5 shrinks):
> "/"
...and for being extra-scrupulous about corner cases we might not even
have thought of
> Specify that we want System.FilePath.Posix in Darcs.External
> ------------------------------------------------------------
> Okey, just what I was concerned above. We ensure </> gives us forward slashes
> here even on Windows.
Yeah, this is actually a request that David made for one of my
System.FilePath patches (not this one), but I never got around to
sending it.
> From a distance, this looks safe (with the above remarks about normalise). A
> good catch about the "." case, too.
Caught by our regression testing suite :-)
> Sorry to have taken this long, but it's a busy day today and other issues keep
> interrupting me. : - \ Hopefully, you find the review helpful.
Very much so! I wasn't expecting a quick reply with this, but I am
quite pleased to finish getting rid of our deprecated filepath modules.
--
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9
signature.asc
Description: Digital signature
_______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
