You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Lieven Govaerts <sv...@mobsol.be> on 2006/09/10 00:03:07 UTC
[PATCH] added support for 'X:' paths on Windows (issue 1711)
Hi,
as part of the complete solution for issue #1711 this patch adds support
for X: paths. Previous patches already added support for working copies
on 'X:/' (root of a drive).
The patch also contains lots of extra testcases for the path functions,
I've added tests for all impacted functions in this patch.
Lieven.
[[[
Follow up change for issue #1711: add support for 'X:' paths. On Windows,
'X:' is a symbolic link to the current working directory on the X drive.
Added unit tests for all the impacted path functions.
* subversion/libsvn_subr/path.c
(svn_path_join_many, previous_segment,
svn_path_basename, svn_path_is_ancestor): small fixes to support X:
paths on Windows
* subversion/tests/libsvn_subr/path-test.c
(test_join, test_canonicalize, test_remove_component,
test_is_root, ): added extra test cases for Windows paths (X: and X:/)
(test_dirname): new test for svn_path_dirname
(test_path_is_ancestor): new test for svn_path_is_ancestor
(test_funcs): run the two new tests
* subversion/tests/cmdline/update_tests.py
(update_wc_on_windows_drive): expand the test with a testcase for X:
path support
* subversion/tests/cmdline/svntest/tree.py
(create_from_path): on Windows, the 'X:' in 'X:foo' is a folder, so
consider the ':' in the first element of a path as a folder separator
on that platform.
]]]
Re: [PATCH] added support for 'X:' paths on Windows (issue 1711)
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
svnlgo@mobsol.be wrote:
> Hyrum,
>
>
> this is a patch for issue 1711, so you can use that ticket.
>
> It's too bad nobody has time to review this patch though. It's the last part
> from a series of patches to handle both issue 1711 and issue 2556. As far as
> I'm concerned both issues can be closed after committing this patch.
>
> I know it's a large patch and it's Windows only, both more than half of the
> patch are much needed extra C and Python testcases.
>
> Anyway, thanks for bringing it back under attention.
No problem. I've attached it to isseu 1711, including your note about
fixing 2556 as well.
-Hyrum
Re: [PATCH] added support for 'X:' paths on Windows (issue 1711)
Posted by Lieven Govaerts <sv...@mobsol.be>.
D.J. Heap wrote:
> On 9/21/06, Daniel Rall <dl...@collab.net> wrote:
>> On Tue, 19 Sep 2006, svnlgo@mobsol.be wrote:
>> ...
>> > this is a patch for issue 1711, so you can use that ticket.
>> >
>> > It's too bad nobody has time to review this patch though. It's the
>> last part
>> > from a series of patches to handle both issue 1711 and issue 2556.
>> As far as
>> > I'm concerned both issues can be closed after committing this patch.
>> >
>> > I know it's a large patch and it's Windows only, both more than
>> half of the
>> > patch are much needed extra C and Python testcases.
>>
>> I committed two small refactorings from this patch which are unrelated
>> to issue 17111, and not Windows-specific. The patch looks pretty good
>> to me, but I lack a Windows dev environment in which to test it.
>>
>
>
> I reviewed and tested this and it looks good, +1 to commit. Thanks
> Lieven!
>
> DJ
Committed in r21621, thanks for the review!
Lieven.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] added support for 'X:' paths on Windows (issue 1711)
Posted by "D.J. Heap" <dj...@gmail.com>.
On 9/21/06, Daniel Rall <dl...@collab.net> wrote:
> On Tue, 19 Sep 2006, svnlgo@mobsol.be wrote:
> ...
> > this is a patch for issue 1711, so you can use that ticket.
> >
> > It's too bad nobody has time to review this patch though. It's the last part
> > from a series of patches to handle both issue 1711 and issue 2556. As far as
> > I'm concerned both issues can be closed after committing this patch.
> >
> > I know it's a large patch and it's Windows only, both more than half of the
> > patch are much needed extra C and Python testcases.
>
> I committed two small refactorings from this patch which are unrelated
> to issue 17111, and not Windows-specific. The patch looks pretty good
> to me, but I lack a Windows dev environment in which to test it.
>
I reviewed and tested this and it looks good, +1 to commit. Thanks Lieven!
DJ
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] added support for 'X:' paths on Windows (issue 1711)
Posted by Daniel Rall <dl...@collab.net>.
On Tue, 19 Sep 2006, svnlgo@mobsol.be wrote:
...
> this is a patch for issue 1711, so you can use that ticket.
>
> It's too bad nobody has time to review this patch though. It's the last part
> from a series of patches to handle both issue 1711 and issue 2556. As far as
> I'm concerned both issues can be closed after committing this patch.
>
> I know it's a large patch and it's Windows only, both more than half of the
> patch are much needed extra C and Python testcases.
I committed two small refactorings from this patch which are unrelated
to issue 17111, and not Windows-specific. The patch looks pretty good
to me, but I lack a Windows dev environment in which to test it.
Re: [PATCH] added support for 'X:' paths on Windows (issue 1711)
Posted by sv...@mobsol.be.
Hyrum,
this is a patch for issue 1711, so you can use that ticket.
It's too bad nobody has time to review this patch though. It's the last part
from a series of patches to handle both issue 1711 and issue 2556. As far as
I'm concerned both issues can be closed after committing this patch.
I know it's a large patch and it's Windows only, both more than half of the
patch are much needed extra C and Python testcases.
Anyway, thanks for bringing it back under attention.
Lieven.
Quoting "Hyrum K. Wright" <hy...@mail.utexas.edu>:
> Ping...Has anybody had a chance to look at this? If nothing happens in
> the next few days, I'll open an issue.
>
> -Hyrum
>
> Lieven Govaerts wrote:
> > Hi,
> >
> > as part of the complete solution for issue #1711 this patch adds support
> > for X: paths. Previous patches already added support for working copies
> > on 'X:/' (root of a drive).
> >
> > The patch also contains lots of extra testcases for the path functions,
> > I've added tests for all impacted functions in this patch.
> >
> > Lieven.
> >
> >
> > [[[
> > Follow up change for issue #1711: add support for 'X:' paths. On Windows,
> > 'X:' is a symbolic link to the current working directory on the X drive.
> >
> > Added unit tests for all the impacted path functions.
> >
> > * subversion/libsvn_subr/path.c
> > (svn_path_join_many, previous_segment,
> > svn_path_basename, svn_path_is_ancestor): small fixes to support X:
> > paths on Windows
> >
> > * subversion/tests/libsvn_subr/path-test.c
> > (test_join, test_canonicalize, test_remove_component,
> > test_is_root, ): added extra test cases for Windows paths (X: and X:/)
> > (test_dirname): new test for svn_path_dirname
> > (test_path_is_ancestor): new test for svn_path_is_ancestor
> > (test_funcs): run the two new tests
> >
> > * subversion/tests/cmdline/update_tests.py
> > (update_wc_on_windows_drive): expand the test with a testcase for X:
> > path support
> >
> > * subversion/tests/cmdline/svntest/tree.py
> > (create_from_path): on Windows, the 'X:' in 'X:foo' is a folder, so
> > consider the ':' in the first element of a path as a folder separator
> > on that platform.
> > ]]]
> >
> >
> > ------------------------------------------------------------------------
> >
> > Index: subversion/libsvn_subr/path.c
> > ===================================================================
> > --- subversion/libsvn_subr/path.c (revision 21396)
> > +++ subversion/libsvn_subr/path.c (working copy)
> > @@ -185,13 +185,13 @@
> > if (nargs++ < MAX_SAVED_LENGTHS)
> > saved_lengths[nargs] = len;
> >
> > - if (svn_path_is_absolute(s, strlen(s), pool))
> > + if (svn_path_is_absolute(s, len, pool))
> > {
> > /* an absolute path. skip all components to this point and reset
> > the total length. */
> > total_len = len;
> > base_arg = nargs;
> > - base_is_root = len == 1;
> > + base_is_root = svn_path_is_root(s, len, pool);
> > base_is_empty = FALSE;
> > }
> > else if (nargs == base_arg
> > @@ -254,7 +254,8 @@
> > (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 (p != path && p[-1] != '/')
> > + if (p != path && p[-1] != '/' &&
> > + ! (nargs - 1 == base_arg && base_is_root))
> > *p++ = '/';
> >
> > /* copy the new component and advance the pointer */
> > @@ -311,8 +312,13 @@
> > if (len == 0)
> > return 0;
> >
> > - while (len > 0 && path[--len] != '/')
> > - ;
> > + --len;
> > + while (len > 0 && path[len] != '/'
> > +#if defined(WIN32)
> > + && path[len] != ':'
> > +#endif /* WIN32 */
> > + )
> > + --len;
> >
> > /* check if the remaining segment including trailing '/' is a root path
> */
> > if (svn_path_is_root(path, len + 1, NULL))
> > @@ -387,7 +393,11 @@
> > else
> > {
> > start = len;
> > - while (start > 0 && path[start - 1] != '/')
> > + while (start > 0 && path[start - 1] != '/'
> > +#if defined(WIN32)
> > + && path[start - 1] != ':'
> > +#endif /* WIN32 */
> > + )
> > --start;
> > }
> >
> > @@ -698,19 +708,22 @@
> > {
> > apr_size_t path1_len = strlen(path1);
> >
> > - /* If path1 is empty and path2 is not absoulte, then path1 is an
> ancestor. */
> > + /* If path1 is empty and path2 is not absolute, then path1 is an
> ancestor. */
> > if (SVN_PATH_IS_EMPTY(path1))
> > return *path2 != '/';
> >
> > /* If path1 is a prefix of path2, then:
> > - - If path1 ends in a path separator,
> > + - If path1 ends in a path separator ('/' or ':' on Windows),
> > - If the paths are of the same length
> > OR
> > - path2 starts a new path component after the common prefix,
> > then path1 is an ancestor. */
> > if (strncmp(path1, path2, path1_len) == 0)
> > - return path1[path1_len - 1] == '/'
> > - || (path2[path1_len] == '/' || path2[path1_len] == '\0');
> > + return path1[path1_len - 1] == '/' ||
> > +#if defined (WIN32)
> > + path1[path1_len - 1] == ':' ||
> > +#endif /* WIN32 */
> > + (path2[path1_len] == '/' || path2[path1_len] == '\0');
> >
> > return FALSE;
> > }
> > Index: subversion/tests/cmdline/svntest/tree.py
> > ===================================================================
> > --- subversion/tests/cmdline/svntest/tree.py (revision 21396)
> > +++ subversion/tests/cmdline/svntest/tree.py (working copy)
> > @@ -276,10 +276,21 @@
> > ### we should raise a less generic error here. which?
> > raise SVNTreeError
> >
> > - root_node = SVNTreeNode(elements[0], None)
> > + root_node = None
> > +
> > + # if this is Windows: if the path contains a drive name (X:), make it
> > + # the root node.
> > + if os.name == 'nt':
> > + m = re.match("([a-zA-Z]:)(.+)", elements[0])
> > + if m:
> > + root_node = SVNTreeNode(m.group(1), None)
> > + elements[0] = m.group(2)
> > + add_elements_as_path(root_node, elements[0:])
> > +
> > + if not root_node:
> > + root_node = SVNTreeNode(elements[0], None)
> > + add_elements_as_path(root_node, elements[1:])
> >
> > - add_elements_as_path(root_node, elements[1:])
> > -
> > # deposit contents in the very last node.
> > node = root_node
> > while 1:
> > Index: subversion/tests/cmdline/update_tests.py
> > ===================================================================
> > --- subversion/tests/cmdline/update_tests.py (revision 21396)
> > +++ subversion/tests/cmdline/update_tests.py (working copy)
> > @@ -2181,7 +2181,7 @@
> > mu_path = os.path.join(wc_dir, 'A', 'mu')
> > svntest.main.file_append (mu_path, '\nAppended text for mu')
> > zeta_path = os.path.join(wc_dir, 'zeta')
> > - svntest.main.file_append(zeta_path, "This is the file 'zeta'.\n")
> > + svntest.main.file_append(zeta_path, "This is the file 'zeta'\n")
> > svntest.main.run_svn(None, 'add', zeta_path)
> >
> > # Commit.
> > @@ -2204,7 +2204,7 @@
> > os.mkdir(dir1_path)
> > svntest.main.run_svn(None, 'add', '-N', dir1_path)
> > file1_path = os.path.join(dir1_path, 'file1')
> > - svntest.main.file_append(file1_path, "This is the file 'file1'.\n")
> > + svntest.main.file_append(file1_path, "This is the file 'file1'\n")
> > svntest.main.run_svn(None, 'add', '-N', file1_path)
> >
> > expected_output = svntest.wc.State(wc_dir, {
> > @@ -2239,7 +2239,34 @@
> > None, None, None, None, None, 0,
> > '-r', '1', wc_dir)
> >
> > - svntest.main.run_svn(None, 'update', wc_dir)
> > + os.chdir(was_cwd)
> > +
> > + # update to the latest version, but use the relative path 'X:'
> > + wc_dir = drive + ":"
> > + expected_output = svntest.wc.State(wc_dir, {
> > + 'A/mu' : Item(status='U '),
> > + 'zeta' : Item(status='A '),
> > + 'dir1' : Item(status='A '),
> > + 'dir1/file1' : Item(status='A '),
> > + })
> > + expected_status = svntest.actions.get_virginal_state(wc_dir, 3)
> > + expected_status.tweak(wc_rev=3)
> > + expected_status.add({
> > + 'dir1' : Item(status=' ', wc_rev=3),
> > + 'dir1/file1' : Item(status=' ', wc_rev=3),
> > + 'zeta' : Item(status=' ', wc_rev=3),
> > + })
> > + expected_disk.add({
> > + 'zeta' : Item("This is the file 'zeta'\n"),
> > + 'dir1/file1': Item("This is the file 'file1'\n"),
> > + })
> > + expected_disk.tweak('A/mu', contents =
> expected_disk.desc['A/mu'].contents
> > + + '\nAppended text for mu')
> > + svntest.actions.run_and_verify_update(wc_dir,
> > + expected_output,
> > + expected_disk,
> > + expected_status)
> > +
> > finally:
> > os.chdir(was_cwd)
> > # cleanup the virtual drive
> > Index: subversion/tests/libsvn_subr/path-test.c
> > ===================================================================
> > --- subversion/tests/libsvn_subr/path-test.c (revision 21396)
> > +++ subversion/tests/libsvn_subr/path-test.c (working copy)
> > @@ -491,6 +491,13 @@
> > { "X:/abc", "/", "/" },
> > { "X:/abc", "X:/", "X:/" },
> > { "X:/abc", "X:/def", "X:/def" },
> > + { "X:",SVN_EMPTY_PATH, "X:" },
> > + { "X:","abc", "X:abc" },
> > + { "X:", "/def", "/def" },
> > + { "X:abc", "/d", "/d" },
> > + { "X:abc", "/", "/" },
> > + { "X:abc", "X:/", "X:/" },
> > + { "X:abc", "X:/def", "X:/def" },
> > #endif /* WIN32 */
> > };
> >
> > @@ -560,6 +567,27 @@
> > TEST_MANY((pool, SVN_EMPTY_PATH, "/", SVN_EMPTY_PATH, NULL), "/");
> > TEST_MANY((pool, SVN_EMPTY_PATH, SVN_EMPTY_PATH, "/", NULL), "/");
> >
> > +#if defined(WIN32)
> > + TEST_MANY((pool, "X:/", "def", "ghi", NULL), "X:/def/ghi");
> > + TEST_MANY((pool, "abc", "X:/", "ghi", NULL), "X:/ghi");
> > + TEST_MANY((pool, "abc", "def", "X:/", NULL), "X:/");
> > + TEST_MANY((pool, "X:/", "X:/", "ghi", NULL), "X:/ghi");
> > + TEST_MANY((pool, "X:/", "X:/", "/", NULL), "/");
> > + TEST_MANY((pool, "X:/", SVN_EMPTY_PATH, "ghi", NULL), "X:/ghi");
> > + TEST_MANY((pool, "X:/", "def", SVN_EMPTY_PATH, NULL), "X:/def");
> > + TEST_MANY((pool, SVN_EMPTY_PATH, "X:/", "ghi", NULL), "X:/ghi");
> > + TEST_MANY((pool, "X:/", SVN_EMPTY_PATH, SVN_EMPTY_PATH, NULL), "X:/");
> > + TEST_MANY((pool, SVN_EMPTY_PATH, "X:/", SVN_EMPTY_PATH, NULL), "X:/");
> > + TEST_MANY((pool, SVN_EMPTY_PATH, SVN_EMPTY_PATH, "X:/", NULL), "X:/");
> > +
> > + TEST_MANY((pool, "X:", "def", "ghi", NULL), "X:def/ghi");
> > + TEST_MANY((pool, "X:", "X:/", "ghi", NULL), "X:/ghi");
> > + TEST_MANY((pool, "X:", "X:/", "/", NULL), "/");
> > + TEST_MANY((pool, "X:", SVN_EMPTY_PATH, "ghi", NULL), "X:ghi");
> > + TEST_MANY((pool, "X:", "def", SVN_EMPTY_PATH, NULL), "X:def");
> > + TEST_MANY((pool, SVN_EMPTY_PATH, "X:", "ghi", NULL), "X:ghi");
> > +#endif /* WIN32 */
> > +
> > /* ### probably need quite a few more tests... */
> >
> > return SVN_NO_ERROR;
> > @@ -592,6 +620,8 @@
> > #if defined(WIN32)
> > { "X:/", "X:/" },
> > { "X:/abc", "abc" },
> > + { "X:", "X:" },
> > + { "X:abc", "abc" },
> > #endif /* WIN32 */
> > };
> >
> > @@ -617,6 +647,58 @@
> >
> >
> > static svn_error_t *
> > +test_dirname(const char **msg,
> > + svn_boolean_t msg_only,
> > + svn_test_opts_t *opts,
> > + apr_pool_t *pool)
> > +{
> > + int i;
> > + char *result;
> > +
> > + static const char * const paths[][2] = {
> > + { "abc", "" },
> > + { "/abc", "/" },
> > + { "/x/abc", "/x" },
> > + { "/xx/abc", "/xx" },
> > + { "a", "" },
> > + { "/a", "/" },
> > + { "/b/a", "/b" },
> > + { "/", "/" },
> > + { SVN_EMPTY_PATH, SVN_EMPTY_PATH },
> > +#if defined(WIN32)
> > + { "X:/", "X:/" },
> > + { "X:/abc", "X:/" },
> > + { "X:abc", "X:" },
> > + { "X:", "X:" },
> > +#else
> > + /* on non-Windows platforms, ':' is allowed in pathnames */
> > + { "X:", "" },
> > + { "X:abc", "" },
> > +#endif /* WIN32 */
> > + };
> > +
> > + *msg = "test svn_path_dirname";
> > + if (msg_only)
> > + return SVN_NO_ERROR;
> > +
> > + for (i = sizeof(paths) / sizeof(paths[0]); i--; )
> > + {
> > + const char *path = paths[i][0];
> > + const char *expect = paths[i][1];
> > +
> > + result = svn_path_dirname(path, pool);
> > + if (strcmp(result, expect))
> > + return svn_error_createf(SVN_ERR_TEST_FAILED, NULL,
> > + "svn_path_dirname(\"%s\") returned "
> > + "\"%s\". expected \"%s\"",
> > + path, result, expect);
> > + }
> > +
> > + return SVN_NO_ERROR;
> > +}
> > +
> > +
> > +static svn_error_t *
> > test_decompose(const char **msg,
> > svn_boolean_t msg_only,
> > svn_test_opts_t *opts,
> > @@ -722,6 +804,7 @@
> > { "//hst/./", "/hst" },
> > { "X:/foo", "X:/foo" },
> > { "X:/", "X:/" },
> > + { "X:", "X:" },
> > { "X:foo", "X:foo" },
> > #endif /* WIN32 or Cygwin */
> > { NULL, NULL }
> > @@ -764,6 +847,9 @@
> > #if defined(WIN32)
> > { "X:/foo/bar", "X:/foo" },
> > { "X:/foo", "X:/" },
> > + { "X:/", "X:/" },
> > + { "X:foo", "X:" },
> > + { "X:", "X:" },
> > #endif /* WIN32 */
> > { NULL, NULL }
> > };
> > @@ -810,6 +896,8 @@
> > #if defined(WIN32)
> > "X:/foo",
> > "X:/",
> > + "X:foo",
> > + "X:",
> > #endif /* WIN32 */
> > "",
> > };
> > @@ -819,10 +907,11 @@
> > FALSE,
> > FALSE,
> > TRUE,
> > +#if defined(WIN32)
> > FALSE,
> > -#if defined(WIN32)
> > TRUE,
> > FALSE,
> > + TRUE
> > #endif /* WIN32 */
> > };
> >
> > @@ -846,6 +935,86 @@
> > return SVN_NO_ERROR;
> > }
> >
> > +static svn_error_t *
> > +test_path_is_ancestor(const char **msg,
> > + svn_boolean_t msg_only,
> > + svn_test_opts_t *opts,
> > + apr_pool_t *pool)
> > +{
> > + apr_size_t i;
> > +
> > + static const char * const paths[][2] = {
> > + { "/foo", "/foo/bar" },
> > + { "/foo/bar", "/foo/bar/" },
> > + { "/", "/foo" },
> > + { SVN_EMPTY_PATH, "foo" },
> > + { SVN_EMPTY_PATH, ".bar", },
> > +
> > + { "/.bar", "/" },
> > + { "foo/bar", "foo" },
> > + { "/foo/bar", "/foo" },
> > + { "foo", "foo/bar" },
> > + { "foo.", "foo./.bar" },
> > +
> > + { "../foo", ".." },
> > + { SVN_EMPTY_PATH, SVN_EMPTY_PATH },
> > + { "/", "/" },
> > +
> > +#if defined(WIN32)
> > + { "X:/", "X:/" },
> > + { "X:/foo", "X:/" },
> > + { "X:/", "X:/foo" },
> > + { "X:", "X:foo" },
> > + { "X:foo", "X:bar" },
> > +#endif /* WIN32 */
> > + };
> > +
> > + /* Expected results of the tests. */
> > + static const svn_boolean_t retvals[] = {
> > + TRUE,
> > + TRUE,
> > + TRUE,
> > + TRUE,
> > + TRUE,
> > +
> > + FALSE,
> > + FALSE,
> > + FALSE,
> > + TRUE,
> > + TRUE,
> > +
> > + FALSE,
> > + TRUE,
> > + TRUE,
> > +#if defined(WIN32)
> > + TRUE,
> > + FALSE,
> > + TRUE,
> > + TRUE,
> > + FALSE
> > +#endif /* WIN32 */
> > + };
> > +
> > + *msg = "test svn_path_is_ancestor";
> > +
> > + if (msg_only)
> > + return SVN_NO_ERROR;
> > +
> > + for (i = 0; i < sizeof(paths) / sizeof(paths[0]); i++)
> > + {
> > + svn_boolean_t retval;
> > +
> > + retval = svn_path_is_ancestor(paths[i][0], paths[i][1]);
> > + if (retvals[i] != retval)
> > + return svn_error_createf
> > + (SVN_ERR_TEST_FAILED, NULL,
> > + "svn_path_is_ancestor (%s, %s) returned %s instead of %s",
> > + paths[i][0], paths[i][1], retval ? "TRUE" : "FALSE",
> > + retvals[i] ? "TRUE" : "FALSE");
> > + }
> > + return SVN_NO_ERROR;
> > +}
> > +
> >
> > /* The test table. */
> >
> > @@ -862,9 +1031,11 @@
> > SVN_TEST_PASS(test_uri_from_iri),
> > SVN_TEST_PASS(test_join),
> > SVN_TEST_PASS(test_basename),
> > + SVN_TEST_PASS(test_dirname),
> > SVN_TEST_PASS(test_decompose),
> > SVN_TEST_PASS(test_canonicalize),
> > SVN_TEST_PASS(test_remove_component),
> > SVN_TEST_PASS(test_is_root),
> > + SVN_TEST_PASS(test_path_is_ancestor),
> > SVN_TEST_NULL
> > };
> >
> >
> >
> > ------------------------------------------------------------------------
>
>
----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] added support for 'X:' paths on Windows (issue 1711)
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Ping...Has anybody had a chance to look at this? If nothing happens in
the next few days, I'll open an issue.
-Hyrum
Lieven Govaerts wrote:
> Hi,
>
> as part of the complete solution for issue #1711 this patch adds support
> for X: paths. Previous patches already added support for working copies
> on 'X:/' (root of a drive).
>
> The patch also contains lots of extra testcases for the path functions,
> I've added tests for all impacted functions in this patch.
>
> Lieven.
>
>
> [[[
> Follow up change for issue #1711: add support for 'X:' paths. On Windows,
> 'X:' is a symbolic link to the current working directory on the X drive.
>
> Added unit tests for all the impacted path functions.
>
> * subversion/libsvn_subr/path.c
> (svn_path_join_many, previous_segment,
> svn_path_basename, svn_path_is_ancestor): small fixes to support X:
> paths on Windows
>
> * subversion/tests/libsvn_subr/path-test.c
> (test_join, test_canonicalize, test_remove_component,
> test_is_root, ): added extra test cases for Windows paths (X: and X:/)
> (test_dirname): new test for svn_path_dirname
> (test_path_is_ancestor): new test for svn_path_is_ancestor
> (test_funcs): run the two new tests
>
> * subversion/tests/cmdline/update_tests.py
> (update_wc_on_windows_drive): expand the test with a testcase for X:
> path support
>
> * subversion/tests/cmdline/svntest/tree.py
> (create_from_path): on Windows, the 'X:' in 'X:foo' is a folder, so
> consider the ':' in the first element of a path as a folder separator
> on that platform.
> ]]]
>
>
> ------------------------------------------------------------------------
>
> Index: subversion/libsvn_subr/path.c
> ===================================================================
> --- subversion/libsvn_subr/path.c (revision 21396)
> +++ subversion/libsvn_subr/path.c (working copy)
> @@ -185,13 +185,13 @@
> if (nargs++ < MAX_SAVED_LENGTHS)
> saved_lengths[nargs] = len;
>
> - if (svn_path_is_absolute(s, strlen(s), pool))
> + if (svn_path_is_absolute(s, len, pool))
> {
> /* an absolute path. skip all components to this point and reset
> the total length. */
> total_len = len;
> base_arg = nargs;
> - base_is_root = len == 1;
> + base_is_root = svn_path_is_root(s, len, pool);
> base_is_empty = FALSE;
> }
> else if (nargs == base_arg
> @@ -254,7 +254,8 @@
> (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 (p != path && p[-1] != '/')
> + if (p != path && p[-1] != '/' &&
> + ! (nargs - 1 == base_arg && base_is_root))
> *p++ = '/';
>
> /* copy the new component and advance the pointer */
> @@ -311,8 +312,13 @@
> if (len == 0)
> return 0;
>
> - while (len > 0 && path[--len] != '/')
> - ;
> + --len;
> + while (len > 0 && path[len] != '/'
> +#if defined(WIN32)
> + && path[len] != ':'
> +#endif /* WIN32 */
> + )
> + --len;
>
> /* check if the remaining segment including trailing '/' is a root path */
> if (svn_path_is_root(path, len + 1, NULL))
> @@ -387,7 +393,11 @@
> else
> {
> start = len;
> - while (start > 0 && path[start - 1] != '/')
> + while (start > 0 && path[start - 1] != '/'
> +#if defined(WIN32)
> + && path[start - 1] != ':'
> +#endif /* WIN32 */
> + )
> --start;
> }
>
> @@ -698,19 +708,22 @@
> {
> apr_size_t path1_len = strlen(path1);
>
> - /* If path1 is empty and path2 is not absoulte, then path1 is an ancestor. */
> + /* If path1 is empty and path2 is not absolute, then path1 is an ancestor. */
> if (SVN_PATH_IS_EMPTY(path1))
> return *path2 != '/';
>
> /* If path1 is a prefix of path2, then:
> - - If path1 ends in a path separator,
> + - If path1 ends in a path separator ('/' or ':' on Windows),
> - If the paths are of the same length
> OR
> - path2 starts a new path component after the common prefix,
> then path1 is an ancestor. */
> if (strncmp(path1, path2, path1_len) == 0)
> - return path1[path1_len - 1] == '/'
> - || (path2[path1_len] == '/' || path2[path1_len] == '\0');
> + return path1[path1_len - 1] == '/' ||
> +#if defined (WIN32)
> + path1[path1_len - 1] == ':' ||
> +#endif /* WIN32 */
> + (path2[path1_len] == '/' || path2[path1_len] == '\0');
>
> return FALSE;
> }
> Index: subversion/tests/cmdline/svntest/tree.py
> ===================================================================
> --- subversion/tests/cmdline/svntest/tree.py (revision 21396)
> +++ subversion/tests/cmdline/svntest/tree.py (working copy)
> @@ -276,10 +276,21 @@
> ### we should raise a less generic error here. which?
> raise SVNTreeError
>
> - root_node = SVNTreeNode(elements[0], None)
> + root_node = None
> +
> + # if this is Windows: if the path contains a drive name (X:), make it
> + # the root node.
> + if os.name == 'nt':
> + m = re.match("([a-zA-Z]:)(.+)", elements[0])
> + if m:
> + root_node = SVNTreeNode(m.group(1), None)
> + elements[0] = m.group(2)
> + add_elements_as_path(root_node, elements[0:])
> +
> + if not root_node:
> + root_node = SVNTreeNode(elements[0], None)
> + add_elements_as_path(root_node, elements[1:])
>
> - add_elements_as_path(root_node, elements[1:])
> -
> # deposit contents in the very last node.
> node = root_node
> while 1:
> Index: subversion/tests/cmdline/update_tests.py
> ===================================================================
> --- subversion/tests/cmdline/update_tests.py (revision 21396)
> +++ subversion/tests/cmdline/update_tests.py (working copy)
> @@ -2181,7 +2181,7 @@
> mu_path = os.path.join(wc_dir, 'A', 'mu')
> svntest.main.file_append (mu_path, '\nAppended text for mu')
> zeta_path = os.path.join(wc_dir, 'zeta')
> - svntest.main.file_append(zeta_path, "This is the file 'zeta'.\n")
> + svntest.main.file_append(zeta_path, "This is the file 'zeta'\n")
> svntest.main.run_svn(None, 'add', zeta_path)
>
> # Commit.
> @@ -2204,7 +2204,7 @@
> os.mkdir(dir1_path)
> svntest.main.run_svn(None, 'add', '-N', dir1_path)
> file1_path = os.path.join(dir1_path, 'file1')
> - svntest.main.file_append(file1_path, "This is the file 'file1'.\n")
> + svntest.main.file_append(file1_path, "This is the file 'file1'\n")
> svntest.main.run_svn(None, 'add', '-N', file1_path)
>
> expected_output = svntest.wc.State(wc_dir, {
> @@ -2239,7 +2239,34 @@
> None, None, None, None, None, 0,
> '-r', '1', wc_dir)
>
> - svntest.main.run_svn(None, 'update', wc_dir)
> + os.chdir(was_cwd)
> +
> + # update to the latest version, but use the relative path 'X:'
> + wc_dir = drive + ":"
> + expected_output = svntest.wc.State(wc_dir, {
> + 'A/mu' : Item(status='U '),
> + 'zeta' : Item(status='A '),
> + 'dir1' : Item(status='A '),
> + 'dir1/file1' : Item(status='A '),
> + })
> + expected_status = svntest.actions.get_virginal_state(wc_dir, 3)
> + expected_status.tweak(wc_rev=3)
> + expected_status.add({
> + 'dir1' : Item(status=' ', wc_rev=3),
> + 'dir1/file1' : Item(status=' ', wc_rev=3),
> + 'zeta' : Item(status=' ', wc_rev=3),
> + })
> + expected_disk.add({
> + 'zeta' : Item("This is the file 'zeta'\n"),
> + 'dir1/file1': Item("This is the file 'file1'\n"),
> + })
> + expected_disk.tweak('A/mu', contents = expected_disk.desc['A/mu'].contents
> + + '\nAppended text for mu')
> + svntest.actions.run_and_verify_update(wc_dir,
> + expected_output,
> + expected_disk,
> + expected_status)
> +
> finally:
> os.chdir(was_cwd)
> # cleanup the virtual drive
> Index: subversion/tests/libsvn_subr/path-test.c
> ===================================================================
> --- subversion/tests/libsvn_subr/path-test.c (revision 21396)
> +++ subversion/tests/libsvn_subr/path-test.c (working copy)
> @@ -491,6 +491,13 @@
> { "X:/abc", "/", "/" },
> { "X:/abc", "X:/", "X:/" },
> { "X:/abc", "X:/def", "X:/def" },
> + { "X:",SVN_EMPTY_PATH, "X:" },
> + { "X:","abc", "X:abc" },
> + { "X:", "/def", "/def" },
> + { "X:abc", "/d", "/d" },
> + { "X:abc", "/", "/" },
> + { "X:abc", "X:/", "X:/" },
> + { "X:abc", "X:/def", "X:/def" },
> #endif /* WIN32 */
> };
>
> @@ -560,6 +567,27 @@
> TEST_MANY((pool, SVN_EMPTY_PATH, "/", SVN_EMPTY_PATH, NULL), "/");
> TEST_MANY((pool, SVN_EMPTY_PATH, SVN_EMPTY_PATH, "/", NULL), "/");
>
> +#if defined(WIN32)
> + TEST_MANY((pool, "X:/", "def", "ghi", NULL), "X:/def/ghi");
> + TEST_MANY((pool, "abc", "X:/", "ghi", NULL), "X:/ghi");
> + TEST_MANY((pool, "abc", "def", "X:/", NULL), "X:/");
> + TEST_MANY((pool, "X:/", "X:/", "ghi", NULL), "X:/ghi");
> + TEST_MANY((pool, "X:/", "X:/", "/", NULL), "/");
> + TEST_MANY((pool, "X:/", SVN_EMPTY_PATH, "ghi", NULL), "X:/ghi");
> + TEST_MANY((pool, "X:/", "def", SVN_EMPTY_PATH, NULL), "X:/def");
> + TEST_MANY((pool, SVN_EMPTY_PATH, "X:/", "ghi", NULL), "X:/ghi");
> + TEST_MANY((pool, "X:/", SVN_EMPTY_PATH, SVN_EMPTY_PATH, NULL), "X:/");
> + TEST_MANY((pool, SVN_EMPTY_PATH, "X:/", SVN_EMPTY_PATH, NULL), "X:/");
> + TEST_MANY((pool, SVN_EMPTY_PATH, SVN_EMPTY_PATH, "X:/", NULL), "X:/");
> +
> + TEST_MANY((pool, "X:", "def", "ghi", NULL), "X:def/ghi");
> + TEST_MANY((pool, "X:", "X:/", "ghi", NULL), "X:/ghi");
> + TEST_MANY((pool, "X:", "X:/", "/", NULL), "/");
> + TEST_MANY((pool, "X:", SVN_EMPTY_PATH, "ghi", NULL), "X:ghi");
> + TEST_MANY((pool, "X:", "def", SVN_EMPTY_PATH, NULL), "X:def");
> + TEST_MANY((pool, SVN_EMPTY_PATH, "X:", "ghi", NULL), "X:ghi");
> +#endif /* WIN32 */
> +
> /* ### probably need quite a few more tests... */
>
> return SVN_NO_ERROR;
> @@ -592,6 +620,8 @@
> #if defined(WIN32)
> { "X:/", "X:/" },
> { "X:/abc", "abc" },
> + { "X:", "X:" },
> + { "X:abc", "abc" },
> #endif /* WIN32 */
> };
>
> @@ -617,6 +647,58 @@
>
>
> static svn_error_t *
> +test_dirname(const char **msg,
> + svn_boolean_t msg_only,
> + svn_test_opts_t *opts,
> + apr_pool_t *pool)
> +{
> + int i;
> + char *result;
> +
> + static const char * const paths[][2] = {
> + { "abc", "" },
> + { "/abc", "/" },
> + { "/x/abc", "/x" },
> + { "/xx/abc", "/xx" },
> + { "a", "" },
> + { "/a", "/" },
> + { "/b/a", "/b" },
> + { "/", "/" },
> + { SVN_EMPTY_PATH, SVN_EMPTY_PATH },
> +#if defined(WIN32)
> + { "X:/", "X:/" },
> + { "X:/abc", "X:/" },
> + { "X:abc", "X:" },
> + { "X:", "X:" },
> +#else
> + /* on non-Windows platforms, ':' is allowed in pathnames */
> + { "X:", "" },
> + { "X:abc", "" },
> +#endif /* WIN32 */
> + };
> +
> + *msg = "test svn_path_dirname";
> + if (msg_only)
> + return SVN_NO_ERROR;
> +
> + for (i = sizeof(paths) / sizeof(paths[0]); i--; )
> + {
> + const char *path = paths[i][0];
> + const char *expect = paths[i][1];
> +
> + result = svn_path_dirname(path, pool);
> + if (strcmp(result, expect))
> + return svn_error_createf(SVN_ERR_TEST_FAILED, NULL,
> + "svn_path_dirname(\"%s\") returned "
> + "\"%s\". expected \"%s\"",
> + path, result, expect);
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
> +
> +static svn_error_t *
> test_decompose(const char **msg,
> svn_boolean_t msg_only,
> svn_test_opts_t *opts,
> @@ -722,6 +804,7 @@
> { "//hst/./", "/hst" },
> { "X:/foo", "X:/foo" },
> { "X:/", "X:/" },
> + { "X:", "X:" },
> { "X:foo", "X:foo" },
> #endif /* WIN32 or Cygwin */
> { NULL, NULL }
> @@ -764,6 +847,9 @@
> #if defined(WIN32)
> { "X:/foo/bar", "X:/foo" },
> { "X:/foo", "X:/" },
> + { "X:/", "X:/" },
> + { "X:foo", "X:" },
> + { "X:", "X:" },
> #endif /* WIN32 */
> { NULL, NULL }
> };
> @@ -810,6 +896,8 @@
> #if defined(WIN32)
> "X:/foo",
> "X:/",
> + "X:foo",
> + "X:",
> #endif /* WIN32 */
> "",
> };
> @@ -819,10 +907,11 @@
> FALSE,
> FALSE,
> TRUE,
> +#if defined(WIN32)
> FALSE,
> -#if defined(WIN32)
> TRUE,
> FALSE,
> + TRUE
> #endif /* WIN32 */
> };
>
> @@ -846,6 +935,86 @@
> return SVN_NO_ERROR;
> }
>
> +static svn_error_t *
> +test_path_is_ancestor(const char **msg,
> + svn_boolean_t msg_only,
> + svn_test_opts_t *opts,
> + apr_pool_t *pool)
> +{
> + apr_size_t i;
> +
> + static const char * const paths[][2] = {
> + { "/foo", "/foo/bar" },
> + { "/foo/bar", "/foo/bar/" },
> + { "/", "/foo" },
> + { SVN_EMPTY_PATH, "foo" },
> + { SVN_EMPTY_PATH, ".bar", },
> +
> + { "/.bar", "/" },
> + { "foo/bar", "foo" },
> + { "/foo/bar", "/foo" },
> + { "foo", "foo/bar" },
> + { "foo.", "foo./.bar" },
> +
> + { "../foo", ".." },
> + { SVN_EMPTY_PATH, SVN_EMPTY_PATH },
> + { "/", "/" },
> +
> +#if defined(WIN32)
> + { "X:/", "X:/" },
> + { "X:/foo", "X:/" },
> + { "X:/", "X:/foo" },
> + { "X:", "X:foo" },
> + { "X:foo", "X:bar" },
> +#endif /* WIN32 */
> + };
> +
> + /* Expected results of the tests. */
> + static const svn_boolean_t retvals[] = {
> + TRUE,
> + TRUE,
> + TRUE,
> + TRUE,
> + TRUE,
> +
> + FALSE,
> + FALSE,
> + FALSE,
> + TRUE,
> + TRUE,
> +
> + FALSE,
> + TRUE,
> + TRUE,
> +#if defined(WIN32)
> + TRUE,
> + FALSE,
> + TRUE,
> + TRUE,
> + FALSE
> +#endif /* WIN32 */
> + };
> +
> + *msg = "test svn_path_is_ancestor";
> +
> + if (msg_only)
> + return SVN_NO_ERROR;
> +
> + for (i = 0; i < sizeof(paths) / sizeof(paths[0]); i++)
> + {
> + svn_boolean_t retval;
> +
> + retval = svn_path_is_ancestor(paths[i][0], paths[i][1]);
> + if (retvals[i] != retval)
> + return svn_error_createf
> + (SVN_ERR_TEST_FAILED, NULL,
> + "svn_path_is_ancestor (%s, %s) returned %s instead of %s",
> + paths[i][0], paths[i][1], retval ? "TRUE" : "FALSE",
> + retvals[i] ? "TRUE" : "FALSE");
> + }
> + return SVN_NO_ERROR;
> +}
> +
>
> /* The test table. */
>
> @@ -862,9 +1031,11 @@
> SVN_TEST_PASS(test_uri_from_iri),
> SVN_TEST_PASS(test_join),
> SVN_TEST_PASS(test_basename),
> + SVN_TEST_PASS(test_dirname),
> SVN_TEST_PASS(test_decompose),
> SVN_TEST_PASS(test_canonicalize),
> SVN_TEST_PASS(test_remove_component),
> SVN_TEST_PASS(test_is_root),
> + SVN_TEST_PASS(test_path_is_ancestor),
> SVN_TEST_NULL
> };
>
>
>
> ------------------------------------------------------------------------