[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: Bert Huijben <bert_at_qqmail.nl>
Date: Wed, 7 Apr 2010 15:46:57 +0200

> -----Original Message-----
> From: Paul Burba [mailto:ptburba_at_gmail.com]
> Sent: woensdag 7 april 2010 15:27
> To: Bert Huijben
> Cc: Julian Foad; rhuijben_at_apache.org; dev_at_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 <bert_at_qqmail.nl> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Paul Burba [mailto:ptburba_at_gmail.com]
> >> Sent: dinsdag 6 april 2010 21:59
> >> To: Julian Foad
> >> Cc: rhuijben_at_apache.org; dev_at_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 <ptburba_at_gmail.com>
> wrote:
> >> > On Tue, Apr 6, 2010 at 10:22 AM, Paul Burba <ptburba_at_gmail.com>
> wrote:
> >> >> On Tue, Apr 6, 2010 at 8:39 AM, Julian Foad
> <julian.foad_at_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
Received on 2010-04-07 15:47:30 CEST

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

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.