On Feb 13, 2008 12:14 AM, <kfogel_at_tigris.org> wrote:
> Author: kfogel
> Date: Wed Feb 13 00:14:42 2008
> New Revision: 29314
>
> Log:
> Report mergeinfo capability more efficiently over ra_svn and ra_local.
> For ra_svn, this avoids the extra round trip of r29263.
>
> Suggested by: glasser
>
> #########################################################################
> ### ###
> ### STATUS ###
> ### ###
> ### ra_dav and ra_local pass; ra_svn gets a typical error as in ###
> ### http://subversion.tigris.org/hacking.html#debugging-ra-svn: ###
> ### ###
> ### $ svn co svn://localhost/repos wc ###
> ### subversion/libsvn_ra_svn/marshal.c:755: (apr_err=210004) ###
> ### svn: Malformed network data ###
> ### ###
> ### There's a protocol trace at http://pastebin.ca/902213; nothing ###
> ### jumped out at me, but it's late. Will pick up in after sleep. ###
> ### ###
> #########################################################################
>
> First make an interface to ask the repository if it supports mergeinfo:
>
> * subversion/include/svn_repos.h
> (svn_repos_has_capability): New prototype.
> (SVN_REPOS_CAPABILITY_MERGEINFO): New capability.
>
> * subversion/libsvn_repos/repos.h
> (repository_capabilities): New field.
>
> * subversion/libsvn_repos/repos.c
> (create_svn_repos_t): Initialize new repository_capabilities field.
> (capability_yes, capability_no): New static constant strings.
> (svn_repos_has_capability): New function.
>
> Then make svnserve use it, and report back to the client early:
>
> * subversion/svnserve/serve.c
> (serve): Only report mergeinfo capability if repos supports it.
>
> * subversion/libsvn_ra_svn/ra_svn.h
> (svn_ra_svn__session_baton_t): Remove repository_supports_mergeinfo
> field that was introduced in r29263.
>
> * subversion/libsvn_ra_svn/client.c
> (open_session): Expect repos capabilities to arrive separately.
> Remove repository_supports_mergeinfo initialization.
> (ra_svn_has_capability): Remove extra round trip introduced in r29263.
>
> * subversion/include/svn_ra_svn.h
> (svn_ra_svn_set_capabilities): Document idempotency.
>
> * subversion/libsvn_ra_svn/protocol: Document where repository
> capabilities fit in.
>
> Make ra_local use the new interface too:
>
> * subversion/libsvn_ra_local/ra_local.h
> (svn_ra_local__session_baton_t): Remove repository_supports_mergeinfo
> field that was introduced in r29263.
>
> * subversion/libsvn_ra_local/ra_plugin.c
> (svn_ra_local__open): Remove repository_supports_mergeinfo initialization.
> (svn_ra_local__has_capability): Just ask repository directly if it
> has mergeinfo capability, and while we're at it, tighten the
> conditional structure introduced in r29263.
>
> Unrelatedly, make mod_dav transmit an svn_error_t correctly:
>
> * subversion/mod_dav_svn/reports/mergeinfo.c
> (dav_svn__get_mergeinfo_report): Don't flush before returning error,
> as flushing might obscure the error due to certain intricacies of
> mod_dav's plumbing.
It would be great if this got review from somebody else familiar with
DAV... I'm unable to revision this myself, and it does seem like a
subtle change.
> Modified: branches/mergeinfo-capability/subversion/libsvn_ra_svn/protocol
> URL: http://svn.collab.net/viewvc/svn/branches/mergeinfo-capability/subversion/libsvn_ra_svn/protocol?pathrev=29314&r1=29313&r2=29314
> ==============================================================================
> --- branches/mergeinfo-capability/subversion/libsvn_ra_svn/protocol (original)
> +++ branches/mergeinfo-capability/subversion/libsvn_ra_svn/protocol Wed Feb 13 00:14:42 2008
> @@ -119,14 +119,18 @@
> an authentication request, which is a command response whose arguments
> match the prototype:
>
> - auth-request: ( ( mech:word ... ) realm:string )
> + auth-request: ( ( mech:word ... ) realm:string ( cap:word ... ) )
>
> The mech values give a list of SASL mechanisms supported by the
> server. The realm string is similar to an HTTP authentication realm
> as defined in [RFC 2617]; it allows the server to indicate which of
> -several protection spaces the server wishes to authenticate in. If
> -the mechanism list is empty, then no authentication is required and no
> -further action takes place as part of the authentication challenge;
> +several protection spaces the server wishes to authenticate in. The
> +cap values list the repository capabilities (that is, capabilities
> +that require both the server and repository to support them before the
> +server can claim them as capabilities, e.g., SVN_RA_SVN_CAP_MERGEINFO).
> +
> +If the mechanism list is empty, then no authentication is required and
> +no further action takes place as part of the authentication challenge;
> otherwise, the client responds with a tuple matching the prototype:
You've changed the doc of the wrong part: I think you added the cap
list to repos-info, not auth-request.
> +/* Repository supports the capability. */
> +static const char *capability_yes = "yes";
> +/* Repository does not support the capability. */
> +static const char *capability_no = "no";
> +
> +svn_error_t *
> +svn_repos_has_capability(svn_repos_t *repos,
> + svn_boolean_t *has,
> + const char *capability,
> + apr_pool_t *pool)
> +{
> + const char *val = apr_hash_get(repos->repository_capabilities,
> + capability, APR_HASH_KEY_STRING);
> +
> + if (val == capability_yes)
> + {
> + *has = TRUE;
> + }
> + else if (val == capability_no)
> + {
> + *has = FALSE;
> + }
> + /* Else don't know, so investigate. */
> + else if (strcmp(capability, SVN_REPOS_CAPABILITY_MERGEINFO) == 0)
> + {
> + svn_error_t *err;
> + svn_fs_root_t *root;
> + apr_hash_t *ignored_mergeoutput;
> + apr_array_header_t *paths = apr_array_make(pool, 1,
> + sizeof(char *));
> +
> + SVN_ERR(svn_fs_revision_root(&root, repos->fs, 0, pool));
> + APR_ARRAY_PUSH(paths, const char *) = "";
> + err = svn_fs_get_mergeinfo(&ignored_mergeoutput, root,
> + paths, FALSE, FALSE, pool);
I really think this should ask the FS directly if it supports
mergeinfo, instead of relying on error handling (which has a tendancy
to leak memory, etc). Yeah, this requires adding Yet Another Layer of
capabilities, but the two backends already have this function.
--dave
> +
> + if (err)
> + {
> + if (err->apr_err == SVN_ERR_UNSUPPORTED_FEATURE)
> + {
> + svn_error_clear(err);
> + apr_hash_set(repos->repository_capabilities,
> + SVN_REPOS_CAPABILITY_MERGEINFO,
> + APR_HASH_KEY_STRING, capability_no);
> + *has = FALSE;
> + }
> + else if (err->apr_err == SVN_ERR_FS_NOT_FOUND)
> + {
> + /* Mergeinfo requests use relative paths, and anyway we're
> + in r0, so we're likely to get this error -- but it
> + means the repository supports mergeinfo! */
> + svn_error_clear(err);
> + apr_hash_set(repos->repository_capabilities,
> + SVN_REPOS_CAPABILITY_MERGEINFO,
> + APR_HASH_KEY_STRING, capability_yes);
> + *has = TRUE;
> + }
> + else
> + {
> + return err;
> + }
> + }
> + else
> + {
> + apr_hash_set(repos->repository_capabilities,
> + SVN_REPOS_CAPABILITY_MERGEINFO,
> + APR_HASH_KEY_STRING, capability_yes);
> + *has = TRUE;
> + }
> + }
> + else
> + {
> + return svn_error_createf(SVN_ERR_UNKNOWN_CAPABILITY, 0,
> + _("unknown capability '%s'"), capability);
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
> +
> svn_fs_t *
> svn_repos_fs(svn_repos_t *repos)
> {
>
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-02-13 20:32:36 CET