Hi,
as a follow up to my previous patch for issue #2556 I looked deeper at
how svn can better support Windows paths like X:/ etc.
To see where the problems are, I locally changed the python test
framework code to automatically map a virtual drive to the test working
copy before running test. This allowed me check the behaviour of all
subversion commands (atleast, the ones covered by the regression tests)
on a working copy at the root of a drive.
Attached patch fixes all the issues I encountered.
I chose to add a new function svn_path_is_absolute. This function is
almost identical as svn_path_is_root function, but I didn't find a
better solution to avoid this code duplication.
regards,
Lieven.
[[[
* subversion/include/svn_path.h
(svn_path_is_absolute): New function declaration.
* subversion/libsvn_subr/path.c
(svn_path_is_child): Allow "" with "H:foo" and "X:/" and "X:/foo" to
be parent/child.
(svn_path_join,
svn_path_join_many): support Windows absolute paths.
(svn_path_is_root): for apr calls, use the cstring string.
(svn_path_is_absolute): New function. Tests for all forms considered
absolute folders on the platform at hand ('/foo', 'X:/foo' etc.)
(svn_path_is_child): support Windows root and absolute paths.
* subversion/libsvn_subr/target.c
(svn_path_condense_targets): support Windows root paths.
* subversion/libsvn_wc/lock.c
(svn_wc_adm_open_anchor): support Windows root paths.
* subversion/libsvn_wc/update_editor.c
(check_wc_root): support Windows root paths.
* subversion/tests/libsvn_subr/path-test.c
(test_path_is_child, test_join,
test_canonicalize): add extra testcases for Windows paths.
* subversion/tests/cmdline/update_tests.py
(update_wc_on_windows_drive): added extra commands to further test
the solution for issue #2556
]]]
Index: subversion/include/svn_path.h
===================================================================
--- subversion/include/svn_path.h (revision 21331)
+++ subversion/include/svn_path.h (working copy)
@@ -184,6 +184,14 @@
svn_boolean_t svn_path_is_root(const char *path, apr_size_t len,
apr_pool_t *pool);
+
+/** Return TRUE if @a path is considered absolute on the platform at
+ * hand, amongst which '/foo' on all platforms or 'X:/foo', '\\\\?\\X:/foo',
+ * '\\\\server\\share\\foo' on Windows.
+ */
+svn_boolean_t svn_path_is_absolute(const char *path, apr_size_t len,
+ apr_pool_t *pool);
+
/** Return a new path (or URL) like @a path, but transformed such that
* some types of path specification redundancies are removed.
*
Index: subversion/libsvn_subr/path.c
===================================================================
--- subversion/libsvn_subr/path.c (revision 21331)
+++ subversion/libsvn_subr/path.c (working copy)
@@ -118,7 +118,7 @@
assert(is_canonical(component, clen));
/* If the component is absolute, then return it. */
- if (*component == '/')
+ if (svn_path_is_absolute(component, clen, pool))
return apr_pmemdup(pool, component, clen + 1);
/* If either is empty return the other */
@@ -185,7 +185,7 @@
if (nargs++ < MAX_SAVED_LENGTHS)
saved_lengths[nargs] = len;
- if (*s == '/')
+ if (svn_path_is_absolute(s, strlen(s), pool))
{
/* an absolute path. skip all components to this point and reset
the total length. */
@@ -447,11 +447,11 @@
goto cleanup;
}
- status = apr_filepath_root(&root_path, &rel_path, 0, strpool);
+ status = apr_filepath_root(&root_path, &rel_path_apr, 0, strpool);
if ((status == APR_SUCCESS ||
status == APR_EINCOMPLETE) &&
- rel_path[0] == '\0')
+ rel_path_apr[0] == '\0')
{
result = TRUE;
goto cleanup;
@@ -464,6 +464,42 @@
}
+svn_boolean_t
+svn_path_is_absolute(const char *path, apr_size_t len, apr_pool_t *pool)
+{
+ const char *root_path = NULL;
+ apr_pool_t *strpool = (pool) ? pool : svn_pool_create(NULL);
+ const char *rel_path = apr_pstrmemdup(strpool, path, len);
+ const char *rel_path_apr;
+ svn_boolean_t result = FALSE;
+
+ /* 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,
+ strpool);
+ if (err)
+ {
+ svn_error_clear(err);
+ goto cleanup;
+ }
+
+ if (apr_filepath_root(&root_path, &rel_path_apr,
+ 0, strpool) != APR_ERELATIVE)
+ {
+ result = TRUE;
+ goto cleanup;
+ }
+
+ cleanup:
+ if (!pool)
+ apr_pool_destroy(strpool);
+ return result;
+}
+
+
int
svn_path_compare_paths(const char *path1,
const char *path2)
@@ -622,12 +658,15 @@
/* assert (is_canonical (path1, strlen (path1))); ### Expensive strlen */
/* assert (is_canonical (path2, strlen (path2))); ### Expensive strlen */
- /* Allow "" and "foo" to be parent/child */
+ /* Allow "" and "foo" or "H:foo" to be parent/child */
if (SVN_PATH_IS_EMPTY(path1)) /* "" is the parent */
{
- if (SVN_PATH_IS_EMPTY(path2) /* "" not a child */
- || path2[0] == '/') /* "/foo" not a child */
+ if (SVN_PATH_IS_EMPTY(path2)) /* "" not a child */
return NULL;
+
+ /* check if this is an absolute path */
+ if (svn_path_is_absolute(path2, strlen(path2), pool))
+ return NULL;
else
return apr_pstrdup(pool, path2); /* everything else is child */
}
@@ -646,13 +685,17 @@
or
/ path1[i] == '\0'
/foo path2[i] != '/'
+
+ Other root paths (like X:/) fall under the former case:
+ X:/ path1[i] == '\0'
+ X:/foo path2[i] != '/'
*/
if (path1[i] == '\0' && path2[i])
{
if (path2[i] == '/')
return apr_pstrdup(pool, path2 + i + 1);
- else if (i == 1 && path1[0] == '/')
- return apr_pstrdup(pool, path2 + 1);
+ else if (svn_path_is_root(path1, i, pool))
+ return apr_pstrdup(pool, path2 + i);
}
/* Otherwise, path2 isn't a child. */
Index: subversion/libsvn_subr/target.c
===================================================================
--- subversion/libsvn_subr/target.c (revision 21331)
+++ subversion/libsvn_subr/target.c (working copy)
@@ -167,9 +167,15 @@
if (basedir_len > 0)
{
/* Only advance our pointer past a path separator if
- REL_ITEM isn't the same as *PCOMMON. */
+ REL_ITEM isn't the same as *PCOMMON.
+
+ If *PCOMMON is a root path, basedir_len will already
+ include the closing '/', so never advance the pointer
+ here.
+ */
rel_item += basedir_len;
- if (rel_item[0])
+ if (rel_item[0] &&
+ ! svn_path_is_root(*pcommon, basedir_len, pool))
rel_item++;
}
Index: subversion/libsvn_wc/lock.c
===================================================================
--- subversion/libsvn_wc/lock.c (revision 21331)
+++ subversion/libsvn_wc/lock.c (working copy)
@@ -1114,7 +1114,8 @@
const char *base_name = svn_path_basename(path, pool);
if (svn_path_is_empty(path)
- || ! strcmp(path, "/") || ! strcmp(base_name, ".."))
+ || svn_path_is_root(path, strlen(path), pool)
+ || ! strcmp(base_name, ".."))
{
SVN_ERR(do_open(anchor_access, NULL, path, write_lock, depth, FALSE,
cancel_func, cancel_baton, pool));
@@ -1567,3 +1568,5 @@
return FALSE;
}
+
+
Index: subversion/libsvn_wc/update_editor.c
===================================================================
--- subversion/libsvn_wc/update_editor.c (revision 21331)
+++ subversion/libsvn_wc/update_editor.c (working copy)
@@ -2822,6 +2822,11 @@
if (svn_path_is_empty(path))
return SVN_NO_ERROR;
+ /* If this is the root folder (of a drive), it should be the WC
+ root too. */
+ if (svn_path_is_root(path, strlen(path), pool))
+ return SVN_NO_ERROR;
+
/* If we cannot get an entry for PATH's parent, PATH is a WC root. */
p_entry = NULL;
svn_path_split(path, &parent, &base_name, pool);
Index: subversion/tests/cmdline/update_tests.py
===================================================================
--- subversion/tests/cmdline/update_tests.py (revision 21331)
+++ subversion/tests/cmdline/update_tests.py (working copy)
@@ -2156,7 +2156,10 @@
if not os.name == 'nt':
raise svntest.Skip
- sbox.build()
+ # just create an empty folder, we'll checkout later.
+ sbox.build(create_wc = False)
+ svntest.main.safe_rmtree(sbox.wc_dir)
+ os.mkdir(sbox.wc_dir)
# create a virtual drive to the working copy folder
drive = find_the_next_available_drive_letter()
@@ -2165,12 +2168,14 @@
os.popen3('subst ' + drive +': ' + sbox.wc_dir, 't')
wc_dir = drive + ':\\\\'
-
was_cwd = os.getcwd()
- os.chdir(wc_dir)
try:
- expected_disk = svntest.main.greek_state.copy()
+ svntest.actions.run_and_verify_svn(None, None, [],
+ 'checkout',
+ '--username', svntest.main.wc_author,
+ '--password', svntest.main.wc_passwd,
+ sbox.repo_url, wc_dir)
# Make some local modifications
mu_path = os.path.join(wc_dir, 'A', 'mu')
@@ -2185,14 +2190,56 @@
'zeta' : Item(verb='Adding'),
})
expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
- expected_status.tweak('', 'A/mu', wc_rev=2)
+ expected_status.tweak('A/mu', wc_rev=2)
expected_status.add({
- 'zeta' : Item(status='A ', wc_rev=0),
+ 'zeta' : Item(status=' ', wc_rev=2),
})
svntest.actions.run_and_verify_commit(wc_dir, expected_output,
expected_status, None,
None, None, None, None,
wc_dir, zeta_path)
+
+ # Non recursive commit
+ dir1_path = os.path.join(wc_dir, 'dir1')
+ 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.run_svn(None, 'add', '-N', file1_path)
+
+ expected_output = svntest.wc.State(wc_dir, {
+ 'dir1' : Item(verb='Adding'),
+ 'dir1/file1' : Item(verb='Adding'),
+ })
+ expected_status.add({
+ 'dir1' : Item(status=' ', wc_rev=3),
+ 'dir1/file1' : Item(status=' ', wc_rev=3),
+ })
+ svntest.actions.run_and_verify_commit(wc_dir, expected_output,
+ expected_status, None,
+ None, None, None, None,
+ '-N',
+ wc_dir,
+ dir1_path, file1_path)
+
+ # revert to previous revision to test update
+ os.chdir(wc_dir)
+
+ expected_disk = svntest.main.greek_state.copy()
+ expected_output = svntest.wc.State(wc_dir, {
+ 'A/mu' : Item(status='U '),
+ 'zeta' : Item(status='D '),
+ 'dir1' : Item(status='D '),
+ })
+ expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
+ svntest.actions.run_and_verify_update(wc_dir,
+ expected_output,
+ expected_disk,
+ expected_status,
+ None, None, None, None, None, 0,
+ '-r', '1', wc_dir)
+
+ svntest.main.run_svn(None, 'update', wc_dir)
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 21331)
+++ subversion/tests/libsvn_subr/path-test.c (working copy)
@@ -34,8 +34,18 @@
apr_pool_t *pool)
{
int i, j;
-#define NUM_TEST_PATHS 9
+/* The path checking code is platform specific, so we shouldn't run
+ the Windows path handling testcases on non-Windows platforms.
+ */
+#if defined(WIN32)
+#define NUM_TEST_PATHS 16
+#define RUN_NUM_TEST_PATHS NUM_TEST_PATHS
+#else
+#define NUM_TEST_PATHS 16
+#define RUN_NUM_TEST_PATHS NUM_TEST_PATHS - 7
+#endif
+
static const char * const paths[NUM_TEST_PATHS] = {
"/foo/bar",
"/foo/baz",
@@ -45,20 +55,35 @@
SVN_EMPTY_PATH,
"foo",
".foo",
- "/"
+ "/",
+ "H:/foo/bar",
+ "H:/foo/baz",
+ "H:/foo/bar/baz",
+ "H:/flu/blar/blaz",
+ "H:/foo/bar/baz/bing/boom",
+ "H:/",
+ "H:/iota"
};
static const char * const remainders[NUM_TEST_PATHS][NUM_TEST_PATHS] = {
- { 0, 0, "baz", 0, "baz/bing/boom", 0, 0, 0, 0 },
- { 0, 0, 0, 0, 0, 0, 0, 0, 0 },
- { 0, 0, 0, 0, "bing/boom", 0, 0, 0, 0 },
- { 0, 0, 0, 0, 0, 0, 0, 0, 0 },
- { 0, 0, 0, 0, 0, 0, 0, 0, 0 },
- { 0, 0, 0, 0, 0, 0, "foo", ".foo", 0 },
- { 0, 0, 0, 0, 0, 0, 0, 0, 0 },
- { 0, 0, 0, 0, 0, 0, 0, 0, 0 },
+ { 0, 0, "baz", 0, "baz/bing/boom", 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 },
+ { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 },
+ { 0, 0, 0, 0, "bing/boom", 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 },
+ { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 },
+ { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 },
+ { 0, 0, 0, 0, 0, 0, "foo", ".foo", 0, 0, 0, 0, 0, 0, 0, 0 },
+ { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 },
+ { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 },
{ "foo/bar", "foo/baz", "foo/bar/baz", "flu/blar/blaz",
- "foo/bar/baz/bing/boom", 0, 0, 0, 0 }
+ "foo/bar/baz/bing/boom", 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 },
+ { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, "baz", 0, "baz/bing/boom", 0, 0 },
+ { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 },
+ { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, "bing/boom", 0, 0 },
+ { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 },
+ { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 },
+ { 0, 0, 0, 0, 0, 0, 0, 0, 0, "foo/bar", "foo/baz", "foo/bar/baz",
+ "flu/blar/blaz", "foo/bar/baz/bing/boom", 0, "iota" },
+ { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 }
};
*msg = "test svn_path_is_child";
@@ -66,9 +91,9 @@
if (msg_only)
return SVN_NO_ERROR;
- for (i = 0; i < NUM_TEST_PATHS; i++)
+ for (i = 0; i < RUN_NUM_TEST_PATHS; i++)
{
- for (j = 0; j < NUM_TEST_PATHS; j++)
+ for (j = 0; j < RUN_NUM_TEST_PATHS; j++)
{
const char *remainder;
@@ -461,6 +486,11 @@
#if defined(WIN32)
{ "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 */
};
@@ -690,6 +720,9 @@
{ "//hst/foo", "//hst/foo" },
{ "//hst", "/hst" },
{ "//hst/./", "/hst" },
+ { "X:/foo", "X:/foo" },
+ { "X:/", "X:/" },
+ { "X:foo", "X:foo" },
#endif /* WIN32 or Cygwin */
{ NULL, NULL }
};
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Sep 1 23:12:28 2006