[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] Fix for broken UNC and windows path handling. (was Re: New svn_path_is_root "X:" stuff makes Subversion sslllooowww)

From: Lieven Govaerts <svnlgo_at_mobsol.be>
Date: 2006-10-23 23:25:28 CEST

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

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