Daniel Shahaf wrote on Fri, Jul 30, 2010 at 14:46:29 +0300:
> Trivial, but I'm trying to avoid a context switch:
>
> [[[
> Index: subversion/libsvn_ra_neon/options.c
> ===================================================================
> --- subversion/libsvn_ra_neon/options.c (revision 980674)
> +++ subversion/libsvn_ra_neon/options.c (working copy)
> @@ -144,6 +144,8 @@ parse_capabilities(ne_request *req,
> *youngest_rev = SVN_INVALID_REVNUM;
>
> /* Start out assuming all capabilities are unsupported. */
> + apr_hash_set(ras->capabilities, SVN_RA_CAPABILITY_PARTIAL_REPLAY,
> + APR_HASH_KEY_STRING, capability_no);
> apr_hash_set(ras->capabilities, SVN_RA_CAPABILITY_DEPTH,
> APR_HASH_KEY_STRING, capability_no);
> apr_hash_set(ras->capabilities, SVN_RA_CAPABILITY_MERGEINFO,
> Index: subversion/libsvn_ra_serf/options.c
> ===================================================================
> --- subversion/libsvn_ra_serf/options.c (revision 980674)
> +++ subversion/libsvn_ra_serf/options.c (working copy)
> @@ -394,6 +394,8 @@ options_response_handler(serf_request_t *request,
> serf_bucket_t *hdrs = serf_bucket_response_get_headers(response);
>
> /* Start out assuming all capabilities are unsupported. */
> + apr_hash_set(orc->session->capabilities, SVN_RA_CAPABILITY_PARTIAL_REPLAY,
> + APR_HASH_KEY_STRING, capability_no);
> apr_hash_set(orc->session->capabilities, SVN_RA_CAPABILITY_DEPTH,
> APR_HASH_KEY_STRING, capability_no);
> apr_hash_set(orc->session->capabilities, SVN_RA_CAPABILITY_MERGEINFO,
> ]]]
>
> How would the lack of these lines cause neon/serf to behave when run against
> old servers? (should they just loop because apr_hash_get() would return NULL?
> but that's /not/ how they actually behave...)
>From IRC:
15:03 <@Bert> danielsh: (Nevermind.. it's the difference between returning
unknown capability.. and not supported capability when the
server doesn't support it)
15:04 <@danielsh> Bert: *nod*, "unknown capability" is what I think should
happen,
15:05 <@danielsh> but if that were the case,
15:05 <@danielsh> I'd expect the error to be reported (by folks who use pre-1.5
servers)
15:06 <@Bert> danielsh: The only way to get in that code path would be to use
svnsync to sync a partial repository. Maybe svnsync just
handles that error?
15:07 <@danielsh> Bert: true, svnsync/main.c:738 just filters the UNKNOWN out
15:07 <@Bert> *nod*.. see svnsync/main.c around line 738
15:07 <@danielsh> isn't that a bug?
15:07 <@danielsh> unknown != no (as you said)
15:08 <@Bert> Well.. the result in both cases is that you get the
SVN_ERR_RA_PARTIAL_REPLAY_NOT_SUPPORTED error from svnsync
15:08 <@danielsh> oh. and it leaks an error too
15:09 <@danielsh> Bert: true, but with a different error message
15:09 <@danielsh> you'd get SVN_ERR_UNKNOWN_CAPABILITY in one case but not in
the other
15:09 <@danielsh> "Don't know anything about capability '%s'"
15:09 <@Bert> So we should probably fix neon.. and then we can just use
SVN_ERR()
15:10 <@danielsh> I'll write that
So, here's an updated patch. I'll commit it in a few days if I don't hear
objections.
[[[
Fix a bug in the neon/serf capabilities exchange, and plug a related error
leak in svnsync.
* subversion/libsvn_ra_neon/options.c (parse_capabilities),
* subversion/libsvn_ra_serf/options.c (options_response_handler):
Initialize SVN_RA_CAPABILITY_PARTIAL_REPLAY in the capabilities hash.
* subversion/svnsync/main.c
(do_initialize):
Plug an error leak, and start considering "unknown capability" as an
actual error.
]]]
[[[
Index: subversion/libsvn_ra_neon/options.c
===================================================================
--- subversion/libsvn_ra_neon/options.c (revision 980674)
+++ subversion/libsvn_ra_neon/options.c (working copy)
@@ -144,6 +144,8 @@ parse_capabilities(ne_request *req,
*youngest_rev = SVN_INVALID_REVNUM;
/* Start out assuming all capabilities are unsupported. */
+ apr_hash_set(ras->capabilities, SVN_RA_CAPABILITY_PARTIAL_REPLAY,
+ APR_HASH_KEY_STRING, capability_no);
apr_hash_set(ras->capabilities, SVN_RA_CAPABILITY_DEPTH,
APR_HASH_KEY_STRING, capability_no);
apr_hash_set(ras->capabilities, SVN_RA_CAPABILITY_MERGEINFO,
Index: subversion/libsvn_ra_serf/options.c
===================================================================
--- subversion/libsvn_ra_serf/options.c (revision 980674)
+++ subversion/libsvn_ra_serf/options.c (working copy)
@@ -394,6 +394,8 @@ options_response_handler(serf_request_t *request,
serf_bucket_t *hdrs = serf_bucket_response_get_headers(response);
/* Start out assuming all capabilities are unsupported. */
+ apr_hash_set(orc->session->capabilities, SVN_RA_CAPABILITY_PARTIAL_REPLAY,
+ APR_HASH_KEY_STRING, capability_no);
apr_hash_set(orc->session->capabilities, SVN_RA_CAPABILITY_DEPTH,
APR_HASH_KEY_STRING, capability_no);
apr_hash_set(orc->session->capabilities, SVN_RA_CAPABILITY_MERGEINFO,
Index: subversion/svnsync/main.c
===================================================================
--- subversion/svnsync/main.c (revision 980674)
+++ subversion/svnsync/main.c (working copy)
@@ -735,14 +735,11 @@ do_initialize(svn_ra_session_t *to_session,
&server_supports_partial_replay,
SVN_RA_CAPABILITY_PARTIAL_REPLAY,
pool);
- if (err && err->apr_err == SVN_ERR_UNKNOWN_CAPABILITY)
- {
- svn_error_clear(err);
- server_supports_partial_replay = FALSE;
- }
+ if (err && err->apr_err != SVN_ERR_UNKNOWN_CAPABILITY)
+ return svn_error_return(err);
- if (!server_supports_partial_replay)
- return svn_error_create(SVN_ERR_RA_PARTIAL_REPLAY_NOT_SUPPORTED, NULL,
+ if (err || !server_supports_partial_replay)
+ return svn_error_create(SVN_ERR_RA_PARTIAL_REPLAY_NOT_SUPPORTED, err,
NULL);
}
]]]
Daniel
(btw, should the "partial replay capability present" check be done at every
connection, rather than only in 'svnsync init'? Well, maybe...)
