You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by pb...@apache.org on 2010/04/02 19:35:32 UTC
svn commit: r930333 - /subversion/branches/1.6.x/STATUS
Author: pburba
Date: Fri Apr 2 17:35:32 2010
New Revision: 930333
URL: http://svn.apache.org/viewvc?rev=930333&view=rev
Log:
* STATUS: Nominate r877014, including rhuijben's vote via IRC.
Modified:
subversion/branches/1.6.x/STATUS
Modified: subversion/branches/1.6.x/STATUS
URL: http://svn.apache.org/viewvc/subversion/branches/1.6.x/STATUS?rev=930333&r1=930332&r2=930333&view=diff
==============================================================================
--- subversion/branches/1.6.x/STATUS (original)
+++ subversion/branches/1.6.x/STATUS Fri Apr 2 17:35:32 2010
@@ -118,6 +118,16 @@ Candidate changes:
Votes:
+1: danielsh
+ * r877014
+ On Windows, treat a path containing invalid characters as a non-existing
+ path (svn_node_none) rather than raising an error.
+ Justification:
+ Fixes basic_tests.py 37 'use folders with names like 'c:hi'' on
+ Windows, which has been failing over ra_local since r923779 (the
+ reintegration of the 1.6.x-issue-3242-partial branch).
+ Votes:
+ +1: pburba, rhuijben (via IRC)
+
Veto-blocked changes:
=====================
Re: r877014 - on Windows, invalid path => svn_node_none [was: svn
commit: r930333 - /subversion/branches/1.6.x/STATUS]
Posted by Julian Foad <ju...@wandisco.com>.
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
Re: r877014 - on Windows, invalid path => svn_node_none [was: svn
commit: r930333 - /subversion/branches/1.6.x/STATUS]
Posted by Julian Foad <ju...@wandisco.com>.
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
Re: r877014 - on Windows, invalid path => svn_node_none [was: svn
commit: r930333 - /subversion/branches/1.6.x/STATUS]
Posted by "C. Michael Pilato" <cm...@collab.net>.
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)
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: r877014 - on Windows, invalid path => svn_node_none [was: svn
commit: r930333 - /subversion/branches/1.6.x/STATUS]
Posted by "C. Michael Pilato" <cm...@collab.net>.
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)
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: r877014 - on Windows, invalid path => svn_node_none [was: svn
commit: r930333 - /subversion/branches/1.6.x/STATUS]
Posted by Julian Foad <ju...@wandisco.com>.
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
Re: r877014 - on Windows, invalid path => svn_node_none [was: svn
commit: r930333 - /subversion/branches/1.6.x/STATUS]
Posted by Julian Foad <ju...@wandisco.com>.
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
Re: r877014 - on Windows, invalid path => svn_node_none [was: svn
commit: r930333 - /subversion/branches/1.6.x/STATUS]
Posted by "C. Michael Pilato" <cm...@collab.net>.
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.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: r877014 - on Windows, invalid path => svn_node_none [was: svn
commit: r930333 - /subversion/branches/1.6.x/STATUS]
Posted by "C. Michael Pilato" <cm...@collab.net>.
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.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: r877014 - on Windows, invalid path => svn_node_none [was: svn
commit: r930333 - /subversion/branches/1.6.x/STATUS]
Posted by Paul Burba <pt...@gmail.com>.
On Wed, Apr 7, 2010 at 9:46 AM, Bert Huijben <be...@qqmail.nl> wrote:
>> -----Original Message-----
>> From: Paul Burba [mailto:ptburba@gmail.com]
>> Sent: woensdag 7 april 2010 15:27
>> To: Bert Huijben
>> Cc: Julian Foad; rhuijben@apache.org; dev@subversion.apache.org
>> Subject: Re: r877014 - on Windows, invalid path => svn_node_none [was: svn
>> commit: r930333 - /subversion/branches/1.6.x/STATUS]
>
>> Bert...don't kill me, but where in either r877014 or my alternative
>> patch are we returning an error we didn't return before?
>
> 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.
Thanks Bert,
That makes r877014 clear (and compelling) particularly when combined
with Julian's doc patch, which I see he just committed.
Sorry for the confusion.
Paul
RE: r877014 - on Windows, invalid path => svn_node_none [was: svn commit: r930333 - /subversion/branches/1.6.x/STATUS]
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Paul Burba [mailto:ptburba@gmail.com]
> Sent: woensdag 7 april 2010 15:27
> To: Bert Huijben
> Cc: Julian Foad; rhuijben@apache.org; dev@subversion.apache.org
> Subject: Re: r877014 - on Windows, invalid path => svn_node_none [was: svn
> commit: r930333 - /subversion/branches/1.6.x/STATUS]
>
> On Wed, Apr 7, 2010 at 5:09 AM, Bert Huijben <be...@qqmail.nl> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Paul Burba [mailto:ptburba@gmail.com]
> >> Sent: dinsdag 6 april 2010 21:59
> >> To: Julian Foad
> >> Cc: rhuijben@apache.org; dev@subversion.apache.org
> >> Subject: Re: r877014 - on Windows, invalid path => svn_node_none [was:
> svn
> >> commit: r930333 - /subversion/branches/1.6.x/STATUS]
> >>
> >> On Tue, Apr 6, 2010 at 10:53 AM, Paul Burba <pt...@gmail.com>
> wrote:
> >> > On Tue, Apr 6, 2010 at 10:22 AM, Paul Burba <pt...@gmail.com>
> wrote:
> >> >> On Tue, Apr 6, 2010 at 8:39 AM, Julian Foad
> <ju...@wandisco.com>
> >> wrote:
> >> >>> Hi Paul, Bert.
> >> >>>
> >> >>> I'm wondering about this r877014 change, which is proposed for 1.6.x
> >> >>> back-port:
> >> >>>
> >> >>> [[[
> >> >>> r877014 | rhuijben | 2009-04-02 14:01:48 +0100 (Thu, 02 Apr 2009) | 7
> >> lines
> >> >>>
> >> >>> * subversion/libsvn_subr/io.c
> >> >>> (io_check_path): On Windows, treat a path containing invalid
> characters
> >> as
> >> >>> a non-existing path. (We already detected invalid drives and invalid
> >> >>> network paths.) This enables locating the repository root via
> >> >>> svn_repos_find_root_path() in cases like:
> >> >>> $ svn mkdir --parents file:///G:/repos/d/f:r/s:t -m ""
> >> >>>
> >> >>> ------------------------------------------------------------------------
> >> >>> if (APR_STATUS_IS_ENOENT(apr_err))
> >> >>> *kind = svn_node_none;
> >> >>> - else if (APR_STATUS_IS_ENOTDIR(apr_err))
> >> >>> + else if (APR_STATUS_IS_ENOTDIR(apr_err)
> >> >>> +#if WIN32
> >> >>> + || (APR_TO_OS_ERROR(apr_err) == ERROR_INVALID_NAME)
> >> >>> +#endif
> >> >>> + )
> >> >>> *kind = svn_node_none;
> >> >>> else if (apr_err)
> >> >>> return svn_error_wrap_apr(apr_err, _("Can't check path '%s'"),
> >> >>> ]]]
> >> >>>
> >> >>> There seems to be a funny interdependency between
> io_check_path()
> >> and
> >> >>> check_repos_path() and svn_repos_find_root_path().
> >> >>>
> >> >>> io_check_path() returns 'none' if the requested entry is not on disk,
> >> >>> obviously, but this change now makes it also return 'none' if the
> name
> >> >>> is invalid, which it didn't do before.
> >> >>>
> >> >>> check_repos_path() says "Return TRUE on errors (which would be
> >> >>> permission errors, probably)", which is a rather rash assumption.
> >> >>
> >> >> Hi Julian,
> >> >>
> >> >> Never mind the rash assumption, how about the fact that this function
> >> >> is effectively asked "is PATH the root of a repository, yes or no?"
> >> >> and answers "yes" when PATH is in fact is the root of a repository and
> >> >> "yes*" when it is not.
> >> >>
> >> >> * "Yes it is, actually wait, we are just kidding it isn't, but don't
> >> >> worry, you'll find that out later!"
> >> >>
> >> >>> svn_repos_find_root_path() first checks whether the path has some
> >> kinds
> >> >>> of invalid characters:
> >> >>>
> >> >>> [[[
> >> >>> /* Try to decode the path, so we don't fail if it contains characters
> >> >>> that aren't supported by the OS filesystem. The subversion fs
> >> >>> isn't restricted by the OS filesystem character set. */
> >> >>> err = svn_utf_cstring_from_utf8(&decoded, candidate, pool);
> >> >>> if (!err && check_repos_path(candidate, pool))
> >> >>> [then return 'candidate']
> >> >>> ]]]
> >> >>>
> >> >>> It looks like the desired effect is being achieved by a rather oddly
> >> >>> layered set of assumptions and interactions in this chain of functions.
> >> >>
> >> >> Yeah, odd indeed. r877014's change to io_check_path() is really about
> >> >> making svn_repos_find_root_path() not choke on
> check_repos_path()'s
> >> >> bizzare semantic of returning TRUE when a path is not in fact the root
> >> >> of a repository. Perhaps we should fix svn_repos_find_root_path()
> >> >> directly?
> >> >
> >> > Bert,
> >> >
> >> > Wouldn't reverting r877014 and moving the fix into
> >> > repos.c:check_repos_path() like so...
> >> >
> >> > [[[
> >> > Index: subversion/libsvn_repos/repos.c
> >> >
> >>
> ==========================================================
> >> =========
> >> > --- subversion/libsvn_repos/repos.c (revision 931168)
> >> > +++ subversion/libsvn_repos/repos.c (working copy)
> >> > @@ -1417,6 +1417,13 @@
> >> > &kind, pool);
> >> > if (err)
> >> > {
> >> > +#ifdef WIN32
> >> > + if (APR_TO_OS_ERROR(err->apr_err) == ERROR_INVALID_NAME)
> >> > + {
> >> > + svn_error_clear(err);
> >> > + return FALSE;
> >> > + }
> >> > +#endif
> >> > svn_error_clear(err);
> >> > return TRUE;
> >> > }
> >> > ]]]
> >> >
> >> > ...solve the problem r877014 was intended to address without changing
> >> > the semantics of svn_io_check_path()?
> >> >
> >> > (trying this right now)
> >> >
> >> > Paul
> >> >
> >> >>> Changing [svn_]io_check_path() has far wider potential
> repercussions
> >> >>> than just the intended result.
> >> >>>
> >> >>> What do you think?
> >> >>
> >> >> I've changed my vote to -0. I can't point to any specific problems,
> >> >> but upon further reflection, I don't feel entirely comfortable saying
> >> >> this change won't cause other problems.
> >> >>
> >> >> Hoping Bert can weigh in on this...
> >>
> >> Hi Bert,
> >>
> >> A few questions below...
> >>
> >> From IRC:
> >>
> >> > <Bert> julianf, pburba: I see no issue with moving the check in the
> >> > repos code. But I think that we should avoid checking for
> >> > windows specific error codes there.
> >>
> >> Why not? Is there something inherently wrong with that?
> >>
> >> > (So maybe wrap the error
> >> > by some SVN_ERR_<new-code> for trunk.
> >>
> >> Wrap what error? svn_repos_find_root_path() and check_repos_path()
> >> don't return errors, they clear everything.
> >>
> >> > We can't introduce new
> >> > error code defines on 1.6.x)
> >>
> >> I wasn't suggesting returning an error, rather making
> >> repos.c:check_repos_path() return FALSE when asked if a path, with
> >> invalid characters in it, could be the root of a repository.
> >>
> >> > <Bert> (To busy working on non subversion things right now to look
> >> > into it myself)
> >> > <Bert> julianf: (As Windows is the only actively supported OS with
> >> > invalid path formats I didn't see the change of return value
> >> > as a big issue or something with high impact when I committed
> >> > the patch. (I don't think there are many callers of that
> >> > function that notice the small behavior change, but fixing it
> >> > for the repository paths is probably a safer solution))
> >
> > 18:47 <@pburba> Bert: Re 'wrap the error by some SVN_ERR_<new-code>
> for trunk. We can't introduce new error code
> > defines on 1.6.x'...so we wouldn't backport anything?
> >
> > We can backport the code with windows specific #ifdef's in libsvn_repos,
> but I would recommend adding a new error code for trunk.
> >
> > Or all users of our libraries have to test for specific errors using #ifdef's for
> platforms they might not even use. (It is hard to document that one of our
> functions can return an error that is not even defined on all platforms).
>
> Bert...don't kill me, but where in either r877014 or my alternative
> patch are we returning an error we didn't return before?
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.
Bert
Re: r877014 - on Windows, invalid path => svn_node_none [was: svn
commit: r930333 - /subversion/branches/1.6.x/STATUS]
Posted by Paul Burba <pt...@gmail.com>.
On Wed, Apr 7, 2010 at 5:09 AM, Bert Huijben <be...@qqmail.nl> wrote:
>
>
>> -----Original Message-----
>> From: Paul Burba [mailto:ptburba@gmail.com]
>> Sent: dinsdag 6 april 2010 21:59
>> To: Julian Foad
>> Cc: rhuijben@apache.org; dev@subversion.apache.org
>> Subject: Re: r877014 - on Windows, invalid path => svn_node_none [was: svn
>> commit: r930333 - /subversion/branches/1.6.x/STATUS]
>>
>> On Tue, Apr 6, 2010 at 10:53 AM, Paul Burba <pt...@gmail.com> wrote:
>> > On Tue, Apr 6, 2010 at 10:22 AM, Paul Burba <pt...@gmail.com> wrote:
>> >> On Tue, Apr 6, 2010 at 8:39 AM, Julian Foad <ju...@wandisco.com>
>> wrote:
>> >>> Hi Paul, Bert.
>> >>>
>> >>> I'm wondering about this r877014 change, which is proposed for 1.6.x
>> >>> back-port:
>> >>>
>> >>> [[[
>> >>> r877014 | rhuijben | 2009-04-02 14:01:48 +0100 (Thu, 02 Apr 2009) | 7
>> lines
>> >>>
>> >>> * subversion/libsvn_subr/io.c
>> >>> (io_check_path): On Windows, treat a path containing invalid characters
>> as
>> >>> a non-existing path. (We already detected invalid drives and invalid
>> >>> network paths.) This enables locating the repository root via
>> >>> svn_repos_find_root_path() in cases like:
>> >>> $ svn mkdir --parents file:///G:/repos/d/f:r/s:t -m ""
>> >>>
>> >>> ------------------------------------------------------------------------
>> >>> if (APR_STATUS_IS_ENOENT(apr_err))
>> >>> *kind = svn_node_none;
>> >>> - else if (APR_STATUS_IS_ENOTDIR(apr_err))
>> >>> + else if (APR_STATUS_IS_ENOTDIR(apr_err)
>> >>> +#if WIN32
>> >>> + || (APR_TO_OS_ERROR(apr_err) == ERROR_INVALID_NAME)
>> >>> +#endif
>> >>> + )
>> >>> *kind = svn_node_none;
>> >>> else if (apr_err)
>> >>> return svn_error_wrap_apr(apr_err, _("Can't check path '%s'"),
>> >>> ]]]
>> >>>
>> >>> There seems to be a funny interdependency between io_check_path()
>> and
>> >>> check_repos_path() and svn_repos_find_root_path().
>> >>>
>> >>> io_check_path() returns 'none' if the requested entry is not on disk,
>> >>> obviously, but this change now makes it also return 'none' if the name
>> >>> is invalid, which it didn't do before.
>> >>>
>> >>> check_repos_path() says "Return TRUE on errors (which would be
>> >>> permission errors, probably)", which is a rather rash assumption.
>> >>
>> >> Hi Julian,
>> >>
>> >> Never mind the rash assumption, how about the fact that this function
>> >> is effectively asked "is PATH the root of a repository, yes or no?"
>> >> and answers "yes" when PATH is in fact is the root of a repository and
>> >> "yes*" when it is not.
>> >>
>> >> * "Yes it is, actually wait, we are just kidding it isn't, but don't
>> >> worry, you'll find that out later!"
>> >>
>> >>> svn_repos_find_root_path() first checks whether the path has some
>> kinds
>> >>> of invalid characters:
>> >>>
>> >>> [[[
>> >>> /* Try to decode the path, so we don't fail if it contains characters
>> >>> that aren't supported by the OS filesystem. The subversion fs
>> >>> isn't restricted by the OS filesystem character set. */
>> >>> err = svn_utf_cstring_from_utf8(&decoded, candidate, pool);
>> >>> if (!err && check_repos_path(candidate, pool))
>> >>> [then return 'candidate']
>> >>> ]]]
>> >>>
>> >>> It looks like the desired effect is being achieved by a rather oddly
>> >>> layered set of assumptions and interactions in this chain of functions.
>> >>
>> >> Yeah, odd indeed. r877014's change to io_check_path() is really about
>> >> making svn_repos_find_root_path() not choke on check_repos_path()'s
>> >> bizzare semantic of returning TRUE when a path is not in fact the root
>> >> of a repository. Perhaps we should fix svn_repos_find_root_path()
>> >> directly?
>> >
>> > Bert,
>> >
>> > Wouldn't reverting r877014 and moving the fix into
>> > repos.c:check_repos_path() like so...
>> >
>> > [[[
>> > Index: subversion/libsvn_repos/repos.c
>> >
>> ==========================================================
>> =========
>> > --- subversion/libsvn_repos/repos.c (revision 931168)
>> > +++ subversion/libsvn_repos/repos.c (working copy)
>> > @@ -1417,6 +1417,13 @@
>> > &kind, pool);
>> > if (err)
>> > {
>> > +#ifdef WIN32
>> > + if (APR_TO_OS_ERROR(err->apr_err) == ERROR_INVALID_NAME)
>> > + {
>> > + svn_error_clear(err);
>> > + return FALSE;
>> > + }
>> > +#endif
>> > svn_error_clear(err);
>> > return TRUE;
>> > }
>> > ]]]
>> >
>> > ...solve the problem r877014 was intended to address without changing
>> > the semantics of svn_io_check_path()?
>> >
>> > (trying this right now)
>> >
>> > Paul
>> >
>> >>> Changing [svn_]io_check_path() has far wider potential repercussions
>> >>> than just the intended result.
>> >>>
>> >>> What do you think?
>> >>
>> >> I've changed my vote to -0. I can't point to any specific problems,
>> >> but upon further reflection, I don't feel entirely comfortable saying
>> >> this change won't cause other problems.
>> >>
>> >> Hoping Bert can weigh in on this...
>>
>> Hi Bert,
>>
>> A few questions below...
>>
>> From IRC:
>>
>> > <Bert> julianf, pburba: I see no issue with moving the check in the
>> > repos code. But I think that we should avoid checking for
>> > windows specific error codes there.
>>
>> Why not? Is there something inherently wrong with that?
>>
>> > (So maybe wrap the error
>> > by some SVN_ERR_<new-code> for trunk.
>>
>> Wrap what error? svn_repos_find_root_path() and check_repos_path()
>> don't return errors, they clear everything.
>>
>> > We can't introduce new
>> > error code defines on 1.6.x)
>>
>> I wasn't suggesting returning an error, rather making
>> repos.c:check_repos_path() return FALSE when asked if a path, with
>> invalid characters in it, could be the root of a repository.
>>
>> > <Bert> (To busy working on non subversion things right now to look
>> > into it myself)
>> > <Bert> julianf: (As Windows is the only actively supported OS with
>> > invalid path formats I didn't see the change of return value
>> > as a big issue or something with high impact when I committed
>> > the patch. (I don't think there are many callers of that
>> > function that notice the small behavior change, but fixing it
>> > for the repository paths is probably a safer solution))
>
> 18:47 <@pburba> Bert: Re 'wrap the error by some SVN_ERR_<new-code> for trunk. We can't introduce new error code
> defines on 1.6.x'...so we wouldn't backport anything?
>
> We can backport the code with windows specific #ifdef's in libsvn_repos, but I would recommend adding a new error code for trunk.
>
> Or all users of our libraries have to test for specific errors using #ifdef's for platforms they might not even use. (It is hard to document that one of our functions can return an error that is not even defined on all platforms).
Bert...don't kill me, but where in either r877014 or my alternative
patch are we returning an error we didn't return before?
Paul
RE: r877014 - on Windows, invalid path => svn_node_none [was: svn commit: r930333 - /subversion/branches/1.6.x/STATUS]
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Bert Huijben [mailto:bert@qqmail.nl]
> Sent: woensdag 7 april 2010 11:09
> To: 'Paul Burba'; 'Julian Foad'
> Cc: rhuijben@apache.org; dev@subversion.apache.org
> Subject: RE: r877014 - on Windows, invalid path => svn_node_none [was: svn
> commit: r930333 - /subversion/branches/1.6.x/STATUS]
>
>
>
> > -----Original Message-----
> > From: Paul Burba [mailto:ptburba@gmail.com]
> > Sent: dinsdag 6 april 2010 21:59
> > To: Julian Foad
> > Cc: rhuijben@apache.org; dev@subversion.apache.org
> > Subject: Re: r877014 - on Windows, invalid path => svn_node_none [was:
> svn
> > commit: r930333 - /subversion/branches/1.6.x/STATUS]
> >
> > On Tue, Apr 6, 2010 at 10:53 AM, Paul Burba <pt...@gmail.com> wrote:
> > > On Tue, Apr 6, 2010 at 10:22 AM, Paul Burba <pt...@gmail.com>
> wrote:
> > >> On Tue, Apr 6, 2010 at 8:39 AM, Julian Foad
> <ju...@wandisco.com>
> > wrote:
> > >>> Hi Paul, Bert.
> > >>>
> > >>> I'm wondering about this r877014 change, which is proposed for 1.6.x
> > >>> back-port:
> > >>>
> > >>> [[[
> > >>> r877014 | rhuijben | 2009-04-02 14:01:48 +0100 (Thu, 02 Apr 2009) | 7
> > lines
> > >>>
> > >>> * subversion/libsvn_subr/io.c
> > >>> (io_check_path): On Windows, treat a path containing invalid
> characters
> > as
> > >>> a non-existing path. (We already detected invalid drives and invalid
> > >>> network paths.) This enables locating the repository root via
> > >>> svn_repos_find_root_path() in cases like:
> > >>> $ svn mkdir --parents file:///G:/repos/d/f:r/s:t -m ""
> > >>>
> > >>> ------------------------------------------------------------------------
> > >>> if (APR_STATUS_IS_ENOENT(apr_err))
> > >>> *kind = svn_node_none;
> > >>> - else if (APR_STATUS_IS_ENOTDIR(apr_err))
> > >>> + else if (APR_STATUS_IS_ENOTDIR(apr_err)
> > >>> +#if WIN32
> > >>> + || (APR_TO_OS_ERROR(apr_err) == ERROR_INVALID_NAME)
> > >>> +#endif
> > >>> + )
> > >>> *kind = svn_node_none;
> > >>> else if (apr_err)
> > >>> return svn_error_wrap_apr(apr_err, _("Can't check path '%s'"),
> > >>> ]]]
> > >>>
> > >>> There seems to be a funny interdependency between
> io_check_path()
> > and
> > >>> check_repos_path() and svn_repos_find_root_path().
> > >>>
> > >>> io_check_path() returns 'none' if the requested entry is not on disk,
> > >>> obviously, but this change now makes it also return 'none' if the name
> > >>> is invalid, which it didn't do before.
> > >>>
> > >>> check_repos_path() says "Return TRUE on errors (which would be
> > >>> permission errors, probably)", which is a rather rash assumption.
> > >>
> > >> Hi Julian,
> > >>
> > >> Never mind the rash assumption, how about the fact that this function
> > >> is effectively asked "is PATH the root of a repository, yes or no?"
> > >> and answers "yes" when PATH is in fact is the root of a repository and
> > >> "yes*" when it is not.
> > >>
> > >> * "Yes it is, actually wait, we are just kidding it isn't, but don't
> > >> worry, you'll find that out later!"
> > >>
> > >>> svn_repos_find_root_path() first checks whether the path has some
> > kinds
> > >>> of invalid characters:
> > >>>
> > >>> [[[
> > >>> /* Try to decode the path, so we don't fail if it contains characters
> > >>> that aren't supported by the OS filesystem. The subversion fs
> > >>> isn't restricted by the OS filesystem character set. */
> > >>> err = svn_utf_cstring_from_utf8(&decoded, candidate, pool);
> > >>> if (!err && check_repos_path(candidate, pool))
> > >>> [then return 'candidate']
> > >>> ]]]
> > >>>
> > >>> It looks like the desired effect is being achieved by a rather oddly
> > >>> layered set of assumptions and interactions in this chain of functions.
> > >>
> > >> Yeah, odd indeed. r877014's change to io_check_path() is really about
> > >> making svn_repos_find_root_path() not choke on
> check_repos_path()'s
> > >> bizzare semantic of returning TRUE when a path is not in fact the root
> > >> of a repository. Perhaps we should fix svn_repos_find_root_path()
> > >> directly?
> > >
> > > Bert,
> > >
> > > Wouldn't reverting r877014 and moving the fix into
> > > repos.c:check_repos_path() like so...
> > >
> > > [[[
> > > Index: subversion/libsvn_repos/repos.c
> > >
> >
> ==========================================================
> > =========
> > > --- subversion/libsvn_repos/repos.c (revision 931168)
> > > +++ subversion/libsvn_repos/repos.c (working copy)
> > > @@ -1417,6 +1417,13 @@
> > > &kind, pool);
> > > if (err)
> > > {
> > > +#ifdef WIN32
> > > + if (APR_TO_OS_ERROR(err->apr_err) == ERROR_INVALID_NAME)
> > > + {
> > > + svn_error_clear(err);
> > > + return FALSE;
> > > + }
> > > +#endif
> > > svn_error_clear(err);
> > > return TRUE;
> > > }
> > > ]]]
> > >
> > > ...solve the problem r877014 was intended to address without changing
> > > the semantics of svn_io_check_path()?
> > >
> > > (trying this right now)
> > >
> > > Paul
> > >
> > >>> Changing [svn_]io_check_path() has far wider potential repercussions
> > >>> than just the intended result.
> > >>>
> > >>> What do you think?
> > >>
> > >> I've changed my vote to -0. I can't point to any specific problems,
> > >> but upon further reflection, I don't feel entirely comfortable saying
> > >> this change won't cause other problems.
> > >>
> > >> Hoping Bert can weigh in on this...
> >
> > Hi Bert,
> >
> > A few questions below...
> >
> > From IRC:
> >
> > > <Bert> julianf, pburba: I see no issue with moving the check in the
> > > repos code. But I think that we should avoid checking for
> > > windows specific error codes there.
> >
> > Why not? Is there something inherently wrong with that?
> >
> > > (So maybe wrap the error
> > > by some SVN_ERR_<new-code> for trunk.
> >
> > Wrap what error? svn_repos_find_root_path() and check_repos_path()
> > don't return errors, they clear everything.
> >
> > > We can't introduce new
> > > error code defines on 1.6.x)
> >
> > I wasn't suggesting returning an error, rather making
> > repos.c:check_repos_path() return FALSE when asked if a path, with
> > invalid characters in it, could be the root of a repository.
> >
> > > <Bert> (To busy working on non subversion things right now to look
> > > into it myself)
> > > <Bert> julianf: (As Windows is the only actively supported OS with
> > > invalid path formats I didn't see the change of return value
> > > as a big issue or something with high impact when I committed
> > > the patch. (I don't think there are many callers of that
> > > function that notice the small behavior change, but fixing it
> > > for the repository paths is probably a safer solution))
>
> 18:47 <@pburba> Bert: Re 'wrap the error by some SVN_ERR_<new-code>
> for trunk. We can't introduce new error code
> defines on 1.6.x'...so we wouldn't backport anything?
>
> We can backport the code with windows specific #ifdef's in libsvn_repos, but
> I would recommend adding a new error code for trunk.
>
> Or all users of our libraries have to test for specific errors using #ifdef's for
> platforms they might not even use. (It is hard to document that one of our
> functions can return an error that is not even defined on all platforms).
I think my original reasoning was:
apr.h has:
#define APR_STATUS_IS_ENOTDIR(s) ((s) == APR_ENOTDIR \
|| (s) == APR_OS_START_SYSERR + ERROR_PATH_NOT_FOUND \
|| (s) == APR_OS_START_SYSERR + ERROR_BAD_NETPATH \
|| (s) == APR_OS_START_SYSERR + ERROR_BAD_NET_NAME \
|| (s) == APR_OS_START_SYSERR + ERROR_BAD_PATHNAME \
|| (s) == APR_OS_START_SYSERR + ERROR_INVALID_DRIVE)
And this patch made Subversion handle ERROR_INVALID_NAME the same as this group of errors.
I would prefer to see the BAD results as an error *or* all of them handled as kind svn_node_none. The old behavior made all the other errors OK, except ERROR_INVALID_NAME... which is just weird behavior.
Bert
RE: r877014 - on Windows, invalid path => svn_node_none [was: svn commit: r930333 - /subversion/branches/1.6.x/STATUS]
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Paul Burba [mailto:ptburba@gmail.com]
> Sent: dinsdag 6 april 2010 21:59
> To: Julian Foad
> Cc: rhuijben@apache.org; dev@subversion.apache.org
> Subject: Re: r877014 - on Windows, invalid path => svn_node_none [was: svn
> commit: r930333 - /subversion/branches/1.6.x/STATUS]
>
> On Tue, Apr 6, 2010 at 10:53 AM, Paul Burba <pt...@gmail.com> wrote:
> > On Tue, Apr 6, 2010 at 10:22 AM, Paul Burba <pt...@gmail.com> wrote:
> >> On Tue, Apr 6, 2010 at 8:39 AM, Julian Foad <ju...@wandisco.com>
> wrote:
> >>> Hi Paul, Bert.
> >>>
> >>> I'm wondering about this r877014 change, which is proposed for 1.6.x
> >>> back-port:
> >>>
> >>> [[[
> >>> r877014 | rhuijben | 2009-04-02 14:01:48 +0100 (Thu, 02 Apr 2009) | 7
> lines
> >>>
> >>> * subversion/libsvn_subr/io.c
> >>> (io_check_path): On Windows, treat a path containing invalid characters
> as
> >>> a non-existing path. (We already detected invalid drives and invalid
> >>> network paths.) This enables locating the repository root via
> >>> svn_repos_find_root_path() in cases like:
> >>> $ svn mkdir --parents file:///G:/repos/d/f:r/s:t -m ""
> >>>
> >>> ------------------------------------------------------------------------
> >>> if (APR_STATUS_IS_ENOENT(apr_err))
> >>> *kind = svn_node_none;
> >>> - else if (APR_STATUS_IS_ENOTDIR(apr_err))
> >>> + else if (APR_STATUS_IS_ENOTDIR(apr_err)
> >>> +#if WIN32
> >>> + || (APR_TO_OS_ERROR(apr_err) == ERROR_INVALID_NAME)
> >>> +#endif
> >>> + )
> >>> *kind = svn_node_none;
> >>> else if (apr_err)
> >>> return svn_error_wrap_apr(apr_err, _("Can't check path '%s'"),
> >>> ]]]
> >>>
> >>> There seems to be a funny interdependency between io_check_path()
> and
> >>> check_repos_path() and svn_repos_find_root_path().
> >>>
> >>> io_check_path() returns 'none' if the requested entry is not on disk,
> >>> obviously, but this change now makes it also return 'none' if the name
> >>> is invalid, which it didn't do before.
> >>>
> >>> check_repos_path() says "Return TRUE on errors (which would be
> >>> permission errors, probably)", which is a rather rash assumption.
> >>
> >> Hi Julian,
> >>
> >> Never mind the rash assumption, how about the fact that this function
> >> is effectively asked "is PATH the root of a repository, yes or no?"
> >> and answers "yes" when PATH is in fact is the root of a repository and
> >> "yes*" when it is not.
> >>
> >> * "Yes it is, actually wait, we are just kidding it isn't, but don't
> >> worry, you'll find that out later!"
> >>
> >>> svn_repos_find_root_path() first checks whether the path has some
> kinds
> >>> of invalid characters:
> >>>
> >>> [[[
> >>> /* Try to decode the path, so we don't fail if it contains characters
> >>> that aren't supported by the OS filesystem. The subversion fs
> >>> isn't restricted by the OS filesystem character set. */
> >>> err = svn_utf_cstring_from_utf8(&decoded, candidate, pool);
> >>> if (!err && check_repos_path(candidate, pool))
> >>> [then return 'candidate']
> >>> ]]]
> >>>
> >>> It looks like the desired effect is being achieved by a rather oddly
> >>> layered set of assumptions and interactions in this chain of functions.
> >>
> >> Yeah, odd indeed. r877014's change to io_check_path() is really about
> >> making svn_repos_find_root_path() not choke on check_repos_path()'s
> >> bizzare semantic of returning TRUE when a path is not in fact the root
> >> of a repository. Perhaps we should fix svn_repos_find_root_path()
> >> directly?
> >
> > Bert,
> >
> > Wouldn't reverting r877014 and moving the fix into
> > repos.c:check_repos_path() like so...
> >
> > [[[
> > Index: subversion/libsvn_repos/repos.c
> >
> ==========================================================
> =========
> > --- subversion/libsvn_repos/repos.c (revision 931168)
> > +++ subversion/libsvn_repos/repos.c (working copy)
> > @@ -1417,6 +1417,13 @@
> > &kind, pool);
> > if (err)
> > {
> > +#ifdef WIN32
> > + if (APR_TO_OS_ERROR(err->apr_err) == ERROR_INVALID_NAME)
> > + {
> > + svn_error_clear(err);
> > + return FALSE;
> > + }
> > +#endif
> > svn_error_clear(err);
> > return TRUE;
> > }
> > ]]]
> >
> > ...solve the problem r877014 was intended to address without changing
> > the semantics of svn_io_check_path()?
> >
> > (trying this right now)
> >
> > Paul
> >
> >>> Changing [svn_]io_check_path() has far wider potential repercussions
> >>> than just the intended result.
> >>>
> >>> What do you think?
> >>
> >> I've changed my vote to -0. I can't point to any specific problems,
> >> but upon further reflection, I don't feel entirely comfortable saying
> >> this change won't cause other problems.
> >>
> >> Hoping Bert can weigh in on this...
>
> Hi Bert,
>
> A few questions below...
>
> From IRC:
>
> > <Bert> julianf, pburba: I see no issue with moving the check in the
> > repos code. But I think that we should avoid checking for
> > windows specific error codes there.
>
> Why not? Is there something inherently wrong with that?
>
> > (So maybe wrap the error
> > by some SVN_ERR_<new-code> for trunk.
>
> Wrap what error? svn_repos_find_root_path() and check_repos_path()
> don't return errors, they clear everything.
>
> > We can't introduce new
> > error code defines on 1.6.x)
>
> I wasn't suggesting returning an error, rather making
> repos.c:check_repos_path() return FALSE when asked if a path, with
> invalid characters in it, could be the root of a repository.
>
> > <Bert> (To busy working on non subversion things right now to look
> > into it myself)
> > <Bert> julianf: (As Windows is the only actively supported OS with
> > invalid path formats I didn't see the change of return value
> > as a big issue or something with high impact when I committed
> > the patch. (I don't think there are many callers of that
> > function that notice the small behavior change, but fixing it
> > for the repository paths is probably a safer solution))
18:47 <@pburba> Bert: Re 'wrap the error by some SVN_ERR_<new-code> for trunk. We can't introduce new error code
defines on 1.6.x'...so we wouldn't backport anything?
We can backport the code with windows specific #ifdef's in libsvn_repos, but I would recommend adding a new error code for trunk.
Or all users of our libraries have to test for specific errors using #ifdef's for platforms they might not even use. (It is hard to document that one of our functions can return an error that is not even defined on all platforms).
Bert
Re: r877014 - on Windows, invalid path => svn_node_none [was: svn
commit: r930333 - /subversion/branches/1.6.x/STATUS]
Posted by Paul Burba <pt...@gmail.com>.
On Tue, Apr 6, 2010 at 10:53 AM, Paul Burba <pt...@gmail.com> wrote:
> On Tue, Apr 6, 2010 at 10:22 AM, Paul Burba <pt...@gmail.com> wrote:
>> On Tue, Apr 6, 2010 at 8:39 AM, Julian Foad <ju...@wandisco.com> wrote:
>>> Hi Paul, Bert.
>>>
>>> I'm wondering about this r877014 change, which is proposed for 1.6.x
>>> back-port:
>>>
>>> [[[
>>> r877014 | rhuijben | 2009-04-02 14:01:48 +0100 (Thu, 02 Apr 2009) | 7 lines
>>>
>>> * subversion/libsvn_subr/io.c
>>> (io_check_path): On Windows, treat a path containing invalid characters as
>>> a non-existing path. (We already detected invalid drives and invalid
>>> network paths.) This enables locating the repository root via
>>> svn_repos_find_root_path() in cases like:
>>> $ svn mkdir --parents file:///G:/repos/d/f:r/s:t -m ""
>>>
>>> ------------------------------------------------------------------------
>>> if (APR_STATUS_IS_ENOENT(apr_err))
>>> *kind = svn_node_none;
>>> - else if (APR_STATUS_IS_ENOTDIR(apr_err))
>>> + else if (APR_STATUS_IS_ENOTDIR(apr_err)
>>> +#if WIN32
>>> + || (APR_TO_OS_ERROR(apr_err) == ERROR_INVALID_NAME)
>>> +#endif
>>> + )
>>> *kind = svn_node_none;
>>> else if (apr_err)
>>> return svn_error_wrap_apr(apr_err, _("Can't check path '%s'"),
>>> ]]]
>>>
>>> There seems to be a funny interdependency between io_check_path() and
>>> check_repos_path() and svn_repos_find_root_path().
>>>
>>> io_check_path() returns 'none' if the requested entry is not on disk,
>>> obviously, but this change now makes it also return 'none' if the name
>>> is invalid, which it didn't do before.
>>>
>>> check_repos_path() says "Return TRUE on errors (which would be
>>> permission errors, probably)", which is a rather rash assumption.
>>
>> Hi Julian,
>>
>> Never mind the rash assumption, how about the fact that this function
>> is effectively asked "is PATH the root of a repository, yes or no?"
>> and answers "yes" when PATH is in fact is the root of a repository and
>> "yes*" when it is not.
>>
>> * "Yes it is, actually wait, we are just kidding it isn't, but don't
>> worry, you'll find that out later!"
>>
>>> svn_repos_find_root_path() first checks whether the path has some kinds
>>> of invalid characters:
>>>
>>> [[[
>>> /* Try to decode the path, so we don't fail if it contains characters
>>> that aren't supported by the OS filesystem. The subversion fs
>>> isn't restricted by the OS filesystem character set. */
>>> err = svn_utf_cstring_from_utf8(&decoded, candidate, pool);
>>> if (!err && check_repos_path(candidate, pool))
>>> [then return 'candidate']
>>> ]]]
>>>
>>> It looks like the desired effect is being achieved by a rather oddly
>>> layered set of assumptions and interactions in this chain of functions.
>>
>> Yeah, odd indeed. r877014's change to io_check_path() is really about
>> making svn_repos_find_root_path() not choke on check_repos_path()'s
>> bizzare semantic of returning TRUE when a path is not in fact the root
>> of a repository. Perhaps we should fix svn_repos_find_root_path()
>> directly?
>
> Bert,
>
> Wouldn't reverting r877014 and moving the fix into
> repos.c:check_repos_path() like so...
>
> [[[
> Index: subversion/libsvn_repos/repos.c
> ===================================================================
> --- subversion/libsvn_repos/repos.c (revision 931168)
> +++ subversion/libsvn_repos/repos.c (working copy)
> @@ -1417,6 +1417,13 @@
> &kind, pool);
> if (err)
> {
> +#ifdef WIN32
> + if (APR_TO_OS_ERROR(err->apr_err) == ERROR_INVALID_NAME)
> + {
> + svn_error_clear(err);
> + return FALSE;
> + }
> +#endif
> svn_error_clear(err);
> return TRUE;
> }
> ]]]
>
> ...solve the problem r877014 was intended to address without changing
> the semantics of svn_io_check_path()?
>
> (trying this right now)
>
> Paul
>
>>> Changing [svn_]io_check_path() has far wider potential repercussions
>>> than just the intended result.
>>>
>>> What do you think?
>>
>> I've changed my vote to -0. I can't point to any specific problems,
>> but upon further reflection, I don't feel entirely comfortable saying
>> this change won't cause other problems.
>>
>> Hoping Bert can weigh in on this...
Hi Bert,
A few questions below...
>From IRC:
> <Bert> julianf, pburba: I see no issue with moving the check in the
> repos code. But I think that we should avoid checking for
> windows specific error codes there.
Why not? Is there something inherently wrong with that?
> (So maybe wrap the error
> by some SVN_ERR_<new-code> for trunk.
Wrap what error? svn_repos_find_root_path() and check_repos_path()
don't return errors, they clear everything.
> We can't introduce new
> error code defines on 1.6.x)
I wasn't suggesting returning an error, rather making
repos.c:check_repos_path() return FALSE when asked if a path, with
invalid characters in it, could be the root of a repository.
> <Bert> (To busy working on non subversion things right now to look
> into it myself)
> <Bert> julianf: (As Windows is the only actively supported OS with
> invalid path formats I didn't see the change of return value
> as a big issue or something with high impact when I committed
> the patch. (I don't think there are many callers of that
> function that notice the small behavior change, but fixing it
> for the repository paths is probably a safer solution))
Paul
Re: r877014 - on Windows, invalid path => svn_node_none [was: svn
commit: r930333 - /subversion/branches/1.6.x/STATUS]
Posted by Paul Burba <pt...@gmail.com>.
On Tue, Apr 6, 2010 at 10:53 AM, Paul Burba <pt...@gmail.com> wrote:
> On Tue, Apr 6, 2010 at 10:22 AM, Paul Burba <pt...@gmail.com> wrote:
>> On Tue, Apr 6, 2010 at 8:39 AM, Julian Foad <ju...@wandisco.com> wrote:
>>> Hi Paul, Bert.
>>>
>>> I'm wondering about this r877014 change, which is proposed for 1.6.x
>>> back-port:
>>>
>>> [[[
>>> r877014 | rhuijben | 2009-04-02 14:01:48 +0100 (Thu, 02 Apr 2009) | 7 lines
>>>
>>> * subversion/libsvn_subr/io.c
>>> (io_check_path): On Windows, treat a path containing invalid characters as
>>> a non-existing path. (We already detected invalid drives and invalid
>>> network paths.) This enables locating the repository root via
>>> svn_repos_find_root_path() in cases like:
>>> $ svn mkdir --parents file:///G:/repos/d/f:r/s:t -m ""
>>>
>>> ------------------------------------------------------------------------
>>> if (APR_STATUS_IS_ENOENT(apr_err))
>>> *kind = svn_node_none;
>>> - else if (APR_STATUS_IS_ENOTDIR(apr_err))
>>> + else if (APR_STATUS_IS_ENOTDIR(apr_err)
>>> +#if WIN32
>>> + || (APR_TO_OS_ERROR(apr_err) == ERROR_INVALID_NAME)
>>> +#endif
>>> + )
>>> *kind = svn_node_none;
>>> else if (apr_err)
>>> return svn_error_wrap_apr(apr_err, _("Can't check path '%s'"),
>>> ]]]
>>>
>>> There seems to be a funny interdependency between io_check_path() and
>>> check_repos_path() and svn_repos_find_root_path().
>>>
>>> io_check_path() returns 'none' if the requested entry is not on disk,
>>> obviously, but this change now makes it also return 'none' if the name
>>> is invalid, which it didn't do before.
>>>
>>> check_repos_path() says "Return TRUE on errors (which would be
>>> permission errors, probably)", which is a rather rash assumption.
>>
>> Hi Julian,
>>
>> Never mind the rash assumption, how about the fact that this function
>> is effectively asked "is PATH the root of a repository, yes or no?"
>> and answers "yes" when PATH is in fact is the root of a repository and
>> "yes*" when it is not.
>>
>> * "Yes it is, actually wait, we are just kidding it isn't, but don't
>> worry, you'll find that out later!"
>>
>>> svn_repos_find_root_path() first checks whether the path has some kinds
>>> of invalid characters:
>>>
>>> [[[
>>> /* Try to decode the path, so we don't fail if it contains characters
>>> that aren't supported by the OS filesystem. The subversion fs
>>> isn't restricted by the OS filesystem character set. */
>>> err = svn_utf_cstring_from_utf8(&decoded, candidate, pool);
>>> if (!err && check_repos_path(candidate, pool))
>>> [then return 'candidate']
>>> ]]]
>>>
>>> It looks like the desired effect is being achieved by a rather oddly
>>> layered set of assumptions and interactions in this chain of functions.
>>
>> Yeah, odd indeed. r877014's change to io_check_path() is really about
>> making svn_repos_find_root_path() not choke on check_repos_path()'s
>> bizzare semantic of returning TRUE when a path is not in fact the root
>> of a repository. Perhaps we should fix svn_repos_find_root_path()
>> directly?
>
> Bert,
>
> Wouldn't reverting r877014 and moving the fix into
> repos.c:check_repos_path() like so...
>
> [[[
> Index: subversion/libsvn_repos/repos.c
> ===================================================================
> --- subversion/libsvn_repos/repos.c (revision 931168)
> +++ subversion/libsvn_repos/repos.c (working copy)
> @@ -1417,6 +1417,13 @@
> &kind, pool);
> if (err)
> {
> +#ifdef WIN32
> + if (APR_TO_OS_ERROR(err->apr_err) == ERROR_INVALID_NAME)
> + {
> + svn_error_clear(err);
> + return FALSE;
> + }
> +#endif
> svn_error_clear(err);
> return TRUE;
> }
> ]]]
>
> ...solve the problem r877014 was intended to address without changing
> the semantics of svn_io_check_path()?
>
> (trying this right now)
>
> Paul
>
>>> Changing [svn_]io_check_path() has far wider potential repercussions
>>> than just the intended result.
>>>
>>> What do you think?
>>
>> I've changed my vote to -0. I can't point to any specific problems,
>> but upon further reflection, I don't feel entirely comfortable saying
>> this change won't cause other problems.
>>
>> Hoping Bert can weigh in on this...
Hi Bert,
A few questions below...
Re: r877014 - on Windows, invalid path => svn_node_none [was: svn
commit: r930333 - /subversion/branches/1.6.x/STATUS]
Posted by Paul Burba <pt...@gmail.com>.
On Tue, Apr 6, 2010 at 10:22 AM, Paul Burba <pt...@gmail.com> wrote:
> On Tue, Apr 6, 2010 at 8:39 AM, Julian Foad <ju...@wandisco.com> wrote:
>> Hi Paul, Bert.
>>
>> I'm wondering about this r877014 change, which is proposed for 1.6.x
>> back-port:
>>
>> [[[
>> r877014 | rhuijben | 2009-04-02 14:01:48 +0100 (Thu, 02 Apr 2009) | 7 lines
>>
>> * subversion/libsvn_subr/io.c
>> (io_check_path): On Windows, treat a path containing invalid characters as
>> a non-existing path. (We already detected invalid drives and invalid
>> network paths.) This enables locating the repository root via
>> svn_repos_find_root_path() in cases like:
>> $ svn mkdir --parents file:///G:/repos/d/f:r/s:t -m ""
>>
>> ------------------------------------------------------------------------
>> if (APR_STATUS_IS_ENOENT(apr_err))
>> *kind = svn_node_none;
>> - else if (APR_STATUS_IS_ENOTDIR(apr_err))
>> + else if (APR_STATUS_IS_ENOTDIR(apr_err)
>> +#if WIN32
>> + || (APR_TO_OS_ERROR(apr_err) == ERROR_INVALID_NAME)
>> +#endif
>> + )
>> *kind = svn_node_none;
>> else if (apr_err)
>> return svn_error_wrap_apr(apr_err, _("Can't check path '%s'"),
>> ]]]
>>
>> There seems to be a funny interdependency between io_check_path() and
>> check_repos_path() and svn_repos_find_root_path().
>>
>> io_check_path() returns 'none' if the requested entry is not on disk,
>> obviously, but this change now makes it also return 'none' if the name
>> is invalid, which it didn't do before.
>>
>> check_repos_path() says "Return TRUE on errors (which would be
>> permission errors, probably)", which is a rather rash assumption.
>
> Hi Julian,
>
> Never mind the rash assumption, how about the fact that this function
> is effectively asked "is PATH the root of a repository, yes or no?"
> and answers "yes" when PATH is in fact is the root of a repository and
> "yes*" when it is not.
>
> * "Yes it is, actually wait, we are just kidding it isn't, but don't
> worry, you'll find that out later!"
>
>> svn_repos_find_root_path() first checks whether the path has some kinds
>> of invalid characters:
>>
>> [[[
>> /* Try to decode the path, so we don't fail if it contains characters
>> that aren't supported by the OS filesystem. The subversion fs
>> isn't restricted by the OS filesystem character set. */
>> err = svn_utf_cstring_from_utf8(&decoded, candidate, pool);
>> if (!err && check_repos_path(candidate, pool))
>> [then return 'candidate']
>> ]]]
>>
>> It looks like the desired effect is being achieved by a rather oddly
>> layered set of assumptions and interactions in this chain of functions.
>
> Yeah, odd indeed. r877014's change to io_check_path() is really about
> making svn_repos_find_root_path() not choke on check_repos_path()'s
> bizzare semantic of returning TRUE when a path is not in fact the root
> of a repository. Perhaps we should fix svn_repos_find_root_path()
> directly?
Bert,
Wouldn't reverting r877014 and moving the fix into
repos.c:check_repos_path() like so...
[[[
Index: subversion/libsvn_repos/repos.c
===================================================================
--- subversion/libsvn_repos/repos.c (revision 931168)
+++ subversion/libsvn_repos/repos.c (working copy)
@@ -1417,6 +1417,13 @@
&kind, pool);
if (err)
{
+#ifdef WIN32
+ if (APR_TO_OS_ERROR(err->apr_err) == ERROR_INVALID_NAME)
+ {
+ svn_error_clear(err);
+ return FALSE;
+ }
+#endif
svn_error_clear(err);
return TRUE;
}
]]]
...solve the problem r877014 was intended to address without changing
the semantics of svn_io_check_path()?
(trying this right now)
Paul
>> Changing [svn_]io_check_path() has far wider potential repercussions
>> than just the intended result.
>>
>> What do you think?
>
> I've changed my vote to -0. I can't point to any specific problems,
> but upon further reflection, I don't feel entirely comfortable saying
> this change won't cause other problems.
>
> Hoping Bert can weigh in on this...
>
> Paul
Re: r877014 - on Windows, invalid path => svn_node_none [was: svn
commit: r930333 - /subversion/branches/1.6.x/STATUS]
Posted by Paul Burba <pt...@gmail.com>.
On Tue, Apr 6, 2010 at 8:39 AM, Julian Foad <ju...@wandisco.com> wrote:
> Hi Paul, Bert.
>
> I'm wondering about this r877014 change, which is proposed for 1.6.x
> back-port:
>
> [[[
> r877014 | rhuijben | 2009-04-02 14:01:48 +0100 (Thu, 02 Apr 2009) | 7 lines
>
> * subversion/libsvn_subr/io.c
> (io_check_path): On Windows, treat a path containing invalid characters as
> a non-existing path. (We already detected invalid drives and invalid
> network paths.) This enables locating the repository root via
> svn_repos_find_root_path() in cases like:
> $ svn mkdir --parents file:///G:/repos/d/f:r/s:t -m ""
>
> ------------------------------------------------------------------------
> if (APR_STATUS_IS_ENOENT(apr_err))
> *kind = svn_node_none;
> - else if (APR_STATUS_IS_ENOTDIR(apr_err))
> + else if (APR_STATUS_IS_ENOTDIR(apr_err)
> +#if WIN32
> + || (APR_TO_OS_ERROR(apr_err) == ERROR_INVALID_NAME)
> +#endif
> + )
> *kind = svn_node_none;
> else if (apr_err)
> return svn_error_wrap_apr(apr_err, _("Can't check path '%s'"),
> ]]]
>
> There seems to be a funny interdependency between io_check_path() and
> check_repos_path() and svn_repos_find_root_path().
>
> io_check_path() returns 'none' if the requested entry is not on disk,
> obviously, but this change now makes it also return 'none' if the name
> is invalid, which it didn't do before.
>
> check_repos_path() says "Return TRUE on errors (which would be
> permission errors, probably)", which is a rather rash assumption.
Hi Julian,
Never mind the rash assumption, how about the fact that this function
is effectively asked "is PATH the root of a repository, yes or no?"
and answers "yes" when PATH is in fact is the root of a repository and
"yes*" when it is not.
* "Yes it is, actually wait, we are just kidding it isn't, but don't
worry, you'll find that out later!"
> svn_repos_find_root_path() first checks whether the path has some kinds
> of invalid characters:
>
> [[[
> /* Try to decode the path, so we don't fail if it contains characters
> that aren't supported by the OS filesystem. The subversion fs
> isn't restricted by the OS filesystem character set. */
> err = svn_utf_cstring_from_utf8(&decoded, candidate, pool);
> if (!err && check_repos_path(candidate, pool))
> [then return 'candidate']
> ]]]
>
> It looks like the desired effect is being achieved by a rather oddly
> layered set of assumptions and interactions in this chain of functions.
Yeah, odd indeed. r877014's change to io_check_path() is really about
making svn_repos_find_root_path() not choke on check_repos_path()'s
bizzare semantic of returning TRUE when a path is not in fact the root
of a repository. Perhaps we should fix svn_repos_find_root_path()
directly?
> Changing [svn_]io_check_path() has far wider potential repercussions
> than just the intended result.
>
> What do you think?
I've changed my vote to -0. I can't point to any specific problems,
but upon further reflection, I don't feel entirely comfortable saying
this change won't cause other problems.
Hoping Bert can weigh in on this...
Paul
> - Julian
>
>
> On Fri, 2010-04-02, pburba@apache.org wrote:
>> * STATUS: Nominate r877014, including rhuijben's vote via IRC.
>
>> + * r877014
>> + On Windows, treat a path containing invalid characters as a non-existing
>> + path (svn_node_none) rather than raising an error.
>> + Justification:
>> + Fixes basic_tests.py 37 'use folders with names like 'c:hi'' on
>> + Windows, which has been failing over ra_local since r923779 (the
>> + reintegration of the 1.6.x-issue-3242-partial branch).
>> + Votes:
>> + +1: pburba, rhuijben (via IRC)
>> +
>
>
>
>
r877014 - on Windows, invalid path => svn_node_none [was: svn
commit: r930333 - /subversion/branches/1.6.x/STATUS]
Posted by Julian Foad <ju...@wandisco.com>.
Hi Paul, Bert.
I'm wondering about this r877014 change, which is proposed for 1.6.x
back-port:
[[[
r877014 | rhuijben | 2009-04-02 14:01:48 +0100 (Thu, 02 Apr 2009) | 7 lines
* subversion/libsvn_subr/io.c
(io_check_path): On Windows, treat a path containing invalid characters as
a non-existing path. (We already detected invalid drives and invalid
network paths.) This enables locating the repository root via
svn_repos_find_root_path() in cases like:
$ svn mkdir --parents file:///G:/repos/d/f:r/s:t -m ""
------------------------------------------------------------------------
if (APR_STATUS_IS_ENOENT(apr_err))
*kind = svn_node_none;
- else if (APR_STATUS_IS_ENOTDIR(apr_err))
+ else if (APR_STATUS_IS_ENOTDIR(apr_err)
+#if WIN32
+ || (APR_TO_OS_ERROR(apr_err) == ERROR_INVALID_NAME)
+#endif
+ )
*kind = svn_node_none;
else if (apr_err)
return svn_error_wrap_apr(apr_err, _("Can't check path '%s'"),
]]]
There seems to be a funny interdependency between io_check_path() and
check_repos_path() and svn_repos_find_root_path().
io_check_path() returns 'none' if the requested entry is not on disk,
obviously, but this change now makes it also return 'none' if the name
is invalid, which it didn't do before.
check_repos_path() says "Return TRUE on errors (which would be
permission errors, probably)", which is a rather rash assumption.
svn_repos_find_root_path() first checks whether the path has some kinds
of invalid characters:
[[[
/* Try to decode the path, so we don't fail if it contains characters
that aren't supported by the OS filesystem. The subversion fs
isn't restricted by the OS filesystem character set. */
err = svn_utf_cstring_from_utf8(&decoded, candidate, pool);
if (!err && check_repos_path(candidate, pool))
[then return 'candidate']
]]]
It looks like the desired effect is being achieved by a rather oddly
layered set of assumptions and interactions in this chain of functions.
Changing [svn_]io_check_path() has far wider potential repercussions
than just the intended result.
What do you think?
- Julian
On Fri, 2010-04-02, pburba@apache.org wrote:
> * STATUS: Nominate r877014, including rhuijben's vote via IRC.
> + * r877014
> + On Windows, treat a path containing invalid characters as a non-existing
> + path (svn_node_none) rather than raising an error.
> + Justification:
> + Fixes basic_tests.py 37 'use folders with names like 'c:hi'' on
> + Windows, which has been failing over ra_local since r923779 (the
> + reintegration of the 1.6.x-issue-3242-partial branch).
> + Votes:
> + +1: pburba, rhuijben (via IRC)
> +