[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:53:51 -0400

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...
>
> Paul
Received on 2010-04-06 16:54:22 CEST

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