You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2010/11/30 14:43:09 UTC
Re: svn commit: r1039398 - in /subversion/trunk/subversion:
include/svn_dirent_uri.h libsvn_delta/path_driver.c
libsvn_subr/dirent_uri.c tests/libsvn_subr/dirent_uri-test.c
Hi Julian,
julianfoad@apache.org wrote on Fri, Nov 26, 2010 at 14:58:17 -0000:
> Author: julianfoad
> Date: Fri Nov 26 14:58:17 2010
> New Revision: 1039398
>
> URL: http://svn.apache.org/viewvc?rev=1039398&view=rev
> Log:
> Add some more svn_fspath__* APIs, with tests for some of them. These tests
> pass both with and without FSPATH_USE_URI defined.
>
> * subversion/include/svn_dirent_uri.h,
> subversion/libsvn_subr/dirent_uri.c
> (svn_fspath__dirname, svn_fspath__split, svn_fspath__skip_ancestor,
> svn_fspath__is_ancestor, svn_fspath__get_longest_ancestor): New functions.
>
> * subversion/libsvn_delta/path_driver.c
> (svn_fspath__get_longest_ancestor): Remove this temporary macro.
>
> * subversion/tests/libsvn_subr/dirent_uri-test.c
> (test_fspath_basename): Rename and extend to become ...
> (test_fspath_dirname_basename_split): ... this new test.
> (test_fspath_get_longest_ancestor): New tests.
> (test_funcs): Add the new tests.
> ### TODO: Tests.
>
What is that ### comment? Is it the mailer saying you should have added
a few more tests to this commit?
Daniel
> Modified:
> subversion/trunk/subversion/include/svn_dirent_uri.h
> subversion/trunk/subversion/libsvn_delta/path_driver.c
> subversion/trunk/subversion/libsvn_subr/dirent_uri.c
> subversion/trunk/subversion/tests/libsvn_subr/dirent_uri-test.c
>
> Modified: subversion/trunk/subversion/include/svn_dirent_uri.h
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_dirent_uri.h?rev=1039398&r1=1039397&r2=1039398&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/include/svn_dirent_uri.h (original)
> +++ subversion/trunk/subversion/include/svn_dirent_uri.h Fri Nov 26 14:58:17 2010
> @@ -812,12 +812,12 @@ svn_uri_get_file_url_from_dirent(const c
> apr_pool_t *pool);
>
>
> -/**
> +/**************************************************************************
> * A private API for Subversion filesystem paths, otherwise known as paths
> * within a repository, that start with a '/'. The rules for a fspath are
> * the same as for a relpath except for the leading '/'. A fspath never
> * ends with '/' except when the whole path is just '/'.
> - */
> + **************************************************************************/
>
> /** Return TRUE iff @a fspath is canonical.
> * @a fspath need not be canonical, of course.
> @@ -827,6 +827,49 @@ svn_uri_get_file_url_from_dirent(const c
> svn_boolean_t
> svn_fspath__is_canonical(const char *fspath);
>
> +
> +/** Return the dirname of @a fspath, defined as the path with its basename
> + * removed. If @a fspath is "/", return "/".
> + *
> + * Allocate the result in @a pool.
> + *
> + * @since New in 1.7.
> + */
> +const char *
> +svn_fspath__dirname(const char *fspath,
> + apr_pool_t *pool);
> +
> +/** Return the last component of @a fspath. The returned value will have no
> + * slashes in it. If @a fspath is "/", return "".
> + *
> + * If @a pool is NULL, return a pointer to within @a fspath, else allocate
> + * the result in @a pool.
> + *
> + * @since New in 1.7.
> + */
> +const char *
> +svn_fspath__basename(const char *fspath,
> + apr_pool_t *pool);
> +
> +/** Divide the canonical @a fspath into @a *dirpath and @a
> + * *base_name, allocated in @a pool.
> + *
> + * If @a dirpath or @a base_name is NULL, then don't set that one.
> + *
> + * Either @a dirpath or @a base_name may be @a fspath's own address, but they
> + * may not both be the same address, or the results are undefined.
> + *
> + * If @a fspath has two or more components, the separator between @a dirpath
> + * and @a base_name is not included in either of the new names.
> + *
> + * @since New in 1.7.
> + */
> +void
> +svn_fspath__split(const char **dirpath,
> + const char **base_name,
> + const char *fspath,
> + apr_pool_t *result_pool);
> +
> /** Return the fspath composed of @a fspath with @a relpath appended.
> * Allocate the result in @a result_pool.
> *
> @@ -855,16 +898,37 @@ svn_fspath__is_child(const char *parent_
> const char *child_fspath,
> apr_pool_t *pool);
>
> -
> -/** Return the last component of @a fspath. The returned value will have no
> - * slashes in it. If @a pool is NULL, return a pointer to within @a fspath,
> - * else allocate the result in @a pool.
> +/** Return the relative path part of @a child_fspath that is below
> + * @a parent_fspath, or just "" if @a parent_fspath is equal to
> + * @a child_fspath. If @a child_fspath is not below @a parent_relpath,
> + * return @a child_fspath.
> + *
> + * ### Returning the child in the no-match case is a bad idea.
> *
> * @since New in 1.7.
> */
> const char *
> -svn_fspath__basename(const char *fspath,
> - apr_pool_t *pool);
> +svn_fspath__skip_ancestor(const char *parent_fspath,
> + const char *child_fspath);
> +
> +/** Return TRUE if @a parent_fspath is an ancestor of @a child_fspath or
> + * the fspaths are equal, and FALSE otherwise.
> + *
> + * @since New in 1.7.
> + */
> +svn_boolean_t
> +svn_fspath__is_ancestor(const char *parent_fspath,
> + const char *child_fspath);
> +
> +/** Return the longest common path shared by two fspaths, @a fspath1 and
> + * @a fspath2. If there's no common ancestor, return "/".
> + *
> + * @since New in 1.7.
> + */
> +char *
> +svn_fspath__get_longest_ancestor(const char *fspath1,
> + const char *fspath2,
> + apr_pool_t *result_pool);
>
>
> #ifdef __cplusplus
>
> Modified: subversion/trunk/subversion/libsvn_delta/path_driver.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_delta/path_driver.c?rev=1039398&r1=1039397&r2=1039398&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_delta/path_driver.c (original)
> +++ subversion/trunk/subversion/libsvn_delta/path_driver.c Fri Nov 26 14:58:17 2010
> @@ -129,14 +129,6 @@ count_components(const char *path)
>
>
> /*** Public interfaces ***/
> -
> -/* ### Temporary definition, until this is available in dirent_uri.h */
> -#define svn_fspath__get_longest_ancestor(path1, path2, pool) \
> - apr_pstrcat((pool), \
> - "/", \
> - svn_relpath_get_longest_ancestor((path1)+1, (path2)+1, (pool)), \
> - NULL)
> -
> svn_error_t *
> svn_delta_path_driver(const svn_delta_editor_t *editor,
> void *edit_baton,
> @@ -225,7 +217,7 @@ svn_delta_path_driver(const svn_delta_ed
> /*** Step C - Open any directories between the common ancestor
> and the parent of the current path. ***/
> if (*path == '/')
> - svn_uri_split(&pdir, &bname, path, iterpool);
> + svn_fspath__split(&pdir, &bname, path, iterpool);
> else
> svn_relpath_split(&pdir, &bname, path, iterpool);
> if (strlen(pdir) > common_len)
>
> Modified: subversion/trunk/subversion/libsvn_subr/dirent_uri.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/dirent_uri.c?rev=1039398&r1=1039397&r2=1039398&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_subr/dirent_uri.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/dirent_uri.c Fri Nov 26 14:58:17 2010
> @@ -2475,47 +2475,78 @@ svn_fspath__is_canonical(const char *fsp
> return fspath[0] == '/' && relpath_is_canonical(fspath + 1);
> }
>
> -char *
> -svn_fspath__join(const char *fspath,
> - const char *relpath,
> - apr_pool_t *result_pool)
> +
> +const char *
> +svn_fspath__is_child(const char *parent_fspath,
> + const char *child_fspath,
> + apr_pool_t *pool)
> {
> - char *result;
> - assert(svn_fspath__is_canonical(fspath));
> - assert(relpath_is_canonical(relpath));
> + const char *result;
> + assert(svn_fspath__is_canonical(parent_fspath));
> + assert(svn_fspath__is_canonical(child_fspath));
>
> #ifdef FSPATH_USE_URI
> - result = svn_uri_join(fspath, relpath, result_pool);
> + result = svn_uri_is_child(parent_fspath, child_fspath, pool);
> #else
> - if (relpath[0] == '\0')
> - result = apr_pstrdup(result_pool, fspath);
> - else if (fspath[1] == '\0')
> - result = apr_pstrcat(result_pool, "/", relpath, (char *)NULL);
> - else
> - result = apr_pstrcat(result_pool, fspath, "/", relpath, (char *)NULL);
> + result = svn_relpath_is_child(parent_fspath + 1, child_fspath + 1, pool);
> #endif
>
> - assert(svn_fspath__is_canonical(result));
> + assert(result == NULL || svn_relpath_is_canonical(result, pool));
> return result;
> }
>
> -
> const char *
> -svn_fspath__is_child(const char *parent_fspath,
> - const char *child_fspath,
> - apr_pool_t *pool)
> +svn_fspath__skip_ancestor(const char *parent_fspath,
> + const char *child_fspath)
> {
> const char *result;
> assert(svn_fspath__is_canonical(parent_fspath));
> assert(svn_fspath__is_canonical(child_fspath));
>
> #ifdef FSPATH_USE_URI
> - result = svn_uri_is_child(parent_fspath, child_fspath, pool);
> + result = svn_uri_skip_ancestor(parent_fspath, child_fspath);
> #else
> - result = svn_relpath_is_child(parent_fspath + 1, child_fspath + 1, pool);
> + if (svn_relpath_is_ancestor(parent_fspath + 1, child_fspath + 1))
> + result = svn_relpath_skip_ancestor(parent_fspath + 1, child_fspath + 1);
> + else
> + result = child_fspath;
> +#endif
> +
> + assert(svn_relpath_is_canonical(result, NULL)
> + || strcmp(result, child_fspath) == 0);
> + return result;
> +}
> +
> +svn_boolean_t
> +svn_fspath__is_ancestor(const char *parent_fspath,
> + const char *child_fspath)
> +{
> + assert(svn_fspath__is_canonical(parent_fspath));
> + assert(svn_fspath__is_canonical(child_fspath));
> +
> +#ifdef FSPATH_USE_URI
> + return svn_uri_is_ancestor(parent_fspath, child_fspath);
> +#else
> + return svn_relpath_is_ancestor(parent_fspath + 1, child_fspath + 1);
> +#endif
> +}
> +
> +
> +const char *
> +svn_fspath__dirname(const char *fspath,
> + apr_pool_t *pool)
> +{
> + const char *result;
> + assert(svn_fspath__is_canonical(fspath));
> +
> +#ifdef FSPATH_USE_URI
> + result = svn_uri_dirname(fspath, pool);
> +#else
> + result = apr_pstrcat(pool, "/", svn_relpath_dirname(fspath + 1, pool),
> + (char *)NULL);
> #endif
>
> - assert(result == NULL || relpath_is_canonical(result));
> + assert(svn_fspath__is_canonical(result));
> return result;
> }
>
> @@ -2537,4 +2568,64 @@ svn_fspath__basename(const char *fspath,
> return result;
> }
>
> +void
> +svn_fspath__split(const char **dirpath,
> + const char **base_name,
> + const char *fspath,
> + apr_pool_t *result_pool)
> +{
> + assert(dirpath != base_name);
> +
> + if (dirpath)
> + *dirpath = svn_fspath__dirname(fspath, result_pool);
> +
> + if (base_name)
> + *base_name = svn_fspath__basename(fspath, result_pool);
> +}
> +
> +char *
> +svn_fspath__join(const char *fspath,
> + const char *relpath,
> + apr_pool_t *result_pool)
> +{
> + char *result;
> + assert(svn_fspath__is_canonical(fspath));
> + assert(svn_relpath_is_canonical(relpath, result_pool));
> +
> +#ifdef FSPATH_USE_URI
> + result = svn_uri_join(fspath, relpath, result_pool);
> +#else
> + if (relpath[0] == '\0')
> + result = apr_pstrdup(result_pool, fspath);
> + else if (fspath[1] == '\0')
> + result = apr_pstrcat(result_pool, "/", relpath, (char *)NULL);
> + else
> + result = apr_pstrcat(result_pool, fspath, "/", relpath, (char *)NULL);
> +#endif
> +
> + assert(svn_fspath__is_canonical(result));
> + return result;
> +}
> +
> +char *
> +svn_fspath__get_longest_ancestor(const char *fspath1,
> + const char *fspath2,
> + apr_pool_t *result_pool)
> +{
> + char *result;
> + assert(svn_fspath__is_canonical(fspath1));
> + assert(svn_fspath__is_canonical(fspath2));
>
> +#ifdef FSPATH_USE_URI
> + result = svn_uri_get_longest_ancestor(fspath1, fspath2, result_pool);
> +#else
> + result = apr_pstrcat(result_pool, "/",
> + svn_relpath_get_longest_ancestor(fspath1 + 1,
> + fspath2 + 1,
> + result_pool),
> + NULL);
> +#endif
> +
> + assert(svn_fspath__is_canonical(result));
> + return result;
> +}
>
> Modified: subversion/trunk/subversion/tests/libsvn_subr/dirent_uri-test.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_subr/dirent_uri-test.c?rev=1039398&r1=1039397&r2=1039398&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/tests/libsvn_subr/dirent_uri-test.c (original)
> +++ subversion/trunk/subversion/tests/libsvn_subr/dirent_uri-test.c Fri Nov 26 14:58:17 2010
> @@ -2961,30 +2961,95 @@ test_fspath_is_child(apr_pool_t *pool)
> }
>
> static svn_error_t *
> -test_fspath_basename(apr_pool_t *pool)
> +test_fspath_dirname_basename_split(apr_pool_t *pool)
> {
> int i;
>
> - struct {
> + static const struct {
> const char *path;
> - const char *result;
> + const char *dirname;
> + const char *basename;
> } tests[] = {
> - { "/", "" },
> - { "/a", "a" },
> - { "/abc", "abc" },
> - { "/x/abc", "abc" },
> + { "/", "/", "" },
> + { "/a", "/", "a" },
> + { "/abc", "/", "abc" },
> + { "/x/abc", "/x", "abc" },
> + { "/x/y/abc", "/x/y", "abc" },
> };
>
> for (i = 0; i < COUNT_OF(tests); i++)
> {
> - const char *result = svn_fspath__basename(tests[i].path, pool);
> + const char *result_dirname, *result_basename;
> +
> + result_dirname = svn_fspath__dirname(tests[i].path, pool);
> + SVN_TEST_STRING_ASSERT(result_dirname, tests[i].dirname);
>
> - SVN_TEST_STRING_ASSERT(result, tests[i].result);
> + result_basename = svn_fspath__basename(tests[i].path, pool);
> + SVN_TEST_STRING_ASSERT(result_basename, tests[i].basename);
> +
> + svn_fspath__split(&result_dirname, &result_basename, tests[i].path,
> + pool);
> + SVN_TEST_STRING_ASSERT(result_dirname, tests[i].dirname);
> + SVN_TEST_STRING_ASSERT(result_basename, tests[i].basename);
> }
>
> return SVN_NO_ERROR;
> }
>
> +static svn_error_t *
> +test_fspath_get_longest_ancestor(apr_pool_t *pool)
> +{
> + int i;
> +
> + /* Paths to test and their expected results. Same as in
> + * test_relpath_get_longest_ancestor() but with '/' prefix. */
> + struct {
> + const char *path1;
> + const char *path2;
> + const char *result;
> + } tests[] = {
> + { "/foo", "/foo/bar", "/foo" },
> + { "/foo/bar", "/foo/bar", "/foo/bar" },
> + { "/", "/foo", "/" },
> + { "/", "/foo", "/" },
> + { "/", "/.bar", "/" },
> + { "/.bar", "/", "/" },
> + { "/foo/bar", "/foo", "/foo" },
> + { "/foo/bar", "/foo", "/foo" },
> + { "/rif", "/raf", "/" },
> + { "/foo", "/bar", "/" },
> + { "/foo", "/foo/bar", "/foo" },
> + { "/foo.", "/foo./.bar", "/foo." },
> + { "/", "/", "/" },
> + { "/http:/test", "/http:/test", "/http:/test" },
> + { "/http:/test", "/http:/taste", "/http:" },
> + { "/http:/test", "/http:/test/foo", "/http:/test" },
> + { "/http:/test", "/file:/test/foo", "/" },
> + { "/http:/test", "/http:/testF", "/http:" },
> + { "/file:/A/C", "/file:/B/D", "/file:" },
> + { "/file:/A/C", "/file:/A/D", "/file:/A" },
> + { "/X:/foo", "/X:", "/X:" },
> + { "/X:/folder1", "/X:/folder2", "/X:" },
> + { "/X:", "/X:foo", "/" },
> + { "/X:foo", "/X:bar", "/" },
> + };
> +
> + for (i = 0; i < COUNT_OF(tests); i++)
> + {
> + const char *result;
> +
> + result = svn_fspath__get_longest_ancestor(tests[i].path1, tests[i].path2,
> + pool);
> + SVN_TEST_STRING_ASSERT(tests[i].result, result);
> +
> + /* changing the order of the paths should return the same result */
> + result = svn_fspath__get_longest_ancestor(tests[i].path2, tests[i].path1,
> + pool);
> + SVN_TEST_STRING_ASSERT(tests[i].result, result);
> + }
> + return SVN_NO_ERROR;
> +}
> +
>
> /* The test table. */
>
> @@ -3091,7 +3156,9 @@ struct svn_test_descriptor_t test_funcs[
> "test svn_fspath__join"),
> SVN_TEST_PASS2(test_fspath_is_child,
> "test svn_fspath__is_child"),
> - SVN_TEST_PASS2(test_fspath_basename,
> - "test svn_fspath__basename"),
> + SVN_TEST_PASS2(test_fspath_dirname_basename_split,
> + "test svn_fspath__dirname/basename/split"),
> + SVN_TEST_PASS2(test_fspath_get_longest_ancestor,
> + "test svn_fspath__get_longest_ancestor"),
> SVN_TEST_NULL
> };
>
>
Re: svn commit: r1039398 - in /subversion/trunk/subversion:
include/svn_dirent_uri.h libsvn_delta/path_driver.c
libsvn_subr/dirent_uri.c tests/libsvn_subr/dirent_uri-test.c
Posted by Julian Foad <ju...@wandisco.com>.
Daniel Shahaf wrote:
> > Add some more svn_fspath__* APIs, with tests for some of them. These tests
> > pass both with and without FSPATH_USE_URI defined.
> >
> > * subversion/include/svn_dirent_uri.h,
> > subversion/libsvn_subr/dirent_uri.c
> > (svn_fspath__dirname, svn_fspath__split, svn_fspath__skip_ancestor,
> > svn_fspath__is_ancestor, svn_fspath__get_longest_ancestor): New functions.
> >
> > * subversion/libsvn_delta/path_driver.c
> > (svn_fspath__get_longest_ancestor): Remove this temporary macro.
> >
> > * subversion/tests/libsvn_subr/dirent_uri-test.c
> > (test_fspath_basename): Rename and extend to become ...
> > (test_fspath_dirname_basename_split): ... this new test.
> > (test_fspath_get_longest_ancestor): New tests.
> > (test_funcs): Add the new tests.
> > ### TODO: Tests.
> >
>
> What is that ### comment? Is it the mailer saying you should have added
> a few more tests to this commit?
Heh! I didn't intend to commit that, but it was me reminding myself
that I ought to write tests for the remaining functions: skip_ancestor
and is_ancestor. In the end I decided I was OK with committing it
without (or before) writing tests for those.
Tests are important ... but somehow those are way down my priority list.
Thanks for noticing. I've edited the log message to clarify.
- Julian