You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Alexander Thomas <al...@collab.net> on 2005/04/14 07:00:08 UTC
[PATCH] issue #2224- copy wc path to itself recurses infinitely
[[[
Patch to add test for fail on locking a non-existent file
* subversion/libsvn_client/copy.c
(setup_copy): Added few line in function setup_copy
to fix issue #2224 -
["svn copy mydir mydir" recurses infinitely, creating
nested directories]
* subversion/tests/clients/cmdline/copy_tests.py
(wc_copy_dir_to_same_dir): Added function to test
failure on copy a wc path to itself
(test_list) : Added wc_copy_dir_to_same_dir to list
of tests
]]]
Re: [PATCH] issue #2224- copy wc path to itself recurses infinitely
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Fri, 15 Apr 2005, Alexander Thomas wrote:
> On Fri, 2005-04-15 at 04:15, John Szakmeister wrote:
>
> > I'm not sure the above is entirely correct. First, svn_path_local_style()
> > only meant to be called on strings that represent local filesystem paths.
> > At this point src_path could be a URL or wc path.
>
> I think, path within error messages should be in local_style
>
Yes, that's the policy if the path is a path on the local computer of the
user (i.e. not an URL or path in the repository filesystem). But if you
change as you proposed below, that's OK. (Converting an URL to local
style on Windows looks very confusing.)
> I feel even to depreciate url-to-url fix from the patch, by editing line
> "if (src_is_url == dst_is_url)" ==> "if (!src_is_url && !dst_is_url)"
>
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue #2224- copy wc path to itself recurses infinitely
Posted by Alexander Thomas <al...@collab.net>.
On Fri, 2005-04-15 at 04:15, John Szakmeister wrote:
> I'm not sure the above is entirely correct. First, svn_path_local_style()
> only meant to be called on strings that represent local filesystem paths.
> At this point src_path could be a URL or wc path.
I think, path within error messages should be in local_style
> Secondly, this only covers two cases: wc-to-wc copies, and url-to-url copies.
I feel even to depreciate url-to-url fix from the patch, by editing line
"if (src_is_url == dst_is_url)" ==> "if (!src_is_url && !dst_is_url)"
thus limiting only to wc-to-wc copy into itself.
> It doesn't prevent someone from doing a wc-to-url or a url-to-wc copy into itself.
> I think if we're going to say that you can't copy a directory into
> itself, then we should do this for all cases.
url-to-url or wc-to-url or url-to-wc needs further discussion and I
better leave this here for others to comment.I personally feel to keep
this feature intact.
-Alexander Thomas(AT)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue #2224- copy wc path to itself recurses infinitely
Posted by John Szakmeister <jo...@szakmeister.net>.
On Thursday 14 April 2005 04:06, Alexander wrote:
> Sorry, re-sending due to missing log message and issues with the test
> case.
>
> There are a couple of issues. One is about the test case - how to kill
> a long-running process from Python in both *nix and Windows? The second
> is the error code - SVN_ERR_UNSUPPORTED_FEATURE is not as good as
> SVN_ERR_INVALID_ARGUMENTS, but that does not exist.
>
> The issue is here:
> http://subversion.tigris.org/issues/show_bug.cgi?id=2224
[snip]
> + if (src_is_url == dst_is_url)
> + {
> + if (strcmp (src_path, dst_path) == 0)
> + return svn_error_createf
> + (SVN_ERR_UNSUPPORTED_FEATURE, NULL,
> + _("Cannot copy path '%s' into itself"),
> + svn_path_local_style (src_path, pool));
> + }
I'm not sure the above is entirely correct. First, svn_path_local_style()
only meant to be called on strings that represent local filesystem paths.
At this point src_path could be a URL or wc path. Secondly, this only
covers two cases: wc-to-wc copies, and url-to-url copies. It doesn't
prevent someone from doing a wc-to-url or a url-to-wc copy into itself.
I think if we're going to say that you can't copy a directory into
itself, then we should do this for all cases.
> + for dirname in dnames:
> + dir_path = os.path.join(sbox.wc_dir, dirname)
> +
> + # try to copy dir to itself
> + ## TODO need to kill the process after a couple of seconds
> + ## to prevent infinite wait
I don't think you need to worry about killing the process... it's going to
die from a lack of handles, or it's going to exceed the maximum path.
Someone else might feel differently about that though.
-John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue #2224- copy wc path to itself recurses infinitely
Posted by Alexander Thomas <al...@collab.net>.
On Thu, 2005-04-14 at 16:23, John Szakmeister wrote:
> If we're following the same logic, then svn_io_copy_dir_recursively()
> should probably be made to error out when the src == dst_path.
Hmmm ... good suggestion I will look in to it.
-Alexander Thomas(AT)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] issue #2224- copy wc path to itself recurses infinitely
Posted by Dale Worley <dw...@pingtel.com>.
> From: John Szakmeister [mailto:john@szakmeister.net]
>
> On Thursday 14 April 2005 06:31, Alexander Thomas wrote:
> > On Thu, 2005-04-14 at 13:59, John Szakmeister wrote:
> > > It's not an error for someone to want to copy a directory into
> > > itself.
> >
> > what I suggest is to follow unix style.
>
> Hmm... I've never really tried that before with cp, but it
> does indeed fail.
On some versions, this cp operation produces an error message, on others it recurses until the path names it is generating are too long for the OS to handle.
But it would be useful for Subversion to handle this situation in one of the two or three graceful ways that make sense.
Dale
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue #2224- copy wc path to itself recurses infinitely
Posted by John Szakmeister <jo...@szakmeister.net>.
On Thursday 14 April 2005 06:31, Alexander Thomas wrote:
> On Thu, 2005-04-14 at 13:59, John Szakmeister wrote:
> > It's not an error for someone to want to copy a directory into
> > itself.
>
> what I suggest is to follow unix style.
Hmm... I've never really tried that before with cp, but it does indeed
fail.
> > * svn_io_copy_dir_recursively() fails to watch for and skip
> > recursing into dst_basename while looping through the directory
> > contents, at least at the top level (it's actually dst_path +
> > dst_basename that you need to filter). :-)
> >
> > Do you mind taking a stab at this again? At the very least,
> > svn_io_copy_dir_administratively() should be made to work correctly.
>
> I will send some time on this, but my strong feeling is to keep it
> simple and solve this before it reaches copy_dir_administratively, like
> my patch do (i guess so..). 'svn_io_copy_dir_recursively' recurse
> infinitely only if 'svn cp' tries to copy wc_path to itself and in
> other cases, I hope it's perfect.
If we're following the same logic, then svn_io_copy_dir_recursively()
should probably be made to error out when the src == dst_path.
I think there are probably a couple of other minor nits with your patch.
I'll give it further review tonight, if someone else doesn't pick it up
first.
-John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue #2224- copy wc path to itself recurses infinitely
Posted by Alexander Thomas <al...@collab.net>.
On Thu, 2005-04-14 at 13:59, John Szakmeister wrote:
> It's not an error for someone to want to copy a directory into itself.
what I suggest is to follow unix style.
> * svn_io_copy_dir_recursively() fails to watch for and skip recursing
> into dst_basename while looping through the directory contents, at least
> at the top level (it's actually dst_path + dst_basename that you need to
> filter). :-)
> Do you mind taking a stab at this again? At the very least,
> svn_io_copy_dir_administratively() should be made to work correctly.
>
I will send some time on this, but my strong feeling is to keep it
simple and solve this before it reaches copy_dir_administratively, like
my patch do (i guess so..). 'svn_io_copy_dir_recursively' recurse
infinitely only if 'svn cp' tries to copy wc_path to itself and in other
cases, I hope it's perfect.
Thanks
Alexander Thomas(AT)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue #2224- copy wc path to itself recurses infinitely
Posted by John Szakmeister <jo...@szakmeister.net>.
On Thursday 14 April 2005 04:06, Alexander wrote:
> Sorry, re-sending due to missing log message and issues with the test
> case.
>
> There are a couple of issues. One is about the test case - how to kill
> a long-running process from Python in both *nix and Windows? The second
> is the error code - SVN_ERR_UNSUPPORTED_FEATURE is not as good as
> SVN_ERR_INVALID_ARGUMENTS, but that does not exist.
>
> The issue is here:
> http://subversion.tigris.org/issues/show_bug.cgi?id=2224
Thanks for taking a stab at this, but I think you've missed the spirit of
the problem (actually there's two of them involving the same code, only
one of them is in the issue report though). It's not an error for
someone to want to copy a directory into itself.
The only case that fails to Do the Right Thing involving copying
directories is 'svn cp wc-path wc-path', where wc-path contains the same
name (and actually represents the same path... ie., it's copy itself into
itself). Part problem actually lies copy_dir_administratively() (located
in subversion/libsvn_wc/copy.c). It's making a call to
svn_io_copy_dir_recursively(), which leads to 2 problems:
* svn_io_copy_dir_recursively() fails to watch for and skip recursing
into dst_basename while looping through the directory contents, at least
at the top level (it's actually dst_path + dst_basename that you need to
filter). :-)
* Since copy_dir_administratively() is using
svn_io_copy_dir_administratively(), it's copying both versioned and
unversioned files. I believe that is also an error, but isn't
necessarily related to this issue.
Do you mind taking a stab at this again? At the very least,
svn_io_copy_dir_administratively() should be made to work correctly.
-John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue #2224- copy wc path to itself recurses infinitely
Posted by Alexander <al...@collab.net>.
Sorry, re-sending due to missing log message and issues with the test
case.
There are a couple of issues. One is about the test case - how to kill a
long-running process from Python in both *nix and Windows? The second is
the error code - SVN_ERR_UNSUPPORTED_FEATURE is not as good as
SVN_ERR_INVALID_ARGUMENTS, but that does not exist.
The issue is here:
http://subversion.tigris.org/issues/show_bug.cgi?id=2224
-Alexander Thomas (AT)