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
Received on 2010-04-07 16:45:31 CEST