[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: r877014 - on Windows, invalid path => svn_node_none [was: svn commit: r930333 - /subversion/branches/1.6.x/STATUS]

From: Julian Foad <julian.foad_at_wandisco.com>
Date: Wed, 07 Apr 2010 15:28:39 +0100

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?

[[[
Index: subversion/libsvn_subr/io.c
===================================================================
--- subversion/libsvn_subr/io.c (revision 931483)
+++ subversion/libsvn_subr/io.c (working copy)
@@ -224,6 +224,9 @@ io_check_path(const char *path,
     *kind = svn_node_none;
   else if (APR_STATUS_IS_ENOTDIR(apr_err)
 #ifdef WIN32
+ /* On Windows, APR_STATUS_IS_ENOTDIR includes several kinds of
+ * invalid-pathname error but not this one, so we include it. */
+ /* ### This fix should go into APR. */
            || (APR_TO_OS_ERROR(apr_err) == ERROR_INVALID_NAME)
 #endif
            )
Index: subversion/include/svn_io.h
===================================================================
--- subversion/include/svn_io.h (revision 931483)
+++ subversion/include/svn_io.h (working copy)
@@ -93,8 +93,9 @@ typedef struct svn_io_dirent_t {
  * If @a path exists but is none of the above, set @a *kind to
  * #svn_node_unknown.
  *
- * If unable to determine @a path's kind, return an error, with @a *kind's
- * value undefined.
+ * If @a path is not a valid pathname, set @a *kind to #svn_node_none. If
+ * unable to determine @a path's kind for any other reason, return an error,
+ * with @a *kind's value undefined.
  *
  * Use @a pool for temporary allocations.
  *
]]]

- Julian
Received on 2010-04-07 16:29:13 CEST

This is an archived mail posted to the Subversion Dev mailing list.