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

Re: [PATCH] neon capabilities exchange

From: Daniel Shahaf <d.s_at_daniel.shahaf.name>
Date: Fri, 30 Jul 2010 15:30:19 +0300

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...)
Received on 2010-07-30 14:32:17 CEST

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