You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Kamesh Jayachandran <ka...@collab.net> on 2006/01/29 15:06:10 UTC
[PATCH][Issue 2295]"svn mkdir URL" gives poor error message when
directory exists
Hi All,
I hereby attach the patch for #2295.
With regards
Kamesh Jayachandran
[[[
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.
]]]
Re: [PATCH][Issue 2295]"svn mkdir URL" gives poor error message when directory exists
Posted by kf...@collab.net.
Kamesh Jayachandran <ka...@collab.net> writes:
> Anyway attaching the patch and log again in this message.
>
> Note: the confusion of r18117 and r18285 was because of patch from my
> and your working copy.
> Mine was always r18117 for sometime, Your reply to my patch against
> your working copy of r18285.
(It's good to update now and then, to make sure your patch is still
applicable. I had to apply one hunk by hand; no big deal, though.)
Anyway, see issue #2295 for where this has gone -- the solution turns
out to be a bit different than we thought. jerenkrantz, dlr, rooneg,
sussman, and I spent a long time in IRC today discussing & closing in
on a patch, but it's not done yet. Justin is hoping to look at it
Monday.
-Karl
--
www.collab.net <> CollabNet | Distributed Development On Demand
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH][Issue 2295]"svn mkdir URL" gives poor error message when
directory exists
Posted by Kamesh Jayachandran <ka...@collab.net>.
Hi Karl,
Sorry for the confusion.
I was just replying to my last post on this issue seeking attention and
hoped the attachment to the original mail will be taken as a patch.
Anyway attaching the patch and log again in this message.
Note: the confusion of r18117 and r18285 was because of patch from my
and your working copy.
Mine was always r18117 for sometime, Your reply to my patch against your
working copy of r18285.
[[[
Fix issue #2295, "svn mkdir URL" gives a poor error message over DAV
when directory exists
Patch by: Kamesh Jayachandran <ka...@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.
]]]
With regards
Kamesh Jayachandran
kfogel@collab.net wrote:
> Kamesh Jayachandran <ka...@collab.net> writes:
>
>> Can someone checkin this patch.
>>
>
> I went to test and apply this patch just now, but there are actually
> three patches buried at different levels of quoting indentation in
> your message. Please somehow clearly mark exactly what patch & log
> message is the one you want applied :-). Otherwise the applier has to
> spend a lot of time figuring out which is the correct one.
>
> For example, at first I thought it would be the patch with the fewest
> levels of quoting (i.e., the one with a single ">" along the left).
> But when I looked more closely, I saw that there was a patch in the
> ">>" level that was made against a higher revision than the one at the
> ">" level! In other words:
>
> >> Index: subversion/tests/cmdline/basic_tests.py
> >> ===================================================================
> >> --- subversion/tests/cmdline/basic_tests.py (revision 18285)
>
> versus
>
> > Index: subversion/tests/cmdline/basic_tests.py
> > ===================================================================
> > --- subversion/tests/cmdline/basic_tests.py (revision 18117)
> > +++ subversion/tests/cmdline/basic_tests.py (working copy)
>
> r18285 is higher than r18117, even though the latter appears at the
> "more recent" level of quoting. That's rather unexpected, and makes
> me not trust myself to figure out which patch should be applied.
> Could you please repost with exactly one patch, the latest one?
>
> In general, I think it's sometimes useful to re-read one's own post
> before sending it. If you read over your original post
>
> http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=113022
> From: Kamesh Jayachandran <ka...@collab.net>
> Subject: Re: [PATCH][Issue 2295]"svn mkdir URL" gives poor error \
> message when directory exists
> To: Kamesh Jayachandran <ka...@collab.net>
> CC: dev@subversion.tigris.org
> Date: Tue, 28 Feb 2006 17:44:28 +0530
> [...]
> [...various patches enclosed...]
> [...]
>
> from the point of view of someone not already familiar with the
> contents, you can see how hard it is to tell what patch is what in
> there.
>
> Thanks,
> -Karl
>
>
>> 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 <ka...@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 <ka...@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 <ka...@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
>>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
>> For additional commands, e-mail: dev-help@subversion.tigris.org
>>
>>
>
>
Re: [PATCH][Issue 2295]"svn mkdir URL" gives poor error message when directory exists
Posted by kf...@collab.net.
Kamesh Jayachandran <ka...@collab.net> writes:
> Can someone checkin this patch.
I went to test and apply this patch just now, but there are actually
three patches buried at different levels of quoting indentation in
your message. Please somehow clearly mark exactly what patch & log
message is the one you want applied :-). Otherwise the applier has to
spend a lot of time figuring out which is the correct one.
For example, at first I thought it would be the patch with the fewest
levels of quoting (i.e., the one with a single ">" along the left).
But when I looked more closely, I saw that there was a patch in the
">>" level that was made against a higher revision than the one at the
">" level! In other words:
>> Index: subversion/tests/cmdline/basic_tests.py
>> ===================================================================
>> --- subversion/tests/cmdline/basic_tests.py (revision 18285)
versus
> Index: subversion/tests/cmdline/basic_tests.py
> ===================================================================
> --- subversion/tests/cmdline/basic_tests.py (revision 18117)
> +++ subversion/tests/cmdline/basic_tests.py (working copy)
r18285 is higher than r18117, even though the latter appears at the
"more recent" level of quoting. That's rather unexpected, and makes
me not trust myself to figure out which patch should be applied.
Could you please repost with exactly one patch, the latest one?
In general, I think it's sometimes useful to re-read one's own post
before sending it. If you read over your original post
http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=113022
From: Kamesh Jayachandran <ka...@collab.net>
Subject: Re: [PATCH][Issue 2295]"svn mkdir URL" gives poor error \
message when directory exists
To: Kamesh Jayachandran <ka...@collab.net>
CC: dev@subversion.tigris.org
Date: Tue, 28 Feb 2006 17:44:28 +0530
[...]
[...various patches enclosed...]
[...]
from the point of view of someone not already familiar with the
contents, you can see how hard it is to tell what patch is what in
there.
Thanks,
-Karl
> 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 <ka...@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 <ka...@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 <ka...@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
>
>
> ---------------------------------------------------------------------
> 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][Issue 2295]"svn mkdir URL" gives poor error message when
directory exists
Posted by Kamesh Jayachandran <ka...@collab.net>.
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 <ka...@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 <ka...@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 <ka...@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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH][Issue 2295]"svn mkdir URL" gives poor error message when
directory exists
Posted by Kamesh Jayachandran <ka...@collab.net>.
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 <ka...@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 <ka...@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 <ka...@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
Re: [PATCH][Issue 2295]"svn mkdir URL" gives poor error message when
directory exists
Posted by Kamesh Jayachandran <ka...@collab.net>.
Hi Julian,
Before calling create collection hook Apache mod_dav code tries to get
it using a get_resource hook.
In case get_resource returns a resource with *exists* member being true,
apache dav does not call create collection hook and error out with
HTTP_METHOD_NOT_ALLOWED.
Reference from httpd-2.0.55/modules/dav/main/mod_dav.c
err = dav_get_resource(r, 0 /* label_allowed */, 0 /* use_checked_in */,
&resource);
if (err != NULL)
return dav_handle_err(r, err, NULL);
if (resource->exists) {
/* oops. something was already there! */
/* Apache will supply a default error for this. */
/* ### we should provide a specific error message! */
return HTTP_METHOD_NOT_ALLOWED;
}
Thanks,
With regards
Kamesh Jayachandran
Julian Foad wrote:
> Kamesh Jayachandran wrote:
>> 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;
>
> No mention of the "r->method" nor "r->status_line" members nor any
> string comparison with a method name appears anywhere else in
> mod_dav_svn. This strikes me as odd, perhaps indicating that this is
> not the right way to detect and set such an error.
>
> Note that this code is within the "get resource" function, not within
> the "create collection" function. Perhaps if it is being called with
> a "MKCOL" method, it is being called wrongly.
>
> Disclaimer: I have no mod_dav_svn knowledge; these are just observations.
>
> - Julian
>
Re: [PATCH][Issue 2295]"svn mkdir URL" gives poor error message when
directory exists
Posted by Julian Foad <ju...@btopenworld.com>.
Kamesh Jayachandran wrote:
> 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;
No mention of the "r->method" nor "r->status_line" members nor any string
comparison with a method name appears anywhere else in mod_dav_svn. This
strikes me as odd, perhaps indicating that this is not the right way to detect
and set such an error.
Note that this code is within the "get resource" function, not within the
"create collection" function. Perhaps if it is being called with a "MKCOL"
method, it is being called wrongly.
Disclaimer: I have no mod_dav_svn knowledge; these are just observations.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH][Issue 2295]"svn mkdir URL" gives poor error message when
directory exists
Posted by Kamesh Jayachandran <ka...@collab.net>.
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 <ka...@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 <ka...@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 <ka...@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.
>
>
Re: [PATCH][Issue 2295]"svn mkdir URL" gives poor error message when directory exists
Posted by kf...@collab.net.
Kamesh Jayachandran <ka...@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 <ka...@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