[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: Paul Burba <ptburba_at_gmail.com>
Date: Tue, 6 Apr 2010 10:22:39 -0400

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?

> 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, pburba_at_apache.org 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)
>> +
>
>
>
>
Received on 2010-04-06 16:23:15 CEST

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