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.
Index: subversion/libsvn_subr/path.c
===================================================================
--- subversion/libsvn_subr/path.c (revision 22087)
+++ subversion/libsvn_subr/path.c (working copy)
@@ -477,26 +477,20 @@
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;
-
- /* 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)
+ /* path is absolute if it starts with '/' */
+ if (len > 0 && path[0] == '/')
return TRUE;
-
+
+ /* 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;
}
Index: subversion/libsvn_subr/path.c
===================================================================
--- subversion/libsvn_subr/path.c (revision 22087)
+++ 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;
}
@@ -565,9 +573,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[0] == '/')
+ last_dirsep = 0;
+#endif /* WIN32 or Cygwin */
+ }
i++;
/* If we get to the end of either path, break out. */
@@ -575,16 +590,30 @@
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.
-
- If these are folders, return their common root folder '/' or 'H:/'
- if they have that. */
+ that non-matching byte. */
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)))
+ || (! urls && (i == 1) && (path1[0] == '/')))
return i;
else
return last_dirsep;
@@ -694,8 +723,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 22089)
+++ subversion/tests/libsvn_subr/path-test.c (working copy)
@@ -1390,39 +1390,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
Received on Mon Oct 23 23:27:00 2006