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
Received on Sun Oct 22 23:06:30 2006