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 2008/07/03 15:12:27 UTC
Re: svn commit: r31900 - in trunk/subversion: libsvn_client tests/cmdline
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Let me record my study so far on this.
Context from test merge_tests-61.
On Pristine working copy of merge_tests-61
1)svn merge -r1:4 http://localhost/svn/merge_tests-61/A/D/gamma@4
A_copy/D/gamma
2)svn merge -r1:5 http://localhost/svn/merge_tests-61/A A_copy --force
Prior to this commit when we run 2)
- ->we open the diff editor for '/A' with default_start=2 and end
revision=5.
- ->we call set_path on /A/D/gamma for start_rev=4
- ->finish_report()
After this commit
- ->we open the diff editor for '/A' with default_start=2 and end
revision=5.
*No set_path on /A/D/gamma is called as it is anyway going to be deleted.
- ->finish_report()
Not calling set_path on /A/D/gamma causes the serf to fail on
'finish_report'.
It fails at 'svn_ra_serf__get_baseline_info' called via the callback
which calls 'svn_ra_get_dir2'.
I could see neon also makes a call to 'svn_ra_get_dir2' with almost same
set of parameters.
One difference I could observe that may be the cause is not sure though.
When neon tries to retrieve the 'dir properties at rev 2' it makes a
request to '/svn/merge_tests-61/!svn/bc/2/A'.
Whereas serf makes a request to '/svn/merge_tests-1/A'.
With regards
Kamesh Jayachandran
Lieven Govaerts wrote:
> kameshj@tigris.org wrote:
>> Author: kameshj
>> Date: Fri Jun 27 07:56:04 2008
>> New Revision: 31900
>>
>> Log:
>> Fix issue #3067, 'subtrees that don't exist at the start or end of a
>> merge
>> range shouldn't break the merge'
>>
>> Fixes the core problem of issue #3076: Don't try to describe invalid
>> subtrees to the merge report editor. Also fixes the ancillary problem
>> described in
>> http://subversion.tigris.org/issues/show_bug.cgi?id=3067#desc34
>> where subtrees with empty mergeinfo don't get their mergeinfo updated
>> properly.
>>
>> * subversion/libsvn_client/merge.c
>> (init_rangelist, push_range): New helpers.
>> (prepare_subtree_ranges): New helper for filter_merged_revisions() when
>> operating on subtrees.
>> (filter_merged_revisions): Use new prepare_subtree_ranges() helper to
>> address various issue #3067 problems. Do away with entry argument
>> and instead pass a svn_client__merge_path_t representing the working
>> copy
>> target path, along with another svn_client__merge_path_t
>> representing the
>> path's parent if the path is a subtree. Cascade some additional
>> args from
>> calculate_remaining_ranges().
>> (calculate_remaining_ranges): Improve doc string. Tweak signature
>> to get
>> svn_client__merge_path_t for target and its nearest parent if target
>> is a
>> subtree. Update call to filter_merged_revisions().
>> (populate_remaining_ranges, do_file_merge): Update calls to
>> calculate_remaining_ranges().
>> (get_mergeinfo_walk_cb): Stop doing any 'preemptive' elision on
>> subtrees.
>> This leaves a local mod on the subtree and the subtree is to be
>> deleted by
>> the requested merge,the local mod will cause the subtree to be skipped.
>> Just add subtrees with empty mergeinfo and let prepare_subtree_ranges()
>> handle it.
>> (get_mergeinfo_paths): Note in doc string that we always include
>> children
>> with empty mergeinfo.
>>
> This commit seems to break 4 merge tests over ra_serf only.
>
> The error message for each of this is:
>
> svn: Premature EOF seen from server
>
> I don't know what specifically in your patch can cause this situation,
> and I don't have a lot of time right now to look into it. If anyone else
> can, great. Otherwise I'll use this as a todo reminder mail.
>
> Lieven
>
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFIbOxb3WHvyO0YTCwRAq30AJ0cbLQRglS9gF1ckneRRUruKcF5AQCfYmPA
bT5beQfvT05nfij6vK9+qpQ=
=d+hU
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31900 - in trunk/subversion: libsvn_client tests/cmdline
Posted by sv...@mobsol.be.
Quoting Kamesh Jayachandran <ka...@collab.net>:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> >>
> > Well, we do return other status codes, and those will be returned from
> > the call to serf_context_run, but as you found out, you shouldn't reuse
> > that connection anymore. I think there are a few places where we rely on
> > serf returning our particular error code. Probably that's why you found
> > some of the tests failing.
>
> Committed the fix at r32056, All tests pass now over serf.
>
Cool, thanks.
>
> > What you're trying to fix is how ra_serf handles invalid XML response
> > bodies. While this is a bug on its own, the real reason the tests are
> > failing is that the server returns invalid XML for a locations-segment
> > report on a non-existant node. Attached patch fixes mod_dav_svn to
> > return an empty (opening & closing tag) report in that case.
>
>
> Better approach to fix this would be to change the http response status
> to '404' for non-existent nodes and stub the ra (neon|serf)layers to a
> svn_error_t with apr_err set to 'SVN_ERR_FS_NOT_FOUND'.
>
Agreed. I'll have a 2nd go at this patch tonight.
Lieven
----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31900 - in trunk/subversion: libsvn_client tests/cmdline
Posted by Kamesh Jayachandran <ka...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
>>
> Well, we do return other status codes, and those will be returned from
> the call to serf_context_run, but as you found out, you shouldn't reuse
> that connection anymore. I think there are a few places where we rely on
> serf returning our particular error code. Probably that's why you found
> some of the tests failing.
Committed the fix at r32056, All tests pass now over serf.
> What you're trying to fix is how ra_serf handles invalid XML response
> bodies. While this is a bug on its own, the real reason the tests are
> failing is that the server returns invalid XML for a locations-segment
> report on a non-existant node. Attached patch fixes mod_dav_svn to
> return an empty (opening & closing tag) report in that case.
Better approach to fix this would be to change the http response status
to '404' for non-existent nodes and stub the ra (neon|serf)layers to a
svn_error_t with apr_err set to 'SVN_ERR_FS_NOT_FOUND'.
With regards
Kamesh Jayachandran
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFIdgZu3WHvyO0YTCwRAvCfAKCHPnSAkuB9+vWoviDQyaUMETavDACggxEo
gFho7RJ+QTRZxuAM2Nu53GU=
=Y8D8
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31900 - in trunk/subversion: libsvn_client tests/cmdline
Posted by Lieven Govaerts <sv...@mobsol.be>.
Nice catch Kamesh!
Kamesh Jayachandran wrote:
> I got the crux of the problem.
>
> Our 'serf_response_handler_t' implementation is wrong.
>
> According to 'serf_response_handler_t' doc implementation can return
> either one of the following,
>
> APR_EAGAIN, APR_EOF, APR_SUCCESS.
>
> Our libsvn_ra_serf's implementation of
> 'serf_response_handler_t'(handle_response) returns arbitrary svn error
> codes and hence 'serf system' thinks that 'EOF on a request has not
> reached' and hence this failure.
>
Well, we do return other status codes, and those will be returned from
the call to serf_context_run, but as you found out, you shouldn't reuse
that connection anymore. I think there are a few places where we rely on
serf returning our particular error code. Probably that's why you found
some of the tests failing.
What you're trying to fix is how ra_serf handles invalid XML response
bodies. While this is a bug on its own, the real reason the tests are
failing is that the server returns invalid XML for a locations-segment
report on a non-existant node. Attached patch fixes mod_dav_svn to
return an empty (opening & closing tag) report in that case.
Lieven
Re: svn commit: r31900 - in trunk/subversion: libsvn_client tests/cmdline
Posted by Kamesh Jayachandran <ka...@collab.net>.
I got the crux of the problem.
Our 'serf_response_handler_t' implementation is wrong.
According to 'serf_response_handler_t' doc implementation can return
either one of the following,
APR_EAGAIN, APR_EOF, APR_SUCCESS.
Our libsvn_ra_serf's implementation of
'serf_response_handler_t'(handle_response) returns arbitrary svn error
codes and hence 'serf system' thinks that 'EOF on a request has not
reached' and hence this failure.
I am attaching the work in progress patch here.(It fails few tests over
my 'maintainer-mode' build).
With regards
Kamesh Jayachandran
Kamesh Jayachandran wrote:
>
> One more update about this which is still closer.
>
> Post r31900 When we merge -rX:Y, we do run location-segments report on
> each corresponding_src_url_of_subtree-with-mergeinfo@Y.
>
> corresponding_src_url_of_subtree-with-mergeinfo@Y may be non-existent.
>
> In merge-tests-61 we run location_segments_report for
>
> http://localhost/svn/merge_tests-61/A/D/gamma@5
>
> which is non-existent since r4.
>
> In 'svn_ra_serf__get_location_segments' we have the following code.
>
> <snip>
> svn_ra_serf__request_create(handler);
>
> err = svn_ra_serf__context_run_wait(&gls_ctx->done, session, pool);
>
> if (gls_ctx->error || parser_ctx->error)
> {
> svn_error_clear(err);
> err = SVN_NO_ERROR;
> SVN_ERR(gls_ctx->error);
> SVN_ERR(parser_ctx->error);
> }
> </snip>
>
> When requests fail like above the request is not getting removed from
> the serf's opaque data structure 'handler->conn->conn->requests' and
> hence subsequent request on the same connection fails while reading the
> first request's response(which should have been removed upon failure).
>
>
> With regards
> Kamesh Jayachandran
>
>
>
>
>
> Kamesh Jayachandran wrote:
>> Let me record my study so far on this.
>
>> Context from test merge_tests-61.
>
>> On Pristine working copy of merge_tests-61
>> 1)svn merge -r1:4 http://localhost/svn/merge_tests-61/A/D/gamma@4
>> A_copy/D/gamma
>> 2)svn merge -r1:5 http://localhost/svn/merge_tests-61/A A_copy --force
>
>
>> Prior to this commit when we run 2)
>> ->we open the diff editor for '/A' with default_start=2 and end
>> revision=5.
>> ->we call set_path on /A/D/gamma for start_rev=4
>> ->finish_report()
>
>> After this commit
>> ->we open the diff editor for '/A' with default_start=2 and end
>> revision=5.
>> *No set_path on /A/D/gamma is called as it is anyway going to be deleted.
>> ->finish_report()
>
>> Not calling set_path on /A/D/gamma causes the serf to fail on
>> 'finish_report'.
>
>> It fails at 'svn_ra_serf__get_baseline_info' called via the callback
>> which calls 'svn_ra_get_dir2'.
>
>> I could see neon also makes a call to 'svn_ra_get_dir2' with almost same
>> set of parameters.
>
>> One difference I could observe that may be the cause is not sure though.
>
>> When neon tries to retrieve the 'dir properties at rev 2' it makes a
>> request to '/svn/merge_tests-61/!svn/bc/2/A'.
>
>> Whereas serf makes a request to '/svn/merge_tests-1/A'.
>
>
>> With regards
>> Kamesh Jayachandran
>> Lieven Govaerts wrote:
>>> kameshj@tigris.org wrote:
>>>> Author: kameshj
>>>> Date: Fri Jun 27 07:56:04 2008
>>>> New Revision: 31900
>>>>
>>>> Log:
>>>> Fix issue #3067, 'subtrees that don't exist at the start or end of a
>>>> merge
>>>> range shouldn't break the merge'
>>>>
>>>> Fixes the core problem of issue #3076: Don't try to describe invalid
>>>> subtrees to the merge report editor. Also fixes the ancillary problem
>>>> described in
>>>> http://subversion.tigris.org/issues/show_bug.cgi?id=3067#desc34
>>>> where subtrees with empty mergeinfo don't get their mergeinfo updated
>>>> properly.
>>>>
>>>> * subversion/libsvn_client/merge.c
>>>> (init_rangelist, push_range): New helpers.
>>>> (prepare_subtree_ranges): New helper for filter_merged_revisions() when
>>>> operating on subtrees.
>>>> (filter_merged_revisions): Use new prepare_subtree_ranges() helper to
>>>> address various issue #3067 problems. Do away with entry argument
>>>> and instead pass a svn_client__merge_path_t representing the working
>>>> copy
>>>> target path, along with another svn_client__merge_path_t
>>>> representing the
>>>> path's parent if the path is a subtree. Cascade some additional
>>>> args from
>>>> calculate_remaining_ranges().
>>>> (calculate_remaining_ranges): Improve doc string. Tweak signature
>>>> to get
>>>> svn_client__merge_path_t for target and its nearest parent if target
>>>> is a
>>>> subtree. Update call to filter_merged_revisions().
>>>> (populate_remaining_ranges, do_file_merge): Update calls to
>>>> calculate_remaining_ranges().
>>>> (get_mergeinfo_walk_cb): Stop doing any 'preemptive' elision on
>>>> subtrees.
>>>> This leaves a local mod on the subtree and the subtree is to be
>>>> deleted by
>>>> the requested merge,the local mod will cause the subtree to be skipped.
>>>> Just add subtrees with empty mergeinfo and let prepare_subtree_ranges()
>>>> handle it.
>>>> (get_mergeinfo_paths): Note in doc string that we always include
>>>> children
>>>> with empty mergeinfo.
>>>>
>>> This commit seems to break 4 merge tests over ra_serf only.
>>> The error message for each of this is:
>>> svn: Premature EOF seen from server
>>> I don't know what specifically in your patch can cause this situation,
>>> and I don't have a lot of time right now to look into it. If anyone else
>>> can, great. Otherwise I'll use this as a todo reminder mail.
>>> Lieven
>
> ---------------------------------------------------------------------
> 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: svn commit: r31900 - in trunk/subversion: libsvn_client tests/cmdline
Posted by Kamesh Jayachandran <ka...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
One more update about this which is still closer.
Post r31900 When we merge -rX:Y, we do run location-segments report on
each corresponding_src_url_of_subtree-with-mergeinfo@Y.
corresponding_src_url_of_subtree-with-mergeinfo@Y may be non-existent.
In merge-tests-61 we run location_segments_report for
http://localhost/svn/merge_tests-61/A/D/gamma@5
which is non-existent since r4.
In 'svn_ra_serf__get_location_segments' we have the following code.
<snip>
svn_ra_serf__request_create(handler);
err = svn_ra_serf__context_run_wait(&gls_ctx->done, session, pool);
if (gls_ctx->error || parser_ctx->error)
{
svn_error_clear(err);
err = SVN_NO_ERROR;
SVN_ERR(gls_ctx->error);
SVN_ERR(parser_ctx->error);
}
</snip>
When requests fail like above the request is not getting removed from
the serf's opaque data structure 'handler->conn->conn->requests' and
hence subsequent request on the same connection fails while reading the
first request's response(which should have been removed upon failure).
With regards
Kamesh Jayachandran
Kamesh Jayachandran wrote:
> Let me record my study so far on this.
>
> Context from test merge_tests-61.
>
> On Pristine working copy of merge_tests-61
> 1)svn merge -r1:4 http://localhost/svn/merge_tests-61/A/D/gamma@4
> A_copy/D/gamma
> 2)svn merge -r1:5 http://localhost/svn/merge_tests-61/A A_copy --force
>
>
> Prior to this commit when we run 2)
> ->we open the diff editor for '/A' with default_start=2 and end
> revision=5.
> ->we call set_path on /A/D/gamma for start_rev=4
> ->finish_report()
>
> After this commit
> ->we open the diff editor for '/A' with default_start=2 and end
> revision=5.
> *No set_path on /A/D/gamma is called as it is anyway going to be deleted.
> ->finish_report()
>
> Not calling set_path on /A/D/gamma causes the serf to fail on
> 'finish_report'.
>
> It fails at 'svn_ra_serf__get_baseline_info' called via the callback
> which calls 'svn_ra_get_dir2'.
>
> I could see neon also makes a call to 'svn_ra_get_dir2' with almost same
> set of parameters.
>
> One difference I could observe that may be the cause is not sure though.
>
> When neon tries to retrieve the 'dir properties at rev 2' it makes a
> request to '/svn/merge_tests-61/!svn/bc/2/A'.
>
> Whereas serf makes a request to '/svn/merge_tests-1/A'.
>
>
> With regards
> Kamesh Jayachandran
> Lieven Govaerts wrote:
>> kameshj@tigris.org wrote:
>>> Author: kameshj
>>> Date: Fri Jun 27 07:56:04 2008
>>> New Revision: 31900
>>>
>>> Log:
>>> Fix issue #3067, 'subtrees that don't exist at the start or end of a
>>> merge
>>> range shouldn't break the merge'
>>>
>>> Fixes the core problem of issue #3076: Don't try to describe invalid
>>> subtrees to the merge report editor. Also fixes the ancillary problem
>>> described in
>>> http://subversion.tigris.org/issues/show_bug.cgi?id=3067#desc34
>>> where subtrees with empty mergeinfo don't get their mergeinfo updated
>>> properly.
>>>
>>> * subversion/libsvn_client/merge.c
>>> (init_rangelist, push_range): New helpers.
>>> (prepare_subtree_ranges): New helper for filter_merged_revisions() when
>>> operating on subtrees.
>>> (filter_merged_revisions): Use new prepare_subtree_ranges() helper to
>>> address various issue #3067 problems. Do away with entry argument
>>> and instead pass a svn_client__merge_path_t representing the working
>>> copy
>>> target path, along with another svn_client__merge_path_t
>>> representing the
>>> path's parent if the path is a subtree. Cascade some additional
>>> args from
>>> calculate_remaining_ranges().
>>> (calculate_remaining_ranges): Improve doc string. Tweak signature
>>> to get
>>> svn_client__merge_path_t for target and its nearest parent if target
>>> is a
>>> subtree. Update call to filter_merged_revisions().
>>> (populate_remaining_ranges, do_file_merge): Update calls to
>>> calculate_remaining_ranges().
>>> (get_mergeinfo_walk_cb): Stop doing any 'preemptive' elision on
>>> subtrees.
>>> This leaves a local mod on the subtree and the subtree is to be
>>> deleted by
>>> the requested merge,the local mod will cause the subtree to be skipped.
>>> Just add subtrees with empty mergeinfo and let prepare_subtree_ranges()
>>> handle it.
>>> (get_mergeinfo_paths): Note in doc string that we always include
>>> children
>>> with empty mergeinfo.
>>>
>> This commit seems to break 4 merge tests over ra_serf only.
>
>> The error message for each of this is:
>
>> svn: Premature EOF seen from server
>
>> I don't know what specifically in your patch can cause this situation,
>> and I don't have a lot of time right now to look into it. If anyone else
>> can, great. Otherwise I'll use this as a todo reminder mail.
>
>> Lieven
>
- ---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFIcjna3WHvyO0YTCwRAr2NAJ9846tQDHZNL5mWhiAdO89//fmUsgCfTr89
+arVWyme3ss/q43UQPVH4B8=
=5uIl
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org