On Mon, Aug 11, 2008 at 12:06:36PM +0800, Rui, Guo wrote:
> What a silly mistake! Here comes the new patch.
>
> [[[
> Handles the situation of existing destination directory better for copy/move.
>
> * subversion/libsvn_wc/copy.c
> (copy_file_administratively): Check the existence of target directory.
>
> * subversion/libsvn_client/copy.c
> (svn_client_copy4, svn_client_move5): Only copy as child if the destination
> exists in the local filesystem.
>
> ]]]
>
> Rui
> Index: subversion/libsvn_wc/copy.c
> ===================================================================
> --- subversion/libsvn_wc/copy.c (revision 32410)
> +++ subversion/libsvn_wc/copy.c (working copy)
> @@ -403,14 +403,13 @@
> _("'%s' already exists and is in the way"),
> svn_path_local_style(dst_path, pool));
>
> - /* Even if DST_PATH doesn't exist it may still be a versioned file; it
> + /* Even if DST_PATH doesn't exist it may still be a versioned item; 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 && dst_entry->schedule != svn_wc_schedule_delete)
> {
> - 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));
The above extends copy_file_administratively to also accept
directories for dst_entry.
The comment above copy_file_administratively says:
/* This function effectively creates and schedules a file for
addition, but does extra administrative things to allow it to
function as a 'copy'.
ASSUMPTIONS:
- src_path points to a file under version control
- dst_parent points to a dir under version control, in the same
working copy.
- dst_basename will be the 'new' name of the copied file in dst_parent
*/
So it assumes the equivalent of:
cp src_path /path/to/dst_parent/dst_basename
where src_path and dst_basename are files.
This is no longer true with the change you've made above,
because this is now considered valid, too:
cp src_path /path/to/dst_parent/dst_basename/
where src_path is a file and dst_basename is a directory.
Now, it seems what you really want to achieve is to prevent the
client from trying to copy something into a dst_parent which happens
to exist in working copy meta data, but not on disk, right?
I'm wondering if we really need to change copy_file_administratively
at all for this, see below.
> Index: subversion/libsvn_client/copy.c
> ===================================================================
> --- subversion/libsvn_client/copy.c (revision 32410)
> +++ subversion/libsvn_client/copy.c (working copy)
> @@ -2000,26 +2000,31 @@
> && (err->apr_err == SVN_ERR_ENTRY_EXISTS
> || err->apr_err == SVN_ERR_FS_ALREADY_EXISTS))
> {
> - const char *src_path = APR_ARRAY_IDX(sources, 0,
> - svn_client_copy_source_t *)->path;
> - const char *src_basename;
> + svn_node_kind_t dst_kind;
> + SVN_ERR(svn_io_check_path(dst_path, &dst_kind, subpool));
The check above assumes that the copy destination is
inside the working copy, but that is not true in all cases!
dst_path may be a URL, so the destination might be in the
repository, in which case this call to svn_io_check_path() won't work.
Let's take a closer look at setup_copy() instead.
setup_copy() figures out what kind of copy operation we
are going to do, and calls helper functions like wc_to_wc_copy(),
wc_to_repos_copy(), repos_to_wc_copy(), and repos_to_repos_copy().
There are also equivalent functions for moves.
But those copying or moving something into the working
copy all end up calling the same function: svn_wc_copy2().
So we can probably put the check there.
Here is the bit of svn_wc_copy2() that looks like it would be
a good place to put the check. It's one level above
copy_file_administratively(), and the directory copying case is
also covered here:
if (src_kind == svn_node_file)
{
/* Check if we are copying a file scheduled for addition,
these require special handling. */
if (src_entry->schedule == svn_wc_schedule_add
&& (! src_entry->copied))
{
SVN_ERR(copy_added_file_administratively(src_path, TRUE,
dst_parent, dst_basename,
cancel_func, cancel_baton,
notify_func, notify_baton,
pool));
}
else
{
SVN_ERR(copy_file_administratively(src_path, adm_access,
dst_parent, dst_basename,
notify_func, notify_baton, pool));
}
}
else if (src_kind == svn_node_dir)
{
/* Check if we are copying a directory scheduled for addition,
these require special handling. */
if (src_entry->schedule == svn_wc_schedule_add
&& (! src_entry->copied))
{
SVN_ERR(copy_added_dir_administratively(src_path, TRUE,
dst_parent, adm_access,
dst_basename, cancel_func,
cancel_baton, notify_func,
notify_baton, pool));
}
else
{
SVN_ERR(copy_dir_administratively(src_path, adm_access,
dst_parent, dst_basename,
cancel_func, cancel_baton,
notify_func, notify_baton, pool));
}
}
You could add something like the following above this if-block
(note, this is just an idea, I haven't tested this at all!):
svn_node_kind_t dst_parent_kind;
const char* dst_parent_path;
[...]
/* If dst_parent does not exist on disk, throw an error. */
dst_parent_path = svn_wc_adm_access_path(dst_parent);
SVN_ERR(svn_io_check_path(dst_parent_path, &dst_parent_kind, pool));
if (dst_parent_kind == svn_node_none)
return svn_error_createf(SVN_ERR_WC_DEST_PATH_MISSING, NULL,
_("Cannot copy into %s"), dst_parent_path);
if (src_kind == svn_node_file)
{
/* Check if we are copying a file scheduled for addition,
these require special handling. */
[...]
The error I used in the example above does not exist yet.
You could create it in svn_error_codes.h like this:
SVN_ERRDEF(SVN_ERR_WC_DEST_PATH_MISSING,
SVN_ERR_WC_CATEGORY_START + <new number here>,
"Destination path is missing from disk")
The working copy library would then explicitly refuse to try
to copy something (file or dir) into a directory which is present
in working copy meta data but not on disk. Right now it tries to
do so and fails with the error you were seeing ("Entry 'F' is already
under version control"). The new error helps users better understand
that the problem is that they have used 'rm' instead of 'svn rm'.
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-11 13:18:05 CEST