Yes have seen the warnings about Capitalized Description string and its
length, was not sure what to do(Rather why not it be greater than 50 or
why not it be capitalized.).
Addressed your comments on why not to return dav_error rather than the
resource itself.
Please find the attached patch.
[[[
Fix issue #2295, "svn mkdir URL" gives a poor error message over DAV
when directory exists
Patch by: Kamesh Jayachandran <kamesh@collab.net>
* subversion/include/svn_error_codes.h
Added the error code SVN_ERR_APMOD_DIRECTORY_EXISTS.
* subversion/mod_dav_svn/repos.c
(dav_svn_get_resource): Populating the request->status_line to
'409 Directory already exists' and return dav_error* .
* subversion/tests/cmdline/basic_tests.py
(duplicate_mkdir_over_dav_url): New test.
(test_list): Run it.
]]]
Thanks
With regards
Kamesh Jayachandran
kfogel@collab.net wrote:
> 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.
>
>
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,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.
Index: subversion/include/svn_error_codes.h
===================================================================
--- subversion/include/svn_error_codes.h (revision 18117)
+++ subversion/include/svn_error_codes.h (working copy)
@@ -847,6 +847,10 @@
SVN_ERR_APMOD_CATEGORY_START + 4,
"Input/output error")
+ SVN_ERRDEF (SVN_ERR_APMOD_DIRECTORY_EXISTS,
+ SVN_ERR_APMOD_CATEGORY_START + 5,
+ "Directory already exists")
+
/* libsvn_client errors */
SVN_ERRDEF (SVN_ERR_CLIENT_VERSIONED_PATH_REQUIRED,
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,14 @@
"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");
+ return dav_new_error(r->pool, HTTP_CONFLICT,
+ SVN_ERR_APMOD_DIRECTORY_EXISTS,
+ "Directory already exists");
+ }
+
*resource = &comb->res;
return NULL;
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Jan 30 10:06:15 2006