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? > 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, [email protected] 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) >> + > > > >
