Re: [PATCH][Issue 2295]"svn mkdir URL" gives poor error message when directory exists

Kamesh Jayachandran <kamesh@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 <kamesh@collab.net>
   Subject: Re: [PATCH][Issue 2295]"svn mkdir URL" gives poor error \
            message when directory exists
   To: Kamesh Jayachandran <kamesh@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 <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
>
>
> ---------------------------------------------------------------------
> 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