On Sun, Aug 10, 2008 at 01:56:21PM +0200, Stefan Sperling wrote:
> > > > @@ -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,
It really helps very much! I did not aware of this misunderstanding until you
pointed it out. I'll use svn_io_check_path() solution since it is good enough.
Thanks again.
Rui
---------------------------------------------------------------------
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 17:20:53 CEST