On Sun, Aug 10, 2008 at 02:58:52PM +0800, Rui, Guo wrote:
> > > Index: subversion/libsvn_subr/io.c
> > > ===================================================================
> > > --- subversion/libsvn_subr/io.c (revision 32410)
> > > +++ subversion/libsvn_subr/io.c (working copy)
> > > @@ -736,7 +736,7 @@
> > >
> > > SVN_ERR(svn_io_check_path(dst_path, &kind, subpool));
> > > if (kind != svn_node_none)
> > > - return svn_error_createf(SVN_ERR_ENTRY_EXISTS, NULL,
> > > + return svn_error_createf(SVN_ERR_FS_ALREADY_EXISTS, NULL,
> > > _("Destination '%s' already exists"),
> > > svn_path_local_style(dst_path, pool));
> >
> > We are checking a local path. Shouldn't we use SVN_ERR_ENTRY_EXISTS?
> >
> > Of course, svn_io_copy_dir_recursively() should document the specific
> > error it will return. Right now it doesn't; but since callers will
> > depending the exact error from now on (whether SVN_ERR_ENTRY_EXISTS or
> > SVN_ERR_FS_ALREADY_EXISTS), we need to document how that error can be
> > produced.
> Well, please see the definition of the two error codes quoted below.
>
> SVN_ERRDEF(SVN_ERR_ENTRY_EXISTS,
> SVN_ERR_ENTRY_CATEGORY_START + 2,
> "Entry already exists")
>
> SVN_ERRDEF(SVN_ERR_FS_ALREADY_EXISTS,
> SVN_ERR_FS_CATEGORY_START + 20,
> "Item already exists in filesystem")
>
> If the description of these error code is accurate, can I assume that a test
> on the FS should raise the latter while a test on the entry cache itself
> should raise the former? This is one reason that I made the modification, and
> the other is that it can result in better response to user. See comment below.
I think you misunderstood what SVN_ERR_FS is supposed to mean.
The SVN_ERR_FS_CATEGORY_START error code is meant to be raised by the
server-side code managing Subversion's filesystems (i.e. libsvn_fs_*),
and not the client and working copy library code. The client can only
receive SVN_ERR_FS_* errors when calling the repository access (RA) layer.
svn_io_check_path() is not part of the RA layer.
It's a bit confusing, because "filesystem" here refers to Subversion's
versioned filesystem in the repository, and not the filesystem of the
operating system the client is running on.
See the other SVN_ERR_FS_* errors, they really don't sound like they
are meant to be used to report errors encountered with the filesystem
of the operating system:
SVN_ERRDEF(SVN_ERR_FS_CLEANUP,
SVN_ERR_FS_CATEGORY_START + 1,
"Error closing filesystem")
SVN_ERRDEF(SVN_ERR_FS_NOT_OPEN,
SVN_ERR_FS_CATEGORY_START + 3,
"Filesystem is not open")
SVN_ERRDEF(SVN_ERR_FS_NO_SUCH_REVISION,
SVN_ERR_FS_CATEGORY_START + 6,
"Invalid filesystem revision number")
Such errors don't make sense for a filesystem provided by the OS
(unless it's versioned, but why use Subversion if you have a filesystem
that does the same thing? :)
> > > @@ -1997,8 +1997,7 @@
> > > /* If the destination exists, try to copy the sources as children of the
> > > destination. */
> > > if (copy_as_child && err && (sources->nelts == 1)
> > > - && (err->apr_err == SVN_ERR_ENTRY_EXISTS
> > > - || err->apr_err == SVN_ERR_FS_ALREADY_EXISTS))
> > > + && (err->apr_err == SVN_ERR_FS_ALREADY_EXISTS))
> > > {
> > > const char *src_path = APR_ARRAY_IDX(sources, 0,
> > > svn_client_copy_source_t *)->path;
> >
> > Okay, I see how changing the error codes simplified the above
> > conditional. But was that the only purpose, or was there some other
> > purpose?
>
> This is not only simplify the condition. Please see the comment in the code.
> This branch will try to copy the source as an sub-entry of the destination if
> the destination exists. However, its only meaningful when the destination
> exists in the FS. An hidden entry will show an obvious exception to this. In
> such a situation, only the entry exists.
You are right, but removing the check for SVN_ERR_ENTRY_EXISTS
is wrong and does not achieve what you want.
Let me explain:
The code snippet quoted above is preceded by a call to setup_copy().
setup_copy() handles all the following cases of a copy operation:
1. working copy -> working copy
2. working copy -> repository
3. repository -> working copy
4. repository -> repository
In cases 1 and 3, it can only return SVN_ERR_ENTRY_EXISTS,
because the target is a working copy. So removing the check
for this error breaks these cases!
In the other cases, it can raise SVN_ERR_FS_ALREADY_EXISTS,
because the target is the repository.
SVN_ERR_ENTRY_EXISTS is raised if
1) the file is present in the local filesystem
2) the file is not present in the local filesystem but is
present in working copy meta data
If I understood you correctly, you want to tell these two cases apart.
But you cannot tell them apart by looking at the error code,
because the error code is ambiguous.
See copy_file_administratively() (in libsvn_wc/copy.c), which
is called by svn_wc_copy2().
I'll quote the relevant bits of copy_file_administratively:
/* Sanity check: if dst file exists already, don't allow overwrite. */
SVN_ERR(svn_io_check_path(dst_path, &dst_kind, pool));
if (dst_kind != svn_node_none)
return svn_error_createf(SVN_ERR_ENTRY_EXISTS, NULL,
_("'%s' already exists and is in the way"),
svn_path_local_style(dst_path, pool));
The above case checks for the file on the local filesystem of the
OS where the client is running, using svn_io_check_path().
It raises SVN_ERR_ENTRY_EXISTS if the file exists in the local
filesystem (kind != svn_node_none). This is what I think you thought
SVN_ERR_FS_ALREADY_EXISTS does mean (but it does not!)
The next bit immediately follows the one above, so it only runs
if the file is not present on disk (kind == svn_node_none):
/* Even if DST_PATH doesn't exist it may still be a versioned file; it
may be scheduled for deletion, or the user may simply have removed the
working copy. Since we are going to write to DST_PATH text-base and
prop-base we need to detect such cases and abort. */
SVN_ERR(svn_wc_entry(&dst_entry, dst_path, dst_parent, FALSE, pool));
if (dst_entry && dst_entry->kind == svn_node_file)
{
if (dst_entry->schedule != svn_wc_schedule_delete)
return svn_error_createf(SVN_ERR_ENTRY_EXISTS, NULL,
_("There is already a versioned item '%s'"),
svn_path_local_style(dst_path, pool));
}
This case checks for the entry appearing in the working copy meta
data, using svn_wc_entry(). It raises SVN_ERR_ENTRY_EXISTS, too,
(albeit with a slightly different message) if the entry meta data
refers to a file (entry->kind == svn_node_file) which is not scheduled
for deletion (entry->schedule != svn_wc_schedule_delete).
If you need to tell the two cases apart, you can use svn_io_check_path()
and compare the dst_kind return value against svn_node_none, like
the first bit I quoted does.
Or you could invent a new error code and create svn_wc_copy3(), which
returns different errors for these cases. Make sure not to break
existing callers of svn_wc_copy2() if you decide to do that.
svn_wc_copy2() should call svn_wc_copy3() (so that the code is not
duplicated), but return SVN_ERR_ENTRY_EXISTS instead if svn_wc_copy3()
returns your new error code.
Hope this helps,
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-08-10 13:56:28 CEST