> -----Original Message-----
> From: Bert Huijben [mailto:bert_at_qqmail.nl]
> Sent: woensdag 7 april 2010 11:09
> To: 'Paul Burba'; '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]
>
>
>
> > -----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).
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
Received on 2010-04-07 14:18:27 CEST