On Mon, Aug 11, 2008 at 01:17:49PM +0200, Stefan Sperling wrote:
> 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.
You have quoted the only caller of this function and the code suggests that
the src_path is the only one that is ensured to be a file. If we can handle
the copy successfully, the dest_basename will be a file with the same content.
But there is nothing we can say what 'dst_basename' is if it actually exists
before the copy, right? I did not change the logic of this function and if the
target is a missing directory, the original code will also capture the problem
when it tries to modify the target entry (However, the wc will be damaged
since the log can not be successfully replay. See my original post for demo.).
I just made the error detection more explicit.
>
> > 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.
Oh, my! I focused too much on my case and forgot the general ones.
>
> 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:
>
The svn_wc_copy2() function is good enough. Part of the motivation of this
patch is that, when handling copy to a missing versioned destination, svn will
complain about path 'dest' is not a directory, which is quite confusing. The
core reason is the svn_client_copy4() will try to copy the src as a child of
dest if the dest seems to exist.
My solution is to prevent the second call to setup_copy() in
svn_client_copy4() and failed to make it work for all cases. I think I should
combine a call to svn_path_is_url() with the svn_io_check_path().
Your solution sounds good. But I suspect that we won't be able to go this far
-- the svn_wc_adm_open() will raise a error for the desitination before we can
go into svn_wc_copy2().
Thanks very much and Best regards,
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-12 08:57:19 CEST