Kamesh Jayachandran <kamesh@collab.net> writes:
> I hereby attach the patch for #2295.
>
> [[[
> Fix issue #2295, "svn mkdir URL" gives poor error message when
> directory exists
>
> * subversion/mod_dav_svn/repos.c
> (dav_svn_get_resource): Populating the request->status_line to
> '409 Directory already exists'.
>
> * subversion/tests/cmdline/basic_tests.py
> (duplicate_mkdir_over_dav_url): New testcase to test #2295.
> (test_list): Run the new test.
>
> ]]]
Looks good. I tightened the log message a bit; see updated patch at
the end of this message.
> Index: subversion/tests/cmdline/basic_tests.py
> ===================================================================
> --- subversion/tests/cmdline/basic_tests.py (revision 18117)
> +++ subversion/tests/cmdline/basic_tests.py (working copy)
> @@ -1695,6 +1695,25 @@
> 'ls', '--verbose', rho_url + '@1')
>
>
> +#----------------------------------------------------------------------
> +# Issue #2295.
> +def duplicate_mkdir_over_dav_url(sbox):
> + "Duplicate mkdir of directory/file over DAV URL should give meaningful \
> + error message"
The description string for this test method is too long, and
capitalizes its first letter. Each of these causes a warning when the
test is run. Did you notice the warnings when you tested the test?
$ pwd
/home/kfogel/src/subversion/subversion/tests/cmdline
$ ./basic_tests.py 30
PASS: basic_tests.py 30: Duplicate mkdir of directory/file over DAV URL should give meaningful error message
WARNING: Test doc string exceeds 50 characters
WARNING: Test doc string is capitalized
$
Also, the issue is only about directory URLs; it says that the errors
for files are already better. So the test description as given above
is not quite right. It's fixed in my updated patch below.
> + sbox.build()
> + url = svntest.main.current_repo_url
> + scheme = url[:string.find(url, ":")]
> + if scheme == 'http' or scheme == 'https':
> + Duplicate_url_path = url + '/Duplicate'
We don't generally capitalize variable names in Python, except for
special cases like "G_path", where the referent really is capitalized.
Don't make people hit their Shift key more often than necessary :-).
> + svntest.actions.run_and_verify_svn("mkdir DAV_URL/DUPLICATE",
> + ["\n", "Committed revision 2.\n"], [],
> + 'mkdir', '-m', 'log_msg', Duplicate_url_path)
> +
> + svntest.actions.run_and_verify_svn("mkdir DAV_URL/DUPLICATE",
> + [], '.*409 Directory already exists.*',
> + 'mkdir', '-m', 'log_msg', Duplicate_url_path)
...I adjusted "Duplicate" to "duplicate" in the updated patch below.
> ########################################################################
> # Run the tests
>
> @@ -1729,7 +1748,8 @@
> repos_root,
> basic_peg_revision,
> info_nonhead,
> - ls_nonhead
> + ls_nonhead,
> + duplicate_mkdir_over_dav_url
> ### todo: more tests needed:
> ### test "svn rm http://some_url"
> ### not sure this file is the right place, though.
You can always just put a comma at the end of the last test, too. We
do this everywhere else, I don't know why it was missing here.
Updated in the patch below.
> Index: subversion/mod_dav_svn/repos.c
> ===================================================================
> --- subversion/mod_dav_svn/repos.c (revision 18117)
> +++ subversion/mod_dav_svn/repos.c (working copy)
> @@ -1544,6 +1544,11 @@
> "trailing slash on the URI.");
> }
>
> + if ((strcmp(r->method, "MKCOL") == 0) && comb->res.exists)
> + {
> + r->status_line = apr_pstrdup(r->pool, "409 Directory already exists");
> + }
> +
> *resource = &comb->res;
> return NULL;
This fix seems to give the right outward-facing behavior: it makes the
better error be output from the client, and it passes 'make check'.
But I don't understand why it's correct. Why don't we error out
immediately, if we know we're in an error case? Why do we set
*resource to something and then return NULL, instead of leaving
*resource alone and returning an error, as we do for the malformed_URI
case directly below?
My apologies if this has been explained already. I'm catching up on
mail and might have missed some previous discussion.
Meanwhile, here's a new patch to work with:
[[[
Fix issue #2295: "svn mkdir URL" gives a poor error message over DAV
when directory exists.
Patch by: Kamesh Jayachandran <kamesh@collab.net>
* subversion/mod_dav_svn/repos.c
(dav_svn_get_resource): Populate the request->status_line to
'409 Directory already exists'.
* subversion/tests/cmdline/basic_tests.py
(duplicate_mkdir_over_dav_url): New test.
(test_list): Run it.
]]]
Index: subversion/mod_dav_svn/repos.c
===================================================================
--- subversion/mod_dav_svn/repos.c (revision 18285)
+++ subversion/mod_dav_svn/repos.c (working copy)
@@ -1546,6 +1546,11 @@
"trailing slash on the URI.");
}
+ if ((strcmp(r->method, "MKCOL") == 0) && comb->res.exists)
+ {
+ r->status_line = apr_pstrdup(r->pool, "409 Directory already exists");
+ }
+
*resource = &comb->res;
return NULL;
Index: subversion/tests/cmdline/basic_tests.py
===================================================================
--- subversion/tests/cmdline/basic_tests.py (revision 18285)
+++ subversion/tests/cmdline/basic_tests.py (working copy)
@@ -1695,6 +1695,27 @@
'ls', '--verbose', rho_url + '@1')
+#----------------------------------------------------------------------
+# Issue #2295.
+def duplicate_mkdir_over_dav_url(sbox):
+ "useful error on 'mkdir URL_TO_EXISTING_DIR'"
+
+ sbox.build()
+ url = svntest.main.current_repo_url
+ scheme = url[:string.find(url, ":")]
+ if scheme == 'http' or scheme == 'https':
+ duplicate_url_path = url + '/duplicate'
+
+ svntest.actions.run_and_verify_svn("mkdir DAV_URL/DUPLICATE",
+ ["\n", "Committed revision 2.\n"], [],
+ 'mkdir', '-m', 'log_msg',
+ duplicate_url_path)
+
+ svntest.actions.run_and_verify_svn("mkdir DAV_URL/DUPLICATE",
+ [], '.*409 Directory already exists.*',
+ 'mkdir', '-m', 'log_msg',
+ duplicate_url_path)
+
########################################################################
# Run the tests
@@ -1729,7 +1750,8 @@
repos_root,
basic_peg_revision,
info_nonhead,
- ls_nonhead
+ ls_nonhead,
+ duplicate_mkdir_over_dav_url,
### todo: more tests needed:
### test "svn rm http://some_url"
### not sure this file is the right place, though.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Jan 30 01:35:05 2006