Can someone checkin this patch?
With regards
Kamesh Jayachandran
Kamesh Jayachandran wrote:
> 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 Wed Feb 8 08:34:54 2006