On Fri, Aug 08, 2008 at 04:26:27PM -0400, Karl Fogel wrote:
> "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.
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.
>
> > @@ -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.
PS: The same logic also apply to the move command. You may have to add a
similar modification as above.
Rui
>
> 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
>
---------------------------------------------------------------------
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 08:59:20 CEST