> -----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
Received on 2010-04-07 11:09:58 CEST