Re: [PATCH] partly fix for issue 1711: make svn_path_decompose support Windows paths

["Hyrum K. Wright" <hy...@mail.utexas.edu>]

Ping...

Is there a committer that can take a look at this?  If nothing happens
in the next few days, I'll file an issue.

-Hyrum

Lieven Govaerts wrote:
> 
> 
>> I'm trying to fix issue 1711 by making all svn_path functions support
>> Windows paths.
>>
>> Attached you'll find a patch for svn_path_decompose. Basically I've
>> reimplemented that function as a wrapper around apr_filepath_root.
>>
>> There's no new regression test because this function is currently only
>> used in the 'mucc' tool in contrib. I did expand the unit tests for
>> this function.
>>
>> There's one more function left to convert to be able to close issue
>> 1711, that'll come in the next hours/days. 
> Attached is an updated version of this patch The strings as returned by
> apr_filepath_root are now converted back to UTF-8.
> 
> Lieven.
> 
> [[[
> Make svn_path_decompose support Windows paths. Basically this is a
> reimplementation of this function, but now based on apr_filepath_root.
> 
> * subversion/libsvn_subr/path.c
>  (svn_path_decompose): make this function a wrapper around
>   apr_filepath_root
> 
> * subversion/tests/libsvn_subr/path-test.c
>  (test_decompose): added extra testcases for Window paths and special
>   characters.
> ]]]
> 
> 
> ------------------------------------------------------------------------
> 
> Index: subversion/libsvn_subr/path.c
> ===================================================================
> --- subversion/libsvn_subr/path.c	(revision 21656)
> +++ subversion/libsvn_subr/path.c	(working copy)
> @@ -732,6 +732,9 @@
>                     apr_pool_t *pool)
>  {
>    apr_size_t i, oldi;
> +  apr_status_t status;
> +  const char *rel_path, *root_path;
> +  svn_error_t *err;
>  
>    apr_array_header_t *components = 
>      apr_array_make(pool, 1, sizeof(const char *));
> @@ -741,30 +744,38 @@
>    if (SVN_PATH_IS_EMPTY(path))
>      return components;  /* ### Should we return a "" component? */
>  
> -  /* If PATH is absolute, store the '/' as the first component. */
> -  i = oldi = 0;
> -  if (path[i] == '/')
> -    {
> -      char dirsep = '/';
> +  /* Check if PATH is absolute */
> +  err = svn_path_cstring_from_utf8(&rel_path, path, pool);
> +  if (err)
> +    goto cleanup;
>  
> -      *((const char **) apr_array_push(components))
> -        = apr_pstrmemdup(pool, &dirsep, sizeof(dirsep));
> +  status = apr_filepath_root(&root_path, &rel_path, 0, pool);
> +  err = svn_path_cstring_to_utf8(&root_path, root_path, pool);
> +  if (err)
> +    goto cleanup;
>  
> -      i++;
> -      oldi++;
> -      if (path[i] == '\0') /* path is a single '/' */
> -        return components;
> -    }
> +  err = svn_path_cstring_to_utf8(&rel_path, rel_path, pool);
> +  if (err)
> +    goto cleanup;
>  
> +  /* If PATH is absolute, store the root path as the first component. */
> +  if (status != APR_ERELATIVE)
> +    *((const char **) apr_array_push(components)) = root_path;
> +
> +  if (rel_path[0] == '\0')   /* PATH is a root path */
> +    return components;
> +
> +  /* Now handle the relative part of the path */
> +  i = oldi = 0;
>    do
>      {
> -      if ((path[i] == '/') || (path[i] == '\0'))
> +      if ((rel_path[i] == '/') || (rel_path[i] == '\0'))
>          {
> -          if (SVN_PATH_IS_PLATFORM_EMPTY(path + oldi, i - oldi))
> +          if (SVN_PATH_IS_PLATFORM_EMPTY(rel_path + oldi, i - oldi))
>              *((const char **) apr_array_push(components)) = SVN_EMPTY_PATH;
>            else
>              *((const char **) apr_array_push(components))
> -              = apr_pstrmemdup(pool, path + oldi, i - oldi);
> +              = apr_pstrmemdup(pool, rel_path + oldi, i - oldi);
>  
>            i++;
>            oldi = i;  /* skipping past the dirsep */
> @@ -772,8 +783,15 @@
>          }
>        i++;
>      }
> -  while (path[i-1]);
> +  while (rel_path[i-1]);
>  
> +cleanup:
> +  if (err)
> +    {
> +      svn_error_clear(err);
> +      return NULL;
> +    }
> +
>    return components;
>  }
>  
> Index: subversion/tests/libsvn_subr/path-test.c
> ===================================================================
> --- subversion/tests/libsvn_subr/path-test.c	(revision 21656)
> +++ subversion/tests/libsvn_subr/path-test.c	(working copy)
> @@ -710,9 +710,19 @@
>      "/foo", "/", "foo", NULL,
>      "/foo/bar", "/", "foo", "bar", NULL,
>      "foo/bar", "foo", "bar", NULL,
> +    "/abç/déf", "/", "abç", "déf", NULL,
>  
>      /* Are these canonical? Should the middle bits produce SVN_EMPTY_PATH? */
>      "foo/bar", "foo", "bar", NULL,
> +#if defined(WIN32)
> +    "X:/", "X:/", NULL,
> +    "X:/abc", "X:/", "abc", NULL,
> +    "X:/abc/def", "X:/", "abc", "def", NULL,
> +    "X:", "X:", NULL,
> +    "X:abc", "X:", "abc", NULL,
> +    "X:abc/def", "X:", "abc", "def", NULL,
> +    "X:abç/déf", "X:", "abç", "déf", NULL,
> +#endif
>      NULL,
>    };
>    int i = 0;