"Rui, Guo" <timmyguo_at_mail.ustc.edu.cn> writes:
> Please, could anybody review my patch?
Yup, here's a review (finally -- sorry for the delay).
The log message and patch below are already different from what you
posted. I've tweaked the log message, and the patch is against latest
trunk (@r32410). However, this is substantially the same as what you
posted.
> [[[
> Check the existence of target directory on copy.
>
> Patch by: Guo Rui <timmyguo_at_mail.ustc.edu.cn>
>
> * subversion/libsvn_subr/io.c
> (svn_io_copy_dir_recursively): Use SVN_ERR_FS_ALREADY_EXISTS instead of
> SVN_ERR_ENTRY_EXISTS.
This part of the change I don't understand. What was wrong with using
SVN_ERR_ENTRY_EXISTS? See more comments about this below.
> * subversion/libsvn_wc/copy.c
> (copy_file_administratively): The same as above. Also check the existence
> of target directory.
>
> * subversion/libsvn_client/copy.c
> (wc_to_wc_copy, repos_to_wc_copy): The same as the first one.
> (svn_client_copy4): Only check SVN_ERR_FS_ALREADY_EXISTS.
>
> See also the discussion thread in which the patch was posted:
>
> http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=140794
> From: "Rui, Guo" <timmyguo_at_mail.ustc.edu.cn>
> To: dev_at_subversion.tigris.org
> Subject: Re: [PATCH] copy file to existing directory.
> Date: Sun, 6 Jul 2008 00:43:50 +0800
> Message-ID: <20080705164350.GE8838_at_Behemoth.ustc.edu.cn>
> ]]]
>
> 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.
> Index: subversion/libsvn_wc/copy.c
> ===================================================================
> --- subversion/libsvn_wc/copy.c (revision 32410)
> +++ subversion/libsvn_wc/copy.c (working copy)
> @@ -399,18 +399,17 @@
> /* 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,
> + return svn_error_createf(SVN_ERR_FS_ALREADY_EXISTS, NULL,
> _("'%s' already exists and is in the way"),
> svn_path_local_style(dst_path, pool));
Same question here: what was wrong with the old error?
> - /* 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 hunk seems to me to be the core of the change -- it fixes the
logical error we had before. It's unrelated to the error code change,
though, isn't it?
> Index: subversion/libsvn_client/copy.c
> ===================================================================
> --- subversion/libsvn_client/copy.c (revision 32410)
> +++ subversion/libsvn_client/copy.c (working copy)
> @@ -517,7 +517,7 @@
> Else, just error out. */
> SVN_ERR(svn_io_check_path(pair->dst, &dst_kind, iterpool));
> if (dst_kind != svn_node_none)
> - return svn_error_createf(SVN_ERR_ENTRY_EXISTS, NULL,
> + return svn_error_createf(SVN_ERR_FS_ALREADY_EXISTS, NULL,
> _("Path '%s' already exists"),
> svn_path_local_style(pair->dst, pool));
Same question: what's wrong with the old error code?
> @@ -1595,7 +1595,7 @@
> SVN_ERR(svn_io_check_path(pair->dst, &dst_kind, iterpool));
> if (dst_kind != svn_node_none)
> {
> - return svn_error_createf(SVN_ERR_ENTRY_EXISTS, NULL,
> + return svn_error_createf(SVN_ERR_FS_ALREADY_EXISTS, NULL,
> _("Path '%s' already exists"),
> svn_path_local_style(pair->dst, pool));
> }
Same here.
> @@ -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?
That's the review. The change looks good, overall; I almost applied it,
but need to understand the reasoning behind the error code change first.
I've left more context quoted below, so people don't have to hunt down
the thread to figure out what's going on.
-Karl
>> On Sun, Jul 06, 2008 at 12:39:43AM +0800, Rui, Guo wrote:
>> > Hi all,
>> > I found some problems in the copy sub-command while trying to make 'svn cp'
>> > works when the destination is a excluded item. I found a suspicious code in
>> > copy_file_administratively(). I then did some experiments and verified my
>> > suspect. The problem is that, the checking of existing destination does not
>> > cover the situation of directory. The following commands can trigger the
>> > problem:
>> >
>> > svn co http://server/greek_tree wc
>> > cd wc/A/B
>> > rm -rf F
>> > svn cp lambda F
>> >
>> > The error output is like this:
>> >
>> > subversion/libsvn_wc/log.c:1602: (apr_err=155020)
>> > svn: In directory '.'
>> > subversion/libsvn_wc/log.c:1602: (apr_err=155020)
>> > svn: Error processing command 'modify-entry' in '.'
>> > subversion/libsvn_wc/log.c:861: (apr_err=155020)
>> > svn: Error modifying entry for 'F'
>> > subversion/libsvn_wc/entries.c:2475: (apr_err=155013)
>> > svn: Entry 'F' is already under version control
>> >
>> > And even an 'svn cleanup' can not bring the wc back to work, which is quite
>> > annoying. After correcting the check, I get this output instead:
>> >
>> > subversion/libsvn_client/copy.c:536: (apr_err=155007)
>> > svn: Path 'F' is not a directory
>> >
>> > The error prompt is kinds of confusing. The reason is that, the code in
>> > libsvn_client will try to copy the source into the target directory if it
>> > exists and the check is not perfect. More effort is needed. Please see the
>> > patch for detail.
>> >
>> > [[[
>> > Check the existence of target directory on copy.
>> >
>> > * subversion/libsvn_subr/io.c
>> > (svn_io_copy_dir_recursively): Use SVN_ERR_FS_ALREADY_EXISTS instead of
>> > SVN_ERR_ENTRY_EXISTS.
>> >
>> > * subversion/libsvn_wc/copy.c
>> > (copy_file_administratively): The same as above. Also check the existence
>> > of target directory.
>> >
>> > * subversion/libsvn_client/copy.c
>> > (wc_to_wc_copy, repos_to_wc_copy): The same as the first one.
>> > (svn_client_copy4): Only check SVN_ERR_FS_ALREADY_EXISTS.
>> >
>> > ]]]
>> >
>> >
>> > If we explicitly rm the destination, the result also changes.
>> >
>> > svn co http://server/greek_tree wc
>> > cd wc/A/B
>> > svn rm F
>> > rm -rf F
>> > svn cp lambda F
>> >
>> > result:
>> > subversion/libsvn_subr/io.c:2628: (apr_err=2)
>> > svn: Can't open file 'F/.svn/tmp/dir-prop-revert':
>> >
>> > This time the result is a little better, since the wc is still usable. Maybe
>> > somebody can makes it work better?
>> >
>> > Still, there is one more strange thing. After the directory F is removed both
>> > from revision control and the disk, the 'svn revert' unsurprisingly fails to
>> > finish its work. An 'svn update' can bring back the F directory. However, the
>> > 'delete' schedule flag of directory F in the parent entries is not cleared.
>> > Will it be harmful?
>> >
>> > Rui
>> >
>> >
>> > ---------------------------------------------------------------------
>> > To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
>> > For additional commands, e-mail: dev-help_at_subversion.tigris.org
>> >
>
>> Index: libsvn_subr/io.c
>> ===================================================================
>> --- libsvn_subr/io.c (revision 32000)
>> +++ libsvn_subr/io.c (working copy)
>> @@ -859,7 +859,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));
>>
>> Index: libsvn_wc/copy.c
>> ===================================================================
>> --- libsvn_wc/copy.c (revision 32000)
>> +++ libsvn_wc/copy.c (working copy)
>> @@ -399,18 +399,17 @@
>> /* 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,
>> + return svn_error_createf(SVN_ERR_FS_ALREADY_EXISTS, NULL,
>> _("'%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));
>> Index: libsvn_client/copy.c
>> ===================================================================
>> --- libsvn_client/copy.c (revision 32000)
>> +++ libsvn_client/copy.c (working copy)
>> @@ -517,7 +517,7 @@
>> Else, just error out. */
>> SVN_ERR(svn_io_check_path(pair->dst, &dst_kind, iterpool));
>> if (dst_kind != svn_node_none)
>> - return svn_error_createf(SVN_ERR_ENTRY_EXISTS, NULL,
>> + return svn_error_createf(SVN_ERR_FS_ALREADY_EXISTS, NULL,
>> _("Path '%s' already exists"),
>> svn_path_local_style(pair->dst, pool));
>>
>> @@ -1600,7 +1600,7 @@
>> SVN_ERR(svn_io_check_path(pair->dst, &dst_kind, iterpool));
>> if (dst_kind != svn_node_none)
>> {
>> - return svn_error_createf(SVN_ERR_ENTRY_EXISTS, NULL,
>> + return svn_error_createf(SVN_ERR_FS_ALREADY_EXISTS, NULL,
>> _("Path '%s' already exists"),
>> svn_path_local_style(pair->dst, pool));
>> }
>> @@ -2002,8 +2002,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;
>>
>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
>> For additional commands, e-mail: dev-help_at_subversion.tigris.org
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
> For additional commands, e-mail: dev-help_at_subversion.tigris.org
---------------------------------------------------------------------
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-08 22:27:00 CEST