[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

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

From: <kfogel_at_collab.net>
Date: 2006-01-29 23:57:04 CET

Kamesh Jayachandran <kamesh@collab.net> writes:
> I hereby attach the patch for #2295.
>
> [[[
> Fix issue #2295, "svn mkdir URL" gives poor error message when
> directory exists
>
> * subversion/mod_dav_svn/repos.c
> (dav_svn_get_resource): Populating the request->status_line to
> '409 Directory already exists'.
>
> * subversion/tests/cmdline/basic_tests.py
> (duplicate_mkdir_over_dav_url): New testcase to test #2295.
> (test_list): Run the new test.
>
> ]]]

Looks good. I tightened the log message a bit; see updated patch at
the end of this message.

> Index: subversion/tests/cmdline/basic_tests.py
> ===================================================================
> --- subversion/tests/cmdline/basic_tests.py (revision 18117)
> +++ subversion/tests/cmdline/basic_tests.py (working copy)
> @@ -1695,6 +1695,25 @@
> 'ls', '--verbose', rho_url + '@1')
>
>
> +#----------------------------------------------------------------------
> +# Issue #2295.
> +def duplicate_mkdir_over_dav_url(sbox):
> + "Duplicate mkdir of directory/file over DAV URL should give meaningful \
> + error message"

The description string for this test method is too long, and
capitalizes its first letter. Each of these causes a warning when the
test is run. Did you notice the warnings when you tested the test?

  $ pwd
  /home/kfogel/src/subversion/subversion/tests/cmdline
  $ ./basic_tests.py 30
  PASS: basic_tests.py 30: Duplicate mkdir of directory/file over DAV URL should give meaningful error message
  WARNING: Test doc string exceeds 50 characters
  WARNING: Test doc string is capitalized
  $

Also, the issue is only about directory URLs; it says that the errors
for files are already better. So the test description as given above
is not quite right. It's fixed in my updated patch below.

> + sbox.build()
> + url = svntest.main.current_repo_url
> + scheme = url[:string.find(url, ":")]
> + if scheme == 'http' or scheme == 'https':
> + Duplicate_url_path = url + '/Duplicate'

We don't generally capitalize variable names in Python, except for
special cases like "G_path", where the referent really is capitalized.
Don't make people hit their Shift key more often than necessary :-).

> + svntest.actions.run_and_verify_svn("mkdir DAV_URL/DUPLICATE",
> + ["\n", "Committed revision 2.\n"], [],
> + 'mkdir', '-m', 'log_msg', Duplicate_url_path)
> +
> + svntest.actions.run_and_verify_svn("mkdir DAV_URL/DUPLICATE",
> + [], '.*409 Directory already exists.*',
> + 'mkdir', '-m', 'log_msg', Duplicate_url_path)

...I adjusted "Duplicate" to "duplicate" in the updated patch below.

> ########################################################################
> # Run the tests
>
> @@ -1729,7 +1748,8 @@
> repos_root,
> basic_peg_revision,
> info_nonhead,
> - ls_nonhead
> + ls_nonhead,
> + duplicate_mkdir_over_dav_url
> ### todo: more tests needed:
> ### test "svn rm http://some_url"
> ### not sure this file is the right place, though.

You can always just put a comma at the end of the last test, too. We
do this everywhere else, I don't know why it was missing here.
Updated in the patch below.

> Index: subversion/mod_dav_svn/repos.c
> ===================================================================
> --- subversion/mod_dav_svn/repos.c (revision 18117)
> +++ subversion/mod_dav_svn/repos.c (working copy)
> @@ -1544,6 +1544,11 @@
> "trailing slash on the URI.");
> }
>
> + if ((strcmp(r->method, "MKCOL") == 0) && comb->res.exists)
> + {
> + r->status_line = apr_pstrdup(r->pool, "409 Directory already exists");
> + }
> +
> *resource = &comb->res;
> return NULL;

This fix seems to give the right outward-facing behavior: it makes the
better error be output from the client, and it passes 'make check'.

But I don't understand why it's correct. Why don't we error out
immediately, if we know we're in an error case? Why do we set
*resource to something and then return NULL, instead of leaving
*resource alone and returning an error, as we do for the malformed_URI
case directly below?

My apologies if this has been explained already. I'm catching up on
mail and might have missed some previous discussion.

Meanwhile, here's a new patch to work with:

[[[
Fix issue #2295: "svn mkdir URL" gives a poor error message over DAV
when directory exists.

Patch by: Kamesh Jayachandran <kamesh@collab.net>

* subversion/mod_dav_svn/repos.c
  (dav_svn_get_resource): Populate the request->status_line to
  '409 Directory already exists'.

* subversion/tests/cmdline/basic_tests.py
  (duplicate_mkdir_over_dav_url): New test.
  (test_list): Run it.
]]]

Index: subversion/mod_dav_svn/repos.c
===================================================================
--- subversion/mod_dav_svn/repos.c (revision 18285)
+++ subversion/mod_dav_svn/repos.c (working copy)
@@ -1546,6 +1546,11 @@
                            "trailing slash on the URI.");
     }
 
+ if ((strcmp(r->method, "MKCOL") == 0) && comb->res.exists)
+ {
+ r->status_line = apr_pstrdup(r->pool, "409 Directory already exists");
+ }
+
   *resource = &comb->res;
   return NULL;
 
Index: subversion/tests/cmdline/basic_tests.py
===================================================================
--- subversion/tests/cmdline/basic_tests.py (revision 18285)
+++ subversion/tests/cmdline/basic_tests.py (working copy)
@@ -1695,6 +1695,27 @@
                                      'ls', '--verbose', rho_url + '@1')
   
 
+#----------------------------------------------------------------------
+# Issue #2295.
+def duplicate_mkdir_over_dav_url(sbox):
+ "useful error on 'mkdir URL_TO_EXISTING_DIR'"
+
+ sbox.build()
+ url = svntest.main.current_repo_url
+ scheme = url[:string.find(url, ":")]
+ if scheme == 'http' or scheme == 'https':
+ duplicate_url_path = url + '/duplicate'
+
+ svntest.actions.run_and_verify_svn("mkdir DAV_URL/DUPLICATE",
+ ["\n", "Committed revision 2.\n"], [],
+ 'mkdir', '-m', 'log_msg',
+ duplicate_url_path)
+
+ svntest.actions.run_and_verify_svn("mkdir DAV_URL/DUPLICATE",
+ [], '.*409 Directory already exists.*',
+ 'mkdir', '-m', 'log_msg',
+ duplicate_url_path)
+
 ########################################################################
 # Run the tests
 
@@ -1729,7 +1750,8 @@
               repos_root,
               basic_peg_revision,
               info_nonhead,
- ls_nonhead
+ ls_nonhead,
+ duplicate_mkdir_over_dav_url,
               ### todo: more tests needed:
               ### test "svn rm http://some_url"
               ### not sure this file is the right place, though.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Jan 30 01:35:05 2006

This is an archived mail posted to the Subversion Dev mailing list.