Re: [PATCH] Improve error handling in svn_ra_serf__replay_range()

[vijay]

On Wednesday 19 December 2012 07:27 PM, Daniel Shahaf wrote:
+@SkipUnless(svntest.main.is_ra_type_dav)
+def dump_url_not_in_head(sbox)  :

Spurious whitespace.

+  "dump: URL deleted in HEAD should return error"
+  sbox.build(create_wc = False)
+
+  E_url = sbox.repo_url + '/A/B/E'

+  # Delete directory 'E'from repository.
+  svntest.actions.run_and_verify_svn(None, None, [], "rm", E_url, "-m",
+                                     "delete 'E'")


Can't you pass read_only=True to sbox.build() and then use EE rather
than E here?

I note that 'svnadmin create r; svnrdump dump file://$PWD/r/foo' does
not error --- maybe your patch fixes that too?

The attached patch fixes this issue. This patch is different from the 
previous versions.

We have to error out immediately if we do 'svnrdump dump 
<non-existent-url>', right?

I could notice a change in behavior after this patch is applied.

The single revision/first revision dump is driven with the repos root 
URL using update mechanism(svn_ra_do_update()). So the following 
commands were succeeding though the dump target URL is not there in HEAD.

[[[
URL=http://svn.apache.org/repos/asf/subversion/trunk/contrib/server-side/mod_setlocale

$ svnrdump dump $URL -r1232154 > /dev/null
* Dumped revision 1232154.

$ svnrdump dump $URL -r1232155 > /dev/null
* Dumped revision 1232155.
]]]

After this patch applied, the above commands will fail. Is this behavior 
correct?

svnrdump should support peg-revisions to take dump of paths not present 
in HEAD.

Thanks & Regards,
Vijayaguru




+  expected_dump_fail_err_re = "svnrdump: E160013: '.*' path not found"
+
+  # Returns error as in issue #4100.
+  svntest.actions.run_and_verify_svnrdump(None, svntest.verify.AnyOutput,
+                                          expected_dump_fail_err_re, 1, '-q',
+                                          'dump', E_url)
+
+
+#----------------------------------------------------------------------
+
+
  ########################################################################
  # Run the tests



Return an error if the dump target URL is not present in HEAD.

* subversion/svnrdump/svnrdump.c
  (main): Check that the dump target actually exists. If not, error out
    immediately.

* subversion/tests/cmdline/svnrdump_tests.py
  (dump_non_existent_url): New test.
  (test_list): Add reference to new test.

Patch by: Vijayaguru G <vijay{_AT_}collab.net
Index: subversion/svnrdump/svnrdump.c
===================================================================
--- subversion/svnrdump/svnrdump.c      (revision 1424276)
+++ subversion/svnrdump/svnrdump.c      (working copy)
@@ -852,6 +852,7 @@ main(int argc, const char **argv)
   apr_getopt_t *os;
   const char *first_arg;
   apr_array_header_t *received_opts;
+  svn_node_kind_t node_kind;
   int i;
 
   if (svn_cmdline_init ("svnrdump", stderr) != EXIT_SUCCESS)
@@ -1147,6 +1148,17 @@ main(int argc, const char **argv)
                                           opt_baton->url,
                                           opt_baton->ctx, pool));
 
+  SVNRDUMP_ERR(svn_ra_check_path(opt_baton->session, "", 
+                                 SVN_INVALID_REVNUM,
+                                 &node_kind, pool));
+  
+  if (node_kind == svn_node_none)
+    {
+      err = svn_error_createf(SVN_ERR_RA_ILLEGAL_URL, NULL,
+                              _("URL '%s' doesn't exist"), opt_baton->url);
+      return svn_cmdline_handle_exit_error(err, pool, "svnrdump: ");
+    }
+
   /* Have sane opt_baton->start_revision and end_revision defaults if
      unspecified.  */
   SVNRDUMP_ERR(svn_ra_get_latest_revnum(opt_baton->session,
Index: subversion/tests/cmdline/svnrdump_tests.py
===================================================================
--- subversion/tests/cmdline/svnrdump_tests.py  (revision 1424276)
+++ subversion/tests/cmdline/svnrdump_tests.py  (working copy)
@@ -764,8 +764,22 @@ def only_trunk_A_range_dump(sbox):
 
 
 #----------------------------------------------------------------------
+def dump_non_existent_url(sbox):
+  "dump: non-existent URL should return error"
+  sbox.build(read_only = True, create_wc = False) 
+  
+  non_existent_url = sbox.repo_url + '/A/B/EE'
 
+  expected_dump_fail_err_re = "svnrdump: E170000: URL '.*' doesn't exist"
+ 
+  svntest.actions.run_and_verify_svnrdump(None, None,
+                                          expected_dump_fail_err_re, 1, '-q', 
+                                          'dump', non_existent_url)
 
+  
+#----------------------------------------------------------------------
+
+
 ########################################################################
 # Run the tests
 
@@ -820,6 +834,7 @@ test_list = [ None,
               range_dump,
               only_trunk_range_dump,
               only_trunk_A_range_dump,
+              dump_non_existent_url,
              ]
 
 if __name__ == '__main__':