On Tue, Apr 6, 2010 at 10:53 AM, Paul Burba <[email protected]> wrote: > On Tue, Apr 6, 2010 at 10:22 AM, Paul Burba <[email protected]> wrote: >> On Tue, Apr 6, 2010 at 8:39 AM, Julian Foad <[email protected]> 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
