You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Karl Fogel <kf...@red-bean.com> on 2008/08/08 20:26:27 UTC
Re: [PATCH] copy file to existing directory.
"Rui, Guo" <ti...@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 <ti...@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" <ti...@mail.ustc.edu.cn>
> To: dev@subversion.tigris.org
> Subject: Re: [PATCH] copy file to existing directory.
> Date: Sun, 6 Jul 2008 00:43:50 +0800
> Message-ID: <20...@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@subversion.tigris.org
>> > For additional commands, e-mail: dev-help@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@subversion.tigris.org
>> For additional commands, e-mail: dev-help@subversion.tigris.org
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] copy file to existing directory.
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
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
On Sun, Aug 10, 2008 at 08:54:04PM +0200, Arfrever Frehtes Taifersar Arahesis wrote:
> 2008-08-10 17:49:48 Rui, Guo napisał(a):
> > On Sun, Aug 10, 2008 at 10:48:15AM -0400, Karl Fogel wrote:
> > > >
> > > > 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.
> > >
> > > Oops. All this depends on the misunderstanding of what "FS" means. I'm
> > > sorry; I wish I had realized earlier!
> > >
> > > Traditionally, in Subversion, "FS" refers to the repository filesystem.
> > > You'll see that convention elsewhere in the code, too.
> > >
> > Thanks. And Stefan also clarified this for me. So I just provide a new patch.
>
> You probably forgot to attach a 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.
> >
> > ]]]
>
> --
> Arfrever Frehtes Taifersar Arahesis
Re: [PATCH] copy file to existing directory.
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] copy file to existing directory.
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
On Tue, Aug 12, 2008 at 12:40:38PM +0200, Stefan Sperling wrote:
> On Tue, Aug 12, 2008 at 02:56:38PM +0800, Rui, Guo wrote:
> > 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().
>
> Oh, OK. That might be the case, I haven't checked.
>
> But in that case, we could catch the case inside of setup_copy(), right?
> I think it's better to do it there than outside of setup_copy() anyway.
>
> Because setup_copy() already does things like checking whether the
> source is a URL, it seems more suitable to modify setup_copy()
> instead of its caller.
I tried to do it in setup_copy() and resulted in a version copy2.diff. In this
way, I can easily tell if the destination is wc or URL. However, I can not
easily tell if the wc destination is within a missing/hidden versioned path.
Many of the code added in the patch is used to check the situation.
copy2.diff
[[[
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
(setup_copy): Raise an informative error if the destination is within
a missing/hidden versioned path.
]]]
I also provide a version that do the check in svn_client_copy/move() function.
The pro of this way is that calling svn_path_is_url() is more trivial than
checking for the entry in the adm-area. The con is that the modification has
to be duplicated for both copy and move functions.
copy3.diff
[[[
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): Don't copy/move as child if the
destination is a missing/hidden versioned wc directory. Raise an
informative error for this situation.
]]]
Choose either one you think better. :-)
Thanks
Rui
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] copy file to existing directory.
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Aug 13, 2008 at 12:55:09PM +0200, Stefan Sperling wrote:
> - copy a locally added directory (copy-added-dir-recipe.sh)
> - move a locally added directory (mv-added-dir-recipe.sh)
> - copy locally added file (copy-added-file-recipe.sh)
> - move locally added file (mv-added-file-recipe.sh)
> - copy directory (copy-dir-recipe.sh)
> - move directory (mv-dir-recipe.sh)
> - copy file (copy-file-recipe.sh)
> - move file (mv-file-recipe.sh)
>
> The scripts are attached to this mail.
Meh, I got bounce notifications from your ISP, saying these
attachments were removed.
You can get the scripts here if you have trouble getting
at them via mail: http://stsp.name/rui-recipes.tar.gz
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] copy file to existing directory.
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
On Wed, Aug 13, 2008 at 12:55:09PM +0200, Stefan Sperling wrote:
> The last time I reviewed your change, I didn't realise that
> looking at dst_parent does not tell us anything about the
> copy destination. The example change to svn_wc_copy2() I proposed
> earlier in the thread is wrong, because it only checks dst_parent.
> But we need to look at dst_basename, which, if it already exists,
> might either be a file or directory. Sorry about that.
>
>
> I've tried looking at this problem from the context of the branch
> you're working on. I think I know now why the bug you're trying
> to fix is a big problem for your branch.
>
> Let me explain to make sure I have understood things correctly:
>
> When a directory is hidden, there exists meta data about it
> in the entries file of its parent directory. But the directory
> does not exist on disk.
>
> In trunk, the problem you demonstrated can only happen when someone
> removes something from the working copy without telling Subversion
> about it. We can always tell those people "Well, you know, you should
> not be using 'rm', you should be using 'svn remove'".
>
> But once your branch is merged to trunk, this is no longer true!
> Users can then set directories as hidden, which means the same
> situation can happen via normal svn commands. If they now try
> to (accidentally) copy something into a hidden directory, the
> working copy gets corrupted beyond repair. Unless people manually
> edit the log and remove the copy command, the log cannot be replayed,
> and 'svn cleanup' will keep failing.
>
> Did I explain the problem correctly?
>
> If so, I am convinced now that your change to copy_file_administratively
> is absolutely correct. I've verified that it makes the corruption problem
> go away.
>
> So, +1 on comitting this to trunk:
Yep! You've got the whole story.
>
> > 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));
>
> Now, on to your other changes:
>
> These changes seem to be about making Subversion report a better
> error message, not about fixing the working copy corruption.
Yep.
>
> Looking at the log messages on your branch, it seems you have done
> most of the hidden directory implementation in the working copy library.
>
> However, these changes are both made in the client library, and
> therefore make it aware of "hidden" directories. Do we really want
> the client library to know about "hidden" directories?
Well the word 'hidden' just comes from the parameter show_hidden in
svn_wc_entry() series of functions (There are three of them, if I remember
correctly). Originally, the parameter show_hidden includes two kinds of
entries: deleted and absent. And now, in my branch, the excluded entry is also
considered hidden.
>
> But if the client library is already aware of hidden directories
> on your branch, then it's fine to put this check there, too.
What I can say is that the client should have the concept of 'excluded'
directory, just as the know something about deleted and absent now. I think
the check here does not make use of any internal detail that the client should
not know. Or have I lost the position to make a fair judgment? :-)
>
> > Index: subversion/libsvn_client/copy.c
> > ===================================================================
> > --- subversion/libsvn_client/copy.c (revision 32410)
> > +++ subversion/libsvn_client/copy.c (working copy)
> > @@ -1801,6 +1801,45 @@
> >
>
> [...]
>
> > + /* The container directory not exists in the disk.
> > + Check if it is a missing/hidden directory or just a normal
> > + non-existing directory. */
> > + svn_path_split(dst_parent, &dst_pparent, &dst_pbase, pool);
> > + err = svn_wc_adm_probe_open3(&adm_parent, NULL,
> > + dst_pparent, FALSE, 0,
> > + NULL, NULL, pool);
>
> In the comment above, you're talking about hidden directories.
> These exist on your branch, but not on trunk (yet). I don't
> think we can apply this patch to trunk with this comment included.
>
Again, the word 'hidden' is not specific to excluded directories. Even though
deleted directory should not be a problem here, there is always a possibility
for absent entry (Very unlikely, though).
> But more importantly, since the working copy corruption problem
> is fixed with the change to copy_file_administratively, I think
> we should only apply that fix to trunk. Making Subversion report
> more useful error messages in case directories are missing/hidden
> can be done on your branch.
>
> Do you agree?
I think this is not specific to my branch, even though it will benefit the
most. So, if you think it should go into my branch first, I'll be OK. But
personally I still believe it can go into trunk.
But (there will always be a but :-)), consistent error report is really a
serious requirement. The most typical error report till now will fall back to
the default 'Entry XXX is already under version control.' I know that is far
from perfect, but it will be really painful to check each of this report and
provide a better one for hidden entries. I even begin to doubt that whether I
should use the default 'under version control' message in my patch.
Whatever message I use, your scripts are of great value. I'll check if the
current state of the error report is satisfactory. But I may not be able to
further improve the error report within the following days -- The final
deadline of the gsoc is approaching. I'm supposed to cleanup codes, write
tests and prepare documents for the final evaluation. I have no idea what
material Karl will ask for. If I got no explicit requirement, the plan for
the final days would be:
1. Merge back my patch to my branch and catch up with the trunk
2. Check suspicious show_hidden=TRUE calls marked as TODO items.
3. The mergeinfo issue.
PS: I'm going to go offline next week (visting Stanford), I think I have to
hurry up. What a busy summer 'vacation'!
>
> If you do this on your branch, you should make sure that Subversion
> reports similar errors in cases which are similar to this one.
>
> I like the error message you put in your diff:
> "Versioned path '%s' is either missing or hidden."
>
> A more explicit message could be:
> "The target '%s' is a versioned item and is either missing from
> disk or hidden."
>
> In case you want to do this, I have created a few shell scripts
> you can use to check each case:
>
> - copy a locally added directory (copy-added-dir-recipe.sh)
> - move a locally added directory (mv-added-dir-recipe.sh)
> - copy locally added file (copy-added-file-recipe.sh)
> - move locally added file (mv-added-file-recipe.sh)
> - copy directory (copy-dir-recipe.sh)
> - move directory (mv-dir-recipe.sh)
> - copy file (copy-file-recipe.sh)
> - move file (mv-file-recipe.sh)
>
> The scripts are attached to this mail.
> They should later be replaced by proper unit tests which check for
> the correct error message.
>
> Note that the scripts run the final svn command in gdb.
> If you're not familiar with gdb: You just need to type
> run
> and press enter to run the svn command from within gdb.
> You should learn about gdb if you don't know it yet, it's nice :)
Yes, I do use gdb. It's very powerful, but not very handy. Do you use any
front end such as ddd or insight?
Thanks,
Rui
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] copy file to existing directory.
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
What a shame! The same mistake once again!
Rui
On Tue, Aug 12, 2008 at 06:49:15PM +0200, Stefan Sperling wrote:
> On Wed, Aug 13, 2008 at 12:27:16AM +0800, Rui, Guo wrote:
> > Choose either one you think better. :-)
> > Thanks
> >
> > Rui
>
> Hey Rui,
>
> I think it looks like you forgot to attach the diffs.
>
> Stefan
Re: [PATCH] copy file to existing directory.
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
On Thu, Aug 14, 2008 at 04:39:09PM -0400, Karl Fogel wrote:
> "Rui, Guo" <ti...@mail.ustc.edu.cn> writes:
> > Let's put off the work on error report now. Please just commit the first part
> > of the patch (those fixes the wc corruption bug) at the moment. I would like
> > to get it merge back. Thanks.
>
> Guo Rui (and Stefan), my apologies for only silently observing this
> thread the last few days. A lot of things came up -- an excuse I'm sure
> you're as tired of hearing as I am tired of giving :-(. But these other
> tasks were urgent and unavoidable.
>
> I will try to get this patch reviewed tomorrow. If nothing unexpected
> comes up (fingers crossed), tomorrow is really, truly clear and I will
> have plenty of time for review.
>
Don't be too hard to yourself. Unexpected affairs always show up. (And I hope
you have already back to heath.) I also have to go off-line next week. So you
have plenty of time to catch up. But I'm afraid I could not response in time
(until Aug. 28).
> Guo Rui, I'm aware that GSoC is about to end, and I don't mind saying in
> public that you've satisfied the requirements and more. Thank you for
> all your work! I will catch up on branch review as soon as I can, and
> we all hope you'll stay involved, of course.
Thank you very much for your affirmative! I'll of cause stay involved, since
this is my favorite version control tool. However, I will be very busy in the
following half year -- students always need to hunt for a job in their last
school year. So I'll keep involved but may not be very active for a period.
Best regards,
Rui
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] copy file to existing directory.
Posted by Karl Fogel <kf...@red-bean.com>.
"Rui, Guo" <ti...@mail.ustc.edu.cn> writes:
> Let's put off the work on error report now. Please just commit the first part
> of the patch (those fixes the wc corruption bug) at the moment. I would like
> to get it merge back. Thanks.
Guo Rui (and Stefan), my apologies for only silently observing this
thread the last few days. A lot of things came up -- an excuse I'm sure
you're as tired of hearing as I am tired of giving :-(. But these other
tasks were urgent and unavoidable.
I will try to get this patch reviewed tomorrow. If nothing unexpected
comes up (fingers crossed), tomorrow is really, truly clear and I will
have plenty of time for review.
Guo Rui, I'm aware that GSoC is about to end, and I don't mind saying in
public that you've satisfied the requirements and more. Thank you for
all your work! I will catch up on branch review as soon as I can, and
we all hope you'll stay involved, of course.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] copy file to existing directory.
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
On Wed, Aug 13, 2008 at 11:08:04PM +0200, Stefan Sperling wrote:
>
> > Whatever message I use, your scripts are of great value. I'll check if the
> > current state of the error report is satisfactory. But I may not be able to
> > further improve the error report within the following days -- The final
> > deadline of the gsoc is approaching.
>
> That's OK. It does not need to be done quickly. You should do
> more important things first, of course!
Let's put off the work on error report now. Please just commit the first part
of the patch (those fixes the wc corruption bug) at the moment. I would like
to get it merge back. Thanks.
>
> > PS: I'm going to go offline next week (visting Stanford), I think I have to
> > hurry up. What a busy summer 'vacation'!
>
> Sounds like you're having fun though, even though you're busy :)
Yep.
>
> > > Note that the scripts run the final svn command in gdb.
> > > If you're not familiar with gdb: You just need to type
> > > run
> > > and press enter to run the svn command from within gdb.
> > > You should learn about gdb if you don't know it yet, it's nice :)
> > Yes, I do use gdb. It's very powerful, but not very handy. Do you use any
> > front end such as ddd or insight?
>
> No. I tried ddd a little a long time ago and I liked the cute
> boxes it can draw with variables in them etc. But I tend to use
> the command line interface anyway because it gets in the way
> much less than a GUI and is a bit quicker to use. I don't use any
The only complain about the GDB CUI is that it's really painful when I want to
inspect many memory locations in a C++ program. A lot of typing, really.
> fancy features so it suits me even from a usability point of view.
> Hint: If you need a fancy display of the code when using gdb,
> try 'gdb -tui'.
Great hint! Thanks.
Rui
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] copy file to existing directory.
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Aug 14, 2008 at 02:07:13AM +0800, Rui, Guo wrote:
> On Wed, Aug 13, 2008 at 12:55:09PM +0200, Stefan Sperling wrote:
> > Did I explain the problem correctly?
> >
> Yep! You've got the whole story.
OK, that makes me happy :)
> > Looking at the log messages on your branch, it seems you have done
> > most of the hidden directory implementation in the working copy library.
> >
> > However, these changes are both made in the client library, and
> > therefore make it aware of "hidden" directories. Do we really want
> > the client library to know about "hidden" directories?
> Well the word 'hidden' just comes from the parameter show_hidden in
> svn_wc_entry() series of functions (There are three of them, if I remember
> correctly). Originally, the parameter show_hidden includes two kinds of
> entries: deleted and absent. And now, in my branch, the excluded entry is also
> considered hidden.
Ah, OK. I just used 'hidden' directory to mean the functionality
you are implementing on your branch. I am not used to the terminology
around sparse checkouts.
You need a damn dictionary for every feature in Subversion, really.
I bet you could put 3 Subversion developers in a single room.
One working on tree conflicts, one working on merge-tracking,
and one working on sparse checkouts. None of them would understand
what the other two were talking about.
> > But if the client library is already aware of hidden directories
> > on your branch, then it's fine to put this check there, too.
>
> What I can say is that the client should have the concept of 'excluded'
> directory, just as the know something about deleted and absent now. I think
> the check here does not make use of any internal detail that the client should
> not know. Or have I lost the position to make a fair judgment? :-)
I'd say you are the one to judge where it makes sense to put it.
I simply don't know whether libsvn_client is supposed to know
about this feature or not.
> > But more importantly, since the working copy corruption problem
> > is fixed with the change to copy_file_administratively, I think
> > we should only apply that fix to trunk. Making Subversion report
> > more useful error messages in case directories are missing/hidden
> > can be done on your branch.
> >
> > Do you agree?
> I think this is not specific to my branch, even though it will benefit the
> most. So, if you think it should go into my branch first, I'll be OK. But
> personally I still believe it can go into trunk.
> But (there will always be a but :-)), consistent error report is really a
> serious requirement.
Yes, I think so, too.
It can be done on trunk as well if you want.
The most important thing for me is that whoever fixes this, on
whatever branch, should make sure that all cases are consistent.
That's why I made the scripts.
> The most typical error report till now will fall back to
> the default 'Entry XXX is already under version control.' I know that is far
> from perfect, but it will be really painful to check each of this report and
> provide a better one for hidden entries. I even begin to doubt that whether I
> should use the default 'under version control' message in my patch.
One argument for doing this in your branch is that we'll probably
want the messages to change anyway when your branch is merged into
trunk. I mean, we probably want the messages to tell the user that
the problem might be that the directory is hidden (or excluded,
or whatever you call that). We can't make the messages say this
on trunk now because the feature does not yet exist there.
> Whatever message I use, your scripts are of great value. I'll check if the
> current state of the error report is satisfactory. But I may not be able to
> further improve the error report within the following days -- The final
> deadline of the gsoc is approaching.
That's OK. It does not need to be done quickly. You should do
more important things first, of course!
> PS: I'm going to go offline next week (visting Stanford), I think I have to
> hurry up. What a busy summer 'vacation'!
Sounds like you're having fun though, even though you're busy :)
> > Note that the scripts run the final svn command in gdb.
> > If you're not familiar with gdb: You just need to type
> > run
> > and press enter to run the svn command from within gdb.
> > You should learn about gdb if you don't know it yet, it's nice :)
> Yes, I do use gdb. It's very powerful, but not very handy. Do you use any
> front end such as ddd or insight?
No. I tried ddd a little a long time ago and I liked the cute
boxes it can draw with variables in them etc. But I tend to use
the command line interface anyway because it gets in the way
much less than a GUI and is a bit quicker to use. I don't use any
fancy features so it suits me even from a usability point of view.
Hint: If you need a fancy display of the code when using gdb,
try 'gdb -tui'.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] copy file to existing directory.
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Aug 13, 2008 at 02:36:47AM +0800, Rui, Guo wrote:
> What a shame! The same mistake once again!
:)
> Rui
> On Tue, Aug 12, 2008 at 06:49:15PM +0200, Stefan Sperling wrote:
> > On Wed, Aug 13, 2008 at 12:27:16AM +0800, Rui, Guo wrote:
> > > Choose either one you think better. :-)
> > > Thanks
> > >
> > > Rui
> >
> > Hey Rui,
> >
> > I think it looks like you forgot to attach the diffs.
> >
> > Stefan
The last time I reviewed your change, I didn't realise that
looking at dst_parent does not tell us anything about the
copy destination. The example change to svn_wc_copy2() I proposed
earlier in the thread is wrong, because it only checks dst_parent.
But we need to look at dst_basename, which, if it already exists,
might either be a file or directory. Sorry about that.
I've tried looking at this problem from the context of the branch
you're working on. I think I know now why the bug you're trying
to fix is a big problem for your branch.
Let me explain to make sure I have understood things correctly:
When a directory is hidden, there exists meta data about it
in the entries file of its parent directory. But the directory
does not exist on disk.
In trunk, the problem you demonstrated can only happen when someone
removes something from the working copy without telling Subversion
about it. We can always tell those people "Well, you know, you should
not be using 'rm', you should be using 'svn remove'".
But once your branch is merged to trunk, this is no longer true!
Users can then set directories as hidden, which means the same
situation can happen via normal svn commands. If they now try
to (accidentally) copy something into a hidden directory, the
working copy gets corrupted beyond repair. Unless people manually
edit the log and remove the copy command, the log cannot be replayed,
and 'svn cleanup' will keep failing.
Did I explain the problem correctly?
If so, I am convinced now that your change to copy_file_administratively
is absolutely correct. I've verified that it makes the corruption problem
go away.
So, +1 on comitting this to trunk:
> 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));
Now, on to your other changes:
These changes seem to be about making Subversion report a better
error message, not about fixing the working copy corruption.
Looking at the log messages on your branch, it seems you have done
most of the hidden directory implementation in the working copy library.
However, these changes are both made in the client library, and
therefore make it aware of "hidden" directories. Do we really want
the client library to know about "hidden" directories?
But if the client library is already aware of hidden directories
on your branch, then it's fine to put this check there, too.
> Index: subversion/libsvn_client/copy.c
> ===================================================================
> --- subversion/libsvn_client/copy.c (revision 32410)
> +++ subversion/libsvn_client/copy.c (working copy)
> @@ -1801,6 +1801,45 @@
>
[...]
> + /* The container directory not exists in the disk.
> + Check if it is a missing/hidden directory or just a normal
> + non-existing directory. */
> + svn_path_split(dst_parent, &dst_pparent, &dst_pbase, pool);
> + err = svn_wc_adm_probe_open3(&adm_parent, NULL,
> + dst_pparent, FALSE, 0,
> + NULL, NULL, pool);
In the comment above, you're talking about hidden directories.
These exist on your branch, but not on trunk (yet). I don't
think we can apply this patch to trunk with this comment included.
But more importantly, since the working copy corruption problem
is fixed with the change to copy_file_administratively, I think
we should only apply that fix to trunk. Making Subversion report
more useful error messages in case directories are missing/hidden
can be done on your branch.
Do you agree?
If you do this on your branch, you should make sure that Subversion
reports similar errors in cases which are similar to this one.
I like the error message you put in your diff:
"Versioned path '%s' is either missing or hidden."
A more explicit message could be:
"The target '%s' is a versioned item and is either missing from
disk or hidden."
In case you want to do this, I have created a few shell scripts
you can use to check each case:
- copy a locally added directory (copy-added-dir-recipe.sh)
- move a locally added directory (mv-added-dir-recipe.sh)
- copy locally added file (copy-added-file-recipe.sh)
- move locally added file (mv-added-file-recipe.sh)
- copy directory (copy-dir-recipe.sh)
- move directory (mv-dir-recipe.sh)
- copy file (copy-file-recipe.sh)
- move file (mv-file-recipe.sh)
The scripts are attached to this mail.
They should later be replaced by proper unit tests which check for
the correct error message.
Note that the scripts run the final svn command in gdb.
If you're not familiar with gdb: You just need to type
run
and press enter to run the svn command from within gdb.
You should learn about gdb if you don't know it yet, it's nice :)
Stefan
Re: [PATCH] copy file to existing directory.
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Aug 13, 2008 at 12:27:16AM +0800, Rui, Guo wrote:
> Choose either one you think better. :-)
> Thanks
>
> Rui
Hey Rui,
I think it looks like you forgot to attach the diffs.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] copy file to existing directory.
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Aug 12, 2008 at 02:56:38PM +0800, Rui, Guo wrote:
> 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().
Oh, OK. That might be the case, I haven't checked.
But in that case, we could catch the case inside of setup_copy(), right?
I think it's better to do it there than outside of setup_copy() anyway.
Because setup_copy() already does things like checking whether the
source is a URL, it seems more suitable to modify setup_copy()
instead of its caller.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] copy file to existing directory.
Posted by Stefan Sperling <st...@elego.de>.
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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] copy file to existing directory.
Posted by Arfrever Frehtes Taifersar Arahesis <ar...@gmail.com>.
2008-08-10 17:49:48 Rui, Guo napisał(a):
> On Sun, Aug 10, 2008 at 10:48:15AM -0400, Karl Fogel wrote:
> > >
> > > 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.
> >
> > Oops. All this depends on the misunderstanding of what "FS" means. I'm
> > sorry; I wish I had realized earlier!
> >
> > Traditionally, in Subversion, "FS" refers to the repository filesystem.
> > You'll see that convention elsewhere in the code, too.
> >
> Thanks. And Stefan also clarified this for me. So I just provide a new patch.
You probably forgot to attach a 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.
>
> ]]]
--
Arfrever Frehtes Taifersar Arahesis
Re: [PATCH] copy file to existing directory.
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
On Sun, Aug 10, 2008 at 10:48:15AM -0400, Karl Fogel wrote:
> >
> > 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.
>
> Oops. All this depends on the misunderstanding of what "FS" means. I'm
> sorry; I wish I had realized earlier!
>
> Traditionally, in Subversion, "FS" refers to the repository filesystem.
> You'll see that convention elsewhere in the code, too.
>
Thanks. And Stefan also clarified this for me. So I just provide a 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.
]]]
Also I find that there is one more question in the end of the original post,
about retained 'deleted' flag after revert.
Best regards,
Rui
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] copy file to existing directory.
Posted by Karl Fogel <kf...@red-bean.com>.
"Rui, Guo" <ti...@mail.ustc.edu.cn> writes:
>> 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")
Ah, it's just a misunderstanding. The "FS" in "SVN_ERR_FS_*" refers to
the Subversion repository filesystem -- that is, the versioned tree with
nodes and representations and DAGs and and all that. It's not the
client's local disk filesystem.
Meanwhile, with "SVN_ERR_ENTRY_EXISTS", it's not clear whether "ENTRY"
refers to a Subversion working copy (.svn) entry, or to a directory
entry in the client's local filesystem, or perhaps both. I agree this
is confusing. In my opinion, you could use SVN_ERR_ENTRY_EXISTS, or
create a new error SVN_ERR_WC_PATH_EXISTS.
>> > @@ -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.
Oops. All this depends on the misunderstanding of what "FS" means. I'm
sorry; I wish I had realized earlier!
Traditionally, in Subversion, "FS" refers to the repository filesystem.
You'll see that convention elsewhere in the code, too.
Best,
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] copy file to existing directory.
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
On Fri, Aug 08, 2008 at 04:26:27PM -0400, Karl Fogel wrote:
> "Rui, Guo" <ti...@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 <ti...@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" <ti...@mail.ustc.edu.cn>
> > To: dev@subversion.tigris.org
> > Subject: Re: [PATCH] copy file to existing directory.
> > Date: Sun, 6 Jul 2008 00:43:50 +0800
> > Message-ID: <20...@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@subversion.tigris.org
> >> > For additional commands, e-mail: dev-help@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@subversion.tigris.org
> >> For additional commands, e-mail: dev-help@subversion.tigris.org
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> > For additional commands, e-mail: dev-help@subversion.tigris.org
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] copy file to existing directory.
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] copy file to existing directory.
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
On Sun, Aug 10, 2008 at 05:55:50PM +0200, Stefan Sperling wrote:
> On Sun, Aug 10, 2008 at 11:20:36PM +0800, Rui, Guo wrote:
> > > 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!
>
> Great :)
>
> > 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.
>
> Yes, it's good enough, but note that adding a new error code will
> probably perform slightly better. svn_io_check_path() does a stat()
> system call on the path. Control flow has to switch into the operating
> system kernel and back each time we call svn_io_check_path().
>
> Using a different error code does not have have this overhead.
>
> But we're calling svn_io_check_path() in way too many places already
> anyway, so feel free to pick the solution you find easier to implement.
> In general, it's better to get code working correctly first before
> optimising it. We can always optimise it later.
>
Well, it will not contribute to performance problem this time, since I only
add only one call in svn_client_copy/move().
Rui
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] copy file to existing directory.
Posted by Stefan Sperling <st...@elego.de>.
On Sun, Aug 10, 2008 at 11:20:36PM +0800, Rui, Guo wrote:
> > 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!
Great :)
> 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.
Yes, it's good enough, but note that adding a new error code will
probably perform slightly better. svn_io_check_path() does a stat()
system call on the path. Control flow has to switch into the operating
system kernel and back each time we call svn_io_check_path().
Using a different error code does not have have this overhead.
But we're calling svn_io_check_path() in way too many places already
anyway, so feel free to pick the solution you find easier to implement.
In general, it's better to get code working correctly first before
optimising it. We can always optimise it later.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] copy file to existing directory.
Posted by Stefan Sperling <st...@elego.de>.
On Sun, Aug 10, 2008 at 02:58:52PM +0800, Rui, Guo wrote:
> > > 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.
I think you misunderstood what SVN_ERR_FS is supposed to mean.
The SVN_ERR_FS_CATEGORY_START error code is meant to be raised by the
server-side code managing Subversion's filesystems (i.e. libsvn_fs_*),
and not the client and working copy library code. The client can only
receive SVN_ERR_FS_* errors when calling the repository access (RA) layer.
svn_io_check_path() is not part of the RA layer.
It's a bit confusing, because "filesystem" here refers to Subversion's
versioned filesystem in the repository, and not the filesystem of the
operating system the client is running on.
See the other SVN_ERR_FS_* errors, they really don't sound like they
are meant to be used to report errors encountered with the filesystem
of the operating system:
SVN_ERRDEF(SVN_ERR_FS_CLEANUP,
SVN_ERR_FS_CATEGORY_START + 1,
"Error closing filesystem")
SVN_ERRDEF(SVN_ERR_FS_NOT_OPEN,
SVN_ERR_FS_CATEGORY_START + 3,
"Filesystem is not open")
SVN_ERRDEF(SVN_ERR_FS_NO_SUCH_REVISION,
SVN_ERR_FS_CATEGORY_START + 6,
"Invalid filesystem revision number")
Such errors don't make sense for a filesystem provided by the OS
(unless it's versioned, but why use Subversion if you have a filesystem
that does the same thing? :)
> > > @@ -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,
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org