kfogel@collab.net wrote:
>
> Any thoughts about this patch's general approach?
> [[[
> Fix issue #2398: user data could trigger assertions in server code.
[...]
> ]]]
>
> Index: subversion/libsvn_fs_base/tree.c
> ===================================================================
>
> - 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));
My first thought was: do we need to change from an assertion to a run-time
error? Does or should the doc string say "FROM_ROOT and TO_ROOT must be in the
same repository"? A hard requirement might be appropriate if every
implementation of this interface would necessarily have that requirement. In
this case, it seems reasonable to return a "soft" error.
I wrote the above paragraph before reading Garrett's reply that it's
"fundamentally misusing the API to pass two different filesystems". If that's
the case, we should document that requirement and keep the assert, and the
requirement is then on callers to ensure that they comply with it. (If the
callers want a function like yours to help them check, that's fine, they can
have it.) It's potentially more efficient this way, as a caller making
multiple calls may only have to check once.
Off-topic:
Call me a stickler for specifications, but I went to look for the doc string of
this copy_helper() function which is effectively root_vtable_t::copy(), and the
cupboard was bare. However, the FS_FS versions of these vtable functions have
doc strings. Would it be good to move these doc strings to the vtable definition?
> Index: subversion/include/svn_error_codes.h
> ===================================================================
>
> + /** @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")
Hmm. That eror code name and default description strikes me as too low level,
or something. Two FS objects not referring to the same FS isn't inherently an
error. What's an error in the function above is that a copy from one FS to
another isn't possible. Thus something like "invalid arguments" or the
existing SVN_ERR_UNSUPPORTED_FEATURE would be a more logical generic error code
to use, with your specific message ("Cannot copy between...") overriding the
default. But it's a bit subjective; never mind if you disagree.
The general approach of the patch seems fine, though I'm a novice in this area
and I have no idea about its efficiency.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Feb 14 01:07:31 2006