Issue #2398 is about a server-side assertion that happens when merging
the diff between two mod_dav_svn URLs into a working copy, when (say)
the client system is case-insensitive and there is a case difference
between the wc's URL and the command's URLs.
But that's not what I'm here to discuss.
We think the issue #2398 bug may be gone these days, because of fixes
in the calling code. However, investigating it revealed a nasty
mis-assumption in our filesystem backend(s). As I point out in
http://subversion.tigris.org/issues/show_bug.cgi?id=2398#desc20
it's just wrong to use a pointer comparison to determine if two
svn_fs_t objects represent the same filesystem. Instead, we should be
comparing some underlying property, like their UUIDs.
The patch below does this. It passes 'make check', and I will be
running it through 'make davcheck' soon. I'd like to get people's
thoughts on it. I have no evidence yet that it causes a performance
hit, therefore have not done anything fancy like, say, modifying
svn_fs_get_uuid() to cache the uuid in the svn_fs_t object. (That
would raise lifetime issues, and I haven't yet looked into whether
copying the UUID into svn_fs_t->pool would solve them.)
Any thoughts about this patch's general approach?
-Karl
[[[
Fix issue #2398: user data could trigger assertions in server code.
################################################################
### ###
### This patch is not ready for commit yet. I'd like to ###
### discuss it on the dev@ list before applying. -kfogel ###
### ###
################################################################
* subversion/include/svn_fs.h
(svn_fs_same_p): New prototype.
* subversion/libsvn_fs/fs-loader.c
(svn_fs_same_p): Implement it.
* subversion/libsvn_fs_fs/tree.c
(copy_helper): Use above to check equality of the two filesystems,
instead of depending on pointer equality.
* subversion/libsvn_fs_base/tree.c
(copy_helper): Same.
* subversion/include/svn_error_codes.h
(SVN_ERR_FS_NOT_SAME): New error code.
]]]
Index: subversion/libsvn_fs_base/tree.c
===================================================================
--- subversion/libsvn_fs_base/tree.c (revision 18363)
+++ subversion/libsvn_fs_base/tree.c (working copy)
@@ -2902,8 +2902,14 @@
apr_pool_t *pool)
{
struct copy_args args;
+ svn_boolean_t same_p;
- assert (from_root->fs == to_root->fs);
+ SVN_ERR (svn_fs_same_p (&same_p, from_root->fs, to_root->fs, pool));
+ if (! same_p)
+ return svn_error_createf
+ (SVN_ERR_FS_NOT_SAME, NULL,
+ _("Cannot copy between two different filesystems ('%s' and '%s')."),
+ svn_fs_path (from_root->fs, pool), svn_fs_path (to_root->fs, pool));
if (! to_root->is_txn_root)
return not_txn (to_root);
Index: subversion/include/svn_error_codes.h
===================================================================
--- subversion/include/svn_error_codes.h (revision 18363)
+++ subversion/include/svn_error_codes.h (working copy)
@@ -585,6 +585,11 @@
SVN_ERR_FS_CATEGORY_START + 43,
"Unsupported FS format")
+ /** @since New in 1.4. */
+ SVN_ERRDEF (SVN_ERR_FS_NOT_SAME,
+ SVN_ERR_FS_CATEGORY_START + 44,
+ "Two FS objects do not refer to the same real FS")
+
/* repos errors */
SVN_ERRDEF (SVN_ERR_REPOS_LOCKED,
Index: subversion/include/svn_fs.h
===================================================================
--- subversion/include/svn_fs.h (revision 18363)
+++ subversion/include/svn_fs.h (working copy)
@@ -1549,6 +1549,12 @@
const char *uuid,
apr_pool_t *pool);
+/** Set @a same_p to @c TRUE iff @a fs1 and @a fs2 have the same UUID. */
+svn_error_t *svn_fs_same_p (svn_boolean_t *same_p,
+ svn_fs_t *fs1,
+ svn_fs_t *fs2,
+ apr_pool_t *pool);
+
/* Non-historical properties. */
Index: subversion/libsvn_fs/fs-loader.c
===================================================================
--- subversion/libsvn_fs/fs-loader.c (revision 18363)
+++ subversion/libsvn_fs/fs-loader.c (working copy)
@@ -879,6 +879,21 @@
return fs->vtable->set_uuid (fs, uuid, pool);
}
+svn_error_t *svn_fs_same_p (svn_boolean_t *same_p,
+ svn_fs_t *fs1,
+ svn_fs_t *fs2,
+ apr_pool_t *pool)
+{
+ const char *uuid1;
+ const char *uuid2;
+
+ SVN_ERR (svn_fs_get_uuid (fs1, &uuid1, pool));
+ SVN_ERR (svn_fs_get_uuid (fs2, &uuid2, pool));
+
+ *same_p = ! strcmp (uuid1, uuid2);
+ return SVN_NO_ERROR;
+}
+
svn_error_t *
svn_fs_lock (svn_lock_t **lock, svn_fs_t *fs, const char *path,
const char *token, const char *comment,
Index: subversion/libsvn_fs_fs/tree.c
===================================================================
--- subversion/libsvn_fs_fs/tree.c (revision 18363)
+++ subversion/libsvn_fs_fs/tree.c (working copy)
@@ -1928,8 +1928,14 @@
dag_node_t *from_node;
parent_path_t *to_parent_path;
const char *txn_id = to_root->txn;
+ svn_boolean_t same_p;
- assert (from_root->fs == to_root->fs);
+ SVN_ERR (svn_fs_same_p (&same_p, from_root->fs, to_root->fs, pool));
+ if (! same_p)
+ return svn_error_createf
+ (SVN_ERR_FS_NOT_SAME, NULL,
+ _("Cannot copy between two different filesystems ('%s' and '%s')."),
+ svn_fs_path (from_root->fs, pool), svn_fs_path (to_root->fs, pool));
if (from_root->is_txn_root)
return svn_error_create
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Feb 13 23:57:50 2006