You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by vijay <vi...@collab.net> on 2012/12/17 16:13:33 UTC
Improve error handling in svn_ra_serf__replay_range()
Hi,
I was looking at the issue #4100: "svnrdump dump -rN:M
<URL-deleted-in-HEAD>' fails" [1].
<issue-snip>
[[[
$
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.
$ svnrdump dump $URL -r1232154:1232155 > /dev/null
* Dumped revision 1232154.
subversion/svnrdump/svnrdump.c:433: (apr_err=160013)
subversion/libsvn_ra/ra_loader.c:1184: (apr_err=160013)
subversion/libsvn_ra_neon/util.c:1323: (apr_err=160013)
subversion/libsvn_ra_neon/util.c:596: (apr_err=160013)
svnrdump: E160013:
'http://svn.apache.org/repos/asf/subversion/trunk/contrib/server-side/mod_setlocale'
path not found
]]]
</issue-snip>
I could see the error "path not found" with ra_neon but ra_serf doesn't
return any error whereas it should.
This patch will fix this issue by handling the errors while processing
connections defined in serf context.
Attached the patch and log message.
[1] http://subversion.tigris.org/issues/show_bug.cgi?id=4100
Thanks & Regards,
Vijayaguru
Re: [PATCH] Improve error handling in svn_ra_serf__replay_range()
Posted by Philip Martin <ph...@wandisco.com>.
vijay <vi...@collab.net> writes:
> Index: subversion/tests/cmdline/svnrdump_tests.py
> ===================================================================
> --- subversion/tests/cmdline/svnrdump_tests.py (revision 1423268)
> +++ subversion/tests/cmdline/svnrdump_tests.py (working copy)
> @@ -765,8 +765,28 @@
>
>
> #----------------------------------------------------------------------
> +@SkipUnless(svntest.main.is_ra_type_dav)
> +def dump_url_not_in_head(sbox) :
> + "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'")
>
> + 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
You haven't added the test to the test_list so it is not being run.
Why is the test DAV only? This test should be run over all RA layers.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
RE: [PATCH] Improve error handling in svn_ra_serf__replay_range()
Posted by Gavin Baumanis <ga...@thespidernet.com>.
Ping.
This submission has received no new comments.
Gavin.
> -----Original Message-----
> From: Daniel Shahaf [mailto:danielsh@elego.de]
> Sent: Tuesday, 8 January 2013 15:49
> To: vijay
> Cc: Philip Martin; Subversion Development
> Subject: Re: [PATCH] Improve error handling in svn_ra_serf__replay_range()
>
> vijay wrote on Tue, Jan 08, 2013 at 15:30:46 +0530:
> > On Thursday 20 December 2012 07:52 PM, vijay wrote:
> >>>
> >>> 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?
> >
> >
> > Is it wrong to fix the issue in this way?
>
> I haven't had time to look into it.
Re: [PATCH] Improve error handling in svn_ra_serf__replay_range()
Posted by Daniel Shahaf <da...@elego.de>.
vijay wrote on Tue, Jan 08, 2013 at 15:30:46 +0530:
> On Thursday 20 December 2012 07:52 PM, vijay wrote:
>>>
>>> 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?
>
>
> Is it wrong to fix the issue in this way?
I haven't had time to look into it.
Re: [PATCH] Improve error handling in svn_ra_serf__replay_range()
Posted by vijay <vi...@collab.net>.
On Thursday 20 December 2012 07:52 PM, vijay wrote:
>>
>> 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?
Is it wrong to fix the issue in this way?
Thanks & Regards,
Vijayaguru
>
> 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
>>>
>>
>
Re: [PATCH] Improve error handling in svn_ra_serf__replay_range()
Posted by vijay <vi...@collab.net>.
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
>>
>
Re: [PATCH] Improve error handling in svn_ra_serf__replay_range()
Posted by Daniel Shahaf <da...@elego.de>.
> +@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?
>
> + 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
>
Re: [PATCH] Improve error handling in svn_ra_serf__replay_range()
Posted by vijay <vi...@collab.net>.
On Monday 17 December 2012 09:34 PM, Philip Martin wrote:
> vijay <vi...@collab.net> writes:
>
>> The function svn_ra_serf__replay_range() doesn't return any error
>> while running 'svnrdump dump -rN:M <URL-deleted-in-HEAD>'[Issue #4100].
>> It behaves as if the dump completed successfully.
>>
>> * subversion/libsvn_ra_serf/replay.c
>> (svn_ra_serf__replay_range): Handle the errors from server's response as
>> well as any errors found while running the serf loop.
>
> Consider adding a regression test to svnrdump_tests.py.
Done. The patch and log message are updated.
>
>> /* Run the serf loop. */
>> - SVN_ERR(svn_ra_serf__context_run_wait(&replay_ctx->done, session, pool));
>> + err = svn_ra_serf__context_run_wait(&replay_ctx->done, session, pool);
>>
>> + /* Handle the errors from server's response as well as any errors
>> + found while running the serf loop. */
>> + SVN_ERR(svn_error_compose_create(
>> + svn_ra_serf__error_on_status(handler->sline.code,
>> + handler->path,
>> + handler->location),
>> + err));
>> +
>
> When I see this sort of change I wonder if it needs to be applied
> anywhere else. The call in update.c:svn_ra_serf__get_file also uses
> SVN_ERR. Does it need the same fix?
>
For the above function svn_ra_serf__get_file(), 404 kind of errors will
be handled in svn_ra_serf__fetch_node_props ->
svn_ra_serf__retrieve_props -> svn_ra_serf__wait_for_props.
There are few more places where I see the comment "### use
svn_ra_serf__error_on_status() ? ". If the errors are not being handled
and if we can trigger it using some command, We will fix it. Your thoughts?
Thanks & Regards,
vijayaguru
Re: Improve error handling in svn_ra_serf__replay_range()
Posted by Philip Martin <ph...@wandisco.com>.
vijay <vi...@collab.net> writes:
> The function svn_ra_serf__replay_range() doesn't return any error
> while running 'svnrdump dump -rN:M <URL-deleted-in-HEAD>'[Issue #4100].
> It behaves as if the dump completed successfully.
>
> * subversion/libsvn_ra_serf/replay.c
> (svn_ra_serf__replay_range): Handle the errors from server's response as
> well as any errors found while running the serf loop.
Consider adding a regression test to svnrdump_tests.py.
> /* Run the serf loop. */
> - SVN_ERR(svn_ra_serf__context_run_wait(&replay_ctx->done, session, pool));
> + err = svn_ra_serf__context_run_wait(&replay_ctx->done, session, pool);
>
> + /* Handle the errors from server's response as well as any errors
> + found while running the serf loop. */
> + SVN_ERR(svn_error_compose_create(
> + svn_ra_serf__error_on_status(handler->sline.code,
> + handler->path,
> + handler->location),
> + err));
> +
When I see this sort of change I wonder if it needs to be applied
anywhere else. The call in update.c:svn_ra_serf__get_file also uses
SVN_ERR. Does it need the same fix?
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download