You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Lieven Govaerts <lg...@mobsol.be> on 2006/10/22 19:55:01 UTC
[PATCH] Fix for broken UNC and windows path handling. (was Re: New
svn_path_is_root "X:" stuff makes Subversion sslllooowww)
Lieven Govaerts wrote:
> The new code for Windows path handling also broke our UNC support. Since
> there were no unit tests for UNC support, I wasn't even aware that we
> supported that on Windows, but apparently svn 1.4.0 does. I've added new
> unit tests for UNC support in r22067, currently they're XFailing.
>
> I propose to fix the UNC path support first while at the same time
> working on suggestion 5. To really solve the performance penalty of
> Windows path support we need to implement all of the above suggestions.
>
Attached is a patch that fixes all known issues in handling of UNC, 'X:'
and 'X:/' paths on Windows, while also removing calls to
svn_path_is_root as much as possible. It doesn't implement any of the
other suggestions as stated above.
I've spent some time this weekend expanding the path unit tests. They're
now pretty complete for *nix and Windows+Cywgin path handling, so I feel
confident attached patch is a correct and complete.solution.
Lieven.
[[[
Fix handling of UNC and Windows paths broken in r21621. As much as possible,
these fixes are implemented without using svn_path_is_root, as this is a
slow
function. Where possible, calls to svn_path_is_root have been replaced
by faster
alternatives.
* subversion/libsvn_subr/path.c
(global): add new define PATH_COMP_ENDS_WITH_SEPARATOR, checks if
a path ends with a separator ('/' or ':' on Windows)
(svn_path_join, svn_path_join_many): replace svn_path_is_root with
PATH_COMP_ENDS_WITH_SEPARATOR
(previous_segment): fix handling of UNC paths.
(svn_path_basename): replaced code now available as macro.
(svn_path_is_absolute): replace call to apr_filepath_root with a custom
and faster implementation
(get_path_ancestor_length): add support for UNC paths, extract the Windows
specific cases in platform specific code.
(svn_path_is_child): replace svn_path_is_root with
PATH_COMP_ENDS_WITH_SEPARATOR and custom handling of Windows path types.
* subversion/tests/libsvn_subr/path-test.c
(global): removed now unneeded WINDOWS_OR_CYGWIN define
(test_funcs): removed all XFail markers, tests are passing again.
]]]
Re: [PATCH] Fix for broken UNC and windows path handling. (was Re:
New svn_path_is_root "X:" stuff makes Subversion sslllooowww)
Posted by Lieven Govaerts <sv...@mobsol.be>.
I've fixed the previous patch to work on *nix.
Also, since one of the changes (reimplementation of
svn_path_is_absolute) stood on its own, I've extracted it in a separate
patch so it's probably easier to review.
Lieven.
Log message for reimplement-svn_path_is_absolute.patch.txt:
[[[
Reimplement svn_path_is_absolute. For improvement performance, don't use
the
slow apr_filepath_root function.
* subversion/libsvn_subr/path.c
(svn_path_is_absolute): replace call to apr_filepath_root with a custom
and faster implementation
]]]
Log message for windows-path-fixes.patch.txt:
[[[
Fix handling of UNC and Windows paths broken in r21621. As much as possible,
these fixes are implemented without using svn_path_is_root, as this is a
slow function. Where possible, calls to svn_path_is_root have been replaced
by faster alternatives.
* subversion/libsvn_subr/path.c
(global): add new define PATH_COMP_ENDS_WITH_SEPARATOR, checks if
a path ends with a separator ('/' or ':' on Windows)
(svn_path_join, svn_path_join_many): replace svn_path_is_root with
PATH_COMP_ENDS_WITH_SEPARATOR
(previous_segment): fix handling of UNC paths.
(svn_path_basename): replaced code now available as macro.
(get_path_ancestor_length): add support for UNC paths, extract the Windows
specific cases in platform specific code.
(svn_path_is_child): replace svn_path_is_root with
PATH_COMP_ENDS_WITH_SEPARATOR and custom handling of Windows path types.
* subversion/tests/libsvn_subr/path-test.c
(global): removed now unneeded WINDOWS_OR_CYGWIN define
(test_funcs): removed all XFail markers, tests are passing again.
]]]
> Lieven Govaerts wrote:
>
>> Lieven Govaerts wrote:
>>
>>
>>> The new code for Windows path handling also broke our UNC support. Since
>>> there were no unit tests for UNC support, I wasn't even aware that we
>>> supported that on Windows, but apparently svn 1.4.0 does. I've added new
>>> unit tests for UNC support in r22067, currently they're XFailing.
>>>
>>> I propose to fix the UNC path support first while at the same time
>>> working on suggestion 5. To really solve the performance penalty of
>>> Windows path support we need to implement all of the above suggestions.
>>>
>>>
>>>
>> Attached is a patch that fixes all known issues in handling of UNC, 'X:'
>> and 'X:/' paths on Windows, while also removing calls to
>> svn_path_is_root as much as possible. It doesn't implement any of the
>> other suggestions as stated above.
>>
>> I've spent some time this weekend expanding the path unit tests. They're
>> now pretty complete for *nix and Windows+Cywgin path handling, so I feel
>> confident attached patch is a correct and complete.solution.
>>
>> Lieven.
Re: [PATCH] Fix for broken UNC and windows path handling. (was Re:
New svn_path_is_root "X:" stuff makes Subversion sslllooowww)
Posted by Lieven Govaerts <sv...@mobsol.be>.
Buildbot shows a small issue with this patch on *nix, I'll check it out
and update the patch as soon as I have a decent internet connection.
Lieven.
Lieven Govaerts wrote:
> Lieven Govaerts wrote:
>
>> The new code for Windows path handling also broke our UNC support. Since
>> there were no unit tests for UNC support, I wasn't even aware that we
>> supported that on Windows, but apparently svn 1.4.0 does. I've added new
>> unit tests for UNC support in r22067, currently they're XFailing.
>>
>> I propose to fix the UNC path support first while at the same time
>> working on suggestion 5. To really solve the performance penalty of
>> Windows path support we need to implement all of the above suggestions.
>>
>>
> Attached is a patch that fixes all known issues in handling of UNC, 'X:'
> and 'X:/' paths on Windows, while also removing calls to
> svn_path_is_root as much as possible. It doesn't implement any of the
> other suggestions as stated above.
>
> I've spent some time this weekend expanding the path unit tests. They're
> now pretty complete for *nix and Windows+Cywgin path handling, so I feel
> confident attached patch is a correct and complete.solution.
>
> Lieven.
>
> [[[
> Fix handling of UNC and Windows paths broken in r21621. As much as possible,
> these fixes are implemented without using svn_path_is_root, as this is a
> slow
> function. Where possible, calls to svn_path_is_root have been replaced
> by faster
> alternatives.
>
> * subversion/libsvn_subr/path.c
> (global): add new define PATH_COMP_ENDS_WITH_SEPARATOR, checks if
> a path ends with a separator ('/' or ':' on Windows)
> (svn_path_join, svn_path_join_many): replace svn_path_is_root with
> PATH_COMP_ENDS_WITH_SEPARATOR
> (previous_segment): fix handling of UNC paths.
> (svn_path_basename): replaced code now available as macro.
> (svn_path_is_absolute): replace call to apr_filepath_root with a custom
> and faster implementation
> (get_path_ancestor_length): add support for UNC paths, extract the Windows
> specific cases in platform specific code.
> (svn_path_is_child): replace svn_path_is_root with
> PATH_COMP_ENDS_WITH_SEPARATOR and custom handling of Windows path types.
>
> * subversion/tests/libsvn_subr/path-test.c
> (global): removed now unneeded WINDOWS_OR_CYGWIN define
> (test_funcs): removed all XFail markers, tests are passing again.
> ]]]
>
>
> ------------------------------------------------------------------------
>
> Index: subversion/libsvn_subr/path.c
> ===================================================================
> --- subversion/libsvn_subr/path.c (revision 22083)
> +++ subversion/libsvn_subr/path.c (working copy)
> @@ -46,6 +46,14 @@
> the OS! */
> #define SVN_PATH_IS_PLATFORM_EMPTY(s,n) ((n) == 1 && (s)[0] == '.')
>
> +/* TRUE if path ends with '/' or ':' on Windows, FALSE otherwise */
> +#if defined(WIN32)
> +#define PATH_COMP_ENDS_WITH_SEPARATOR(path, len)\
> + (path[len - 1] == '/' || path[len - 1] == ':')
> +#else
> +#define PATH_COMP_ENDS_WITH_SEPARATOR(path, len) (path[len - 1] == '/')
> +#endif /* WIN32 */
> +
>
> const char *
> svn_path_internal_style(const char *path, apr_pool_t *pool)
> @@ -127,9 +135,10 @@
> if (SVN_PATH_IS_EMPTY(component))
> return apr_pmemdup(pool, base, blen + 1);
>
> + /* Add separator if the last character of base isn't '/' */
> add_separator = 1;
> - if (svn_path_is_root(base, blen, pool))
> - add_separator = 0; /* Ignore base, just return separator + component */
> + if (PATH_COMP_ENDS_WITH_SEPARATOR(base, blen))
> + add_separator = 0;
>
> /* Construct the new, combined path. */
> path = apr_palloc(pool, blen + add_separator + clen + 1);
> @@ -159,7 +168,7 @@
>
> assert(is_canonical(base, total_len));
>
> - base_is_root = svn_path_is_root(base, total_len, pool);
> + base_is_root = PATH_COMP_ENDS_WITH_SEPARATOR(base, total_len);
> if (!base_is_root &&
> (SVN_PATH_IS_EMPTY(base)))
> {
> @@ -191,7 +200,7 @@
> the total length. */
> total_len = len;
> base_arg = nargs;
> - base_is_root = svn_path_is_root(s, len, pool);
> + base_is_root = PATH_COMP_ENDS_WITH_SEPARATOR(s, len);
> base_is_empty = FALSE;
> }
> else if (nargs == base_arg
> @@ -252,8 +261,8 @@
>
> /* insert a separator if we aren't copying in the first component
> (which can happen when base_arg is set). also, don't put in a slash
> - if the prior character is a slash (occurs when prior component
> - is "/"). */
> + if the prior character is a separator (occurs when prior component
> + is "/" or "X:"). */
> if (p != path && p[-1] != '/' &&
> ! (nargs - 1 == base_arg && base_is_root))
> *p++ = '/';
> @@ -312,19 +321,22 @@
> if (len == 0)
> return 0;
>
> + if (svn_path_is_root(path, len, NULL))
> + return len;
> +
> --len;
> - while (len > 0 && path[len] != '/'
> -#if defined(WIN32)
> - && path[len] != ':'
> -#endif /* WIN32 */
> - )
> + while (len > 0 && ! PATH_COMP_ENDS_WITH_SEPARATOR(path, len))
> --len;
>
> - /* check if the remaining segment including trailing '/' is a root path */
> - if (svn_path_is_root(path, len + 1, NULL))
> - return len + 1;
> + if (len == 0)
> + return 0;
> +
> + /* if the remaining segment including trailing '/' is a root path, keep the
> + closing '/' or ':' on Windows, otherwise remove it */
> + if (svn_path_is_root(path, len, NULL))
> + return len;
> else
> - return len;
> + return len - 1;
> }
>
> void
> @@ -393,11 +405,7 @@
> else
> {
> start = len;
> - while (start > 0 && path[start - 1] != '/'
> -#if defined(WIN32)
> - && path[start - 1] != ':'
> -#endif /* WIN32 */
> - )
> + while (start > 0 && ! PATH_COMP_ENDS_WITH_SEPARATOR(path, start))
> --start;
> }
>
> @@ -477,25 +485,19 @@
> svn_boolean_t
> svn_path_is_absolute(const char *path, apr_size_t len, apr_pool_t *pool)
> {
> - const char *root_path = NULL;
> - const char *rel_path = apr_pstrmemdup(pool, path, len);
> - const char *rel_path_apr;
> + /* path is absolute if it starts with '/' */
> + if (len > 0 && path[0] == '/')
> + return TRUE;
>
> - /* svn_path_cstring_from_utf8 will create a copy of path.
> -
> - It should be safe to convert this error to a false return value. An error
> - in this case would indicate that the path isn't encoded in UTF-8, which
> - will cause problems elsewhere, anyway. */
> - svn_error_t *err = svn_path_cstring_from_utf8(&rel_path_apr, rel_path,
> - pool);
> - if (err)
> - {
> - svn_error_clear(err);
> - return FALSE;
> - }
> -
> - if (apr_filepath_root(&root_path, &rel_path_apr, 0, pool) != APR_ERELATIVE)
> + /* On Windows, path is also absolute when it starts with 'H:' or 'H:/'
> + where 'H' is any letter. */
> +#if defined (WIN32)
> + if (len >= 2 &&
> + (path[1] == ':') &&
> + ((path[0] >= 'A' && path[0] <= 'Z') ||
> + (path[0] >= 'a' && path[0] <= 'z')))
> return TRUE;
> +#endif /* WIN32 */
>
> return FALSE;
> }
> @@ -565,9 +567,16 @@
> while (path1[i] == path2[i])
> {
> /* Keep track of the last directory separator we hit. */
> +
> if (path1[i] == '/')
> - last_dirsep = i;
> -
> + {
> + last_dirsep = i;
> +#if defined(WIN32) || defined(__CYGWIN__)
> + /* for UNC paths, do not consider the second '/' as separator */
> + if (i == 1 && path1[i-1] == '/')
> + last_dirsep = 0;
> +#endif /* WIN32 or Cygwin */
> + }
> i++;
>
> /* If we get to the end of either path, break out. */
> @@ -575,6 +584,23 @@
> break;
> }
>
> + /* No match */
> + if (i == 0)
> + return 0;
> +
> +#if defined(WIN32)
> + /* H: is not a parent of H:/ */
> + if ((path1[i - 1] == ':' && path2[i] == '/') ||
> + (path2[i - 1] == ':' && path1[i] == '/'))
> + return 0;
> +#endif /* WIN32 */
> +#if defined(WIN32) || defined(__CYGWIN__)
> + /* The only shared part is a root path (X:, X:/, /, //srv/shr */
> + if (! urls && svn_path_is_root(path1, i, pool) &&
> + PATH_COMP_ENDS_WITH_SEPARATOR(path1, i))
> + return i;
> +#endif /* WIN32 or Cygwin*/
> +
> /* last_dirsep is now the offset of the last directory separator we
> crossed before reaching a non-matching byte. i is the offset of
> that non-matching byte.
> @@ -583,8 +609,7 @@
> if they have that. */
> if (((i == path1_len) && (path2[i] == '/'))
> || ((i == path2_len) && (path1[i] == '/'))
> - || ((i == path1_len) && (i == path2_len))
> - || (! urls && svn_path_is_root(path1, i, pool)))
> + || ((i == path1_len) && (i == path2_len)))
> return i;
> else
> return last_dirsep;
> @@ -694,8 +719,20 @@
> if (path1[i] == '\0' && path2[i])
> {
> if (path2[i] == '/')
> - return apr_pstrdup(pool, path2 + i + 1);
> - else if (svn_path_is_root(path1, i, pool))
> + {
> +#if defined (WIN32)
> + /* 'H:/foo' is not a child path of 'H:' */
> + if (i == 2 && path1[1] == ':')
> + return NULL;
> +#endif /* WIN32 */
> +#if defined(WIN32) || defined(__CYGWIN__)
> + /* '//srv' is not a child path of '/' */
> + if (i == 1 && path1[0] == '/')
> + return NULL;
> +#endif /* WIN32 or Cygwin */
> + return apr_pstrdup(pool, path2 + i + 1);
> + }
> + else if (PATH_COMP_ENDS_WITH_SEPARATOR(path1, i))
> return apr_pstrdup(pool, path2 + i);
> }
>
> Index: subversion/tests/libsvn_subr/path-test.c
> ===================================================================
> --- subversion/tests/libsvn_subr/path-test.c (revision 22083)
> +++ subversion/tests/libsvn_subr/path-test.c (working copy)
> @@ -1388,39 +1388,32 @@
> return SVN_NO_ERROR;
> }
>
> -/* local define to support XFail-ing tests on Windows/Cygwin only */
> -#if defined(WIN32) || defined(__CYGWIN__)
> -#define WINDOWS_OR_CYGWIN TRUE
> -#else
> -#define WINDOWS_OR_CYGWIN FALSE
> -#endif /* WIN32 or Cygwin */
> -
>
> /* The test table. */
>
> struct svn_test_descriptor_t test_funcs[] =
> {
> SVN_TEST_NULL,
> - SVN_TEST_XFAIL_COND(test_path_is_child, WINDOWS_OR_CYGWIN),
> - SVN_TEST_XFAIL_COND(test_path_split, WINDOWS_OR_CYGWIN),
> + SVN_TEST_PASS(test_path_is_child),
> + SVN_TEST_PASS(test_path_split),
> SVN_TEST_PASS(test_is_url),
> SVN_TEST_PASS(test_is_uri_safe),
> SVN_TEST_PASS(test_uri_encode),
> SVN_TEST_PASS(test_uri_decode),
> SVN_TEST_PASS(test_uri_autoescape),
> SVN_TEST_PASS(test_uri_from_iri),
> - SVN_TEST_XFAIL_COND(test_join, WINDOWS_OR_CYGWIN),
> + SVN_TEST_PASS(test_join),
> SVN_TEST_PASS(test_basename),
> - SVN_TEST_XFAIL_COND(test_dirname, WINDOWS_OR_CYGWIN),
> + SVN_TEST_PASS(test_dirname),
> SVN_TEST_PASS(test_decompose),
> SVN_TEST_PASS(test_canonicalize),
> - SVN_TEST_XFAIL_COND(test_remove_component, WINDOWS_OR_CYGWIN),
> + SVN_TEST_PASS(test_remove_component),
> SVN_TEST_PASS(test_is_root),
> SVN_TEST_PASS(test_is_absolute),
> SVN_TEST_PASS(test_path_is_ancestor),
> SVN_TEST_PASS(test_path_check_valid),
> SVN_TEST_PASS(test_is_single_path_component),
> SVN_TEST_PASS(test_compare_paths),
> - SVN_TEST_XFAIL_COND(test_get_longest_ancestor, WINDOWS_OR_CYGWIN),
> + SVN_TEST_PASS(test_get_longest_ancestor),
> SVN_TEST_NULL
> };
>
>
>
> ------------------------------------------------------------------------
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org