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

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;