[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 15:59:05 -0400

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))

Paul
Received on 2010-04-06 21:59:31 CEST

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