On Wed, 2010-04-07 at 10:32 -0400, C. Michael Pilato wrote: > Julian Foad wrote: > > C. Michael Pilato wrote: > >> Bert Huijben wrote: > >>> Before r877014 we returned an error on paths as > >>> "C:\path\that\is:invalid", but we didn't return an error on an equally > >>> invalid path "//q:" (format is "//server/share" or "q:/") that happens to > >>> return ERROR_BAD_PATHNAME instead of ERROR_INVALID_NAME because it is > >>> handled in another layer of the Windows path redirector. > >>> > >>> So what I tried to say is that the APR_STATUS_IS_ENOENT() and > >>> APR_STATUS_IS_ENOTDIR() checks catch almost every path syntax error, but > >>> not this specific one. I fixed this by also handling this error like the > >>> other bad pathnames. An equally valid (or possibly better) route is to > >>> handle all these invalid path errors as an error. > >> FWIW, I found your prior explanation of why you prefer this route of fix > >> both informative and compelling. What lacks is in-code documentation of > >> why > >> devs would see what appears to the casual passer-by as just some > >> out-of-place special-casing. (That's a lot of hyphens.) Surely others > >> will > >> wonder why ENOENT and ENOTDIR aren't sufficient in this case. > > > > +1. > > > > Is this patch good? > > -1. I prefer my comments in green. > > (Just kidding. +1)
OK, r931568. That sorts out the initial confusion. That just leaves the wierdness of check_repos_path() - but that's only a static function, not a public API, so if it does what it needs to do I'm not too concerned. I don't plan to do anything to it. - Julian
