On Wed, 2 Feb 2005 kfogel@collab.net wrote:
> After I send this, I can go through and take care of some of the more
> trivial cleanups, if you want. But I wanted to first write it all up,
> so we have something to refer to.
>
We agreed to this yesterday, so I will leave trivialia for you;) Below are
some answers to your questions.
> Here we go:
>
> lundblad@tigris.org writes:
> > Log:
> >
> > [...]
>
> A few inconsequential typos in the log message. I just fixed them
> directly with propedit. I wish we (er, I) would set up diffing
> functionality for revprop changes, now that it's possible, so you
> could easily see what the changes were. Oh well, no big deal.
>
I seem to be notorious in this respect:-(
> > --- trunk/subversion/include/svn_ra.h (original)
> > +++ trunk/subversion/include/svn_ra.h Thu Jan 20 15:05:59 2005
> > @@ -277,16 +277,551 @@
> >
> > +svn_error_t *svn_ra_do_update (svn_ra_session_t *session,
> > + const svn_ra_reporter_t **reporter,
> > + void **report_baton,
> > + svn_revnum_t revision_to_update_to,
> > + const char *update_target,
> > + svn_boolean_t recurse,
> > + const svn_delta_editor_t *update_editor,
> > + void *update_baton,
> > + apr_pool_t *pool);
> > +
>
> Again, nice to say specifically that *reporter and *report_baton get
> allocated in pool.
>
Hmmm, to be pedantic, one could say that this is overspecification.
Sometimes the reporters are just static structs whose address is returned.
I think ghudson has pointed out that it is more appropriate to talk about
guaranteed lifetimes in general, and I tend to agree. But that would be a
massive change. Maybe better to be consistent.
> > +/**
> > + * @since New in 1.2.
> > + *
> > + * Ask the RA layer to 'switch' a working copy to a new
> > + * @a switch_url; it's another form of @c svn_ra_do_update().
> > + *
> > + * The client initially provides an @a switch_editor/@a baton to the RA
> > + * layer; this editor contains knowledge of where the change will
> > + * begin in the working copy (when @c open_root() is called).
> > + *
> > + * In return, the client receives a @a reporter/@a report_baton. The
> > + * client then describes its working-copy revision numbers by making
> > + * calls into the @a reporter structure; the RA layer assumes that all
> > + * paths are relative to the URL used to create @a session_baton.
> > + *
> > + * When finished, the client calls @a reporter->finish_report(). The
> > + * RA layer then does a complete drive of @a switch_editor, ending with
> > + * @c close_edit(), to switch the working copy.
> > + *
> > + * @a switch_target is an optional single path component will restrict
> > + * the scope of things affected by the switch to an entry in the
> > + * directory represented by the @a session's URL, or empty if the
> > + * entire directory is meant to be switched.
> > + *
> > + * The working copy will be switched to @a revision_to_switch_to, or the
> > + * "latest" revision if this arg is invalid.
> > + *
> > + * The caller may not perform any RA operations using
> > + * @a session before finishing the report, and may not perform
> > + * any RA operations using @a session_baton from within the editing
> > + * operations of @a switch_editor.
> > + *
> > + * Use @a pool for memory allocation.
> > + */
> > +svn_error_t *svn_ra_do_switch (svn_ra_session_t *session,
> > + const svn_ra_reporter_t **reporter,
> > + void **report_baton,
> > + svn_revnum_t revision_to_switch_to,
> > + const char *switch_target,
> > + svn_boolean_t recurse,
> > + const char *switch_url,
> > + const svn_delta_editor_t *switch_editor,
> > + void *switch_baton,
> > + apr_pool_t *pool);
>
> Ditto about allocation.
>
> > +/**
> > + * @since New in 1.2.
> > + *
> > + * Ask the RA layer to describe the status of a working copy with respect
> > + * to @a revision of the repository (or HEAD, if @a revision is invalid).
> > + *
> > + * The client initially provides an @a status_editor/@a baton to the RA
> > + * layer; this editor contains knowledge of where the change will
> > + * begin in the working copy (when @c open_root() is called).
> > + *
> > + * In return, the client receives a @a reporter/@a report_baton. The
> > + * client then describes its working-copy revision numbers by making
> > + * calls into the @a reporter structure; the RA layer assumes that all
> > + * paths are relative to the URL used to open @a session.
> > + *
> > + * When finished, the client calls @a reporter->finish_report(). The RA
> > + * layer then does a complete drive of @a status_editor, ending with
> > + * @c close_edit(), to report, essentially, what would be modified in
> > + * the working copy were the client to call @c do_update().
> > + * @a status_target is an optional single path component will restrict
> > + * the scope of the status report to an entry in the directory
> > + * represented by the @a session_baton's URL, or empty if the entire
> > + * directory is meant to be examined.
> > + *
> > + * The caller may not perform any RA operations using @a session
> > + * before finishing the report, and may not perform any RA operations
> > + * using @a session from within the editing operations of @a status_editor.
> > + *
> > + * Use @a pool for memory allocation.
> > + */
> > +svn_error_t *svn_ra_do_status (svn_ra_session_t *session,
> > + const svn_ra_reporter_t **reporter,
> > + void **report_baton,
> > + const char *status_target,
> > + svn_revnum_t revision,
> > + svn_boolean_t recurse,
> > + const svn_delta_editor_t *status_editor,
> > + void *status_baton,
> > + apr_pool_t *pool);
>
> Ditto about allocation.
>
> > +/**
> > + * @since New in 1.2.
> > + *
> > + * Ask the RA layer to 'diff' a working copy against @a versus_url;
> > + * it's another form of @c svn_ra_do_update().
> > + *
> > + * [Please note: this function cannot be used to diff a single
> > + * file, only a working copy directory. See the @c svn_ra_do_switch()
> > + * function for more details.]
> > + *
> > + * The client initially provides a @a diff_editor/@a baton to the RA
> > + * layer; this editor contains knowledge of where the common diff
> > + * root is in the working copy (when @c open_root() is called).
> > + *
> > + * In return, the client receives a @a reporter/@a report_baton. The
> > + * client then describes its working-copy revision numbers by making
> > + * calls into the @a reporter structure; the RA layer assumes that all
> > + * paths are relative to the URL used to open @a session.
> > + *
> > + * When finished, the client calls @a reporter->finish_report(). The
> > + * RA layer then does a complete drive of @a diff_editor, ending with
> > + * @c close_edit(), to transmit the diff.
> > + *
> > + * @a diff_target is an optional single path component will restrict
> > + * the scope of the diff to an entry in the directory represented by
> > + * the @a session's URL, or empty if the entire directory is meant to be
> > + * one of the diff paths.
> > + *
> > + * The working copy will be diffed against @a versus_url as it exists
> > + * in revision @a revision, or as it is in head if @a revision is
> > + * @c SVN_INVALID_REVNUM.
> > + *
> > + * Use @a ignore_ancestry to control whether or not items being
> > + * diffed will be checked for relatedness first. Unrelated items
> > + * are typically transmitted to the editor as a deletion of one thing
> > + * and the addition of another, but if this flag is @c TRUE,
> > + * unrelated items will be diffed as if they were related.
> > + *
> > + * The caller may not perform any RA operations using @a session before
> > + * finishing the report, and may not perform any RA operations using
> > + * @a session from within the editing operations of @a diff_editor.
> > + *
> > + * Use @a pool for memory allocation.
> > + */
> > +svn_error_t *svn_ra_do_diff (svn_ra_session_t *session,
> > + const svn_ra_reporter_t **reporter,
> > + void **report_baton,
> > + svn_revnum_t revision,
> > + const char *diff_target,
> > + svn_boolean_t recurse,
> > + svn_boolean_t ignore_ancestry,
> > + const char *versus_url,
> > + const svn_delta_editor_t *diff_editor,
> > + void *diff_baton,
> > + apr_pool_t *pool);
>
> Ditto about allocation.
>
> > +/** @since New in 1.2.
> > + *
> > + * Set @a *uuid to the repository's UUID.
> > + *
> > + * NOTE: the UUID has the same lifetime as the @a session.
> > + *
> > + * Use @a pool for temporary memory allocation.
> > + */
> > +svn_error_t *svn_ra_get_uuid (svn_ra_session_t *session,
> > + const char **uuid,
> > + apr_pool_t *pool);
>
> Hmm, why do we have the word "temporary" in this doc string? Surely
> the allocation of *uuid is permanent for as long as pool lasts :-).
>
I think the intent is to distinguis between memory allocated for temporary
use and the return value whihch is allocated in the session pool. But I
don't think it has to stay.
> > +/**
> > + * @since New in 1.2.
> > + *
> > + * Set @a *url to the repository's root URL. The value will not include
> > + * a trailing '/'. The returned URL is guaranteed to be a prefix of the
> > + * @a session's URL.
> > + *
> > + * NOTE: the URL has the same lifetime as the @a session.
> > + *
> > + * Use @a pool for temporary memory allocation.
> > + */
> > +svn_error_t *svn_ra_get_repos_root (svn_ra_session_t *session,
> > + const char **url,
> > + apr_pool_t *pool);
>
> In other words, *url is not allocated in pool? That's so unusual that
> we should probably state it even more explicitly, for example:
>
> /**
> * @since New in 1.2.
> *
> * Set @a *url to the repository's root URL. The value will not include
> * a trailing '/'. The returned URL is guaranteed to be a prefix of the
> * @a session's URL.
> *
> * Allocate @a *url in @a session's pool, so it has the same
> * lifetime as @a session, and use @a pool only for temporary
> * allocation.
> */
>
As in the earlier comment, I think this is overspecification. Maybe we
should take the chance to make tis API consistent with others and only
guarantee that the string is valid during the lifetime of pool? Same
applies to svn_ra_get_uuid.
> > +/** > > + * @since New in 1.2.
> > + *
> > + * Set @a *locations to the locations at the repository revisions
> > + * @a location_revisions of the file @a path present at the repository in
> > + * revision @a peg_revision. @a path is relative to the URL to which
> > + * @a session was opened. @a location_revisions is an array of
> > + * @c svn_revnum_t's. @a *locations will be a mapping from the revisions to
> > + * their appropriate absolute paths. If the file doesn't exist in a
> > + * location_revision, that revision will be ignored.
> > + *
> > + * Use @a pool for all allocations.
> > + *
> > + * NOTE: This functionality is not available in pre-1.1 servers. If the
> > + * server doesn't implement it, an @c SVN_ERR_RA_NOT_IMPLEMENTED error is
> > + * returned.
> > + */
> > +svn_error_t *svn_ra_get_locations (svn_ra_session_t *session,
> > + apr_hash_t **locations,
> > + const char *path,
> > + svn_revnum_t peg_revision,
> > + apr_array_header_t *location_revisions,
> > + apr_pool_t *pool);
> > +
>
> Wow, that's a heck of a first sentence! I had to read it three times.
> I'd like to reword it, maybe like this:
>
> * Set @a *locations to the locations (at the repository revisions
> * @a location_revisions) of the file identified by @a path in @a
> * peg_revision.
>
Feel free to while your at it:-)
> > +/**
> > + * @since New in 1.2.
> > *
> > - * A vtable structure which encapsulates all the functionality of a
> > - * particular repository-access implementation.
> > + * Retrieve a subset of the interesting revisions of a file @a path
> > + * as seen in revision @a end. Invoke @a handler with @a handler_baton
> > + * as its first argument for each such revision. @a sesson is
Ha! You missed a typo:-) In the last line.
> > +/** Return a @a *descriptions string (allocated in @a pool) that is a textual
> > + * list of all available RA libraries.
> > + *
> > + * @a unused is provided for backwards compatibility with the 1.1 API and is
> > + * ignored.
> > + */
> > +svn_error_t *svn_ra_print_ra_libraries (svn_stringbuf_t **descriptions,
> > + void *unused,
> > + apr_pool_t *pool);
>
> Huh. Is there some reason we didn't do the standard deprecation
> process here, to make a new API without the "unused" parameter?
>
Just that I didn't think we deprecate things in cases like this when there
is no need to. But that might be cleaner. I'll fix.
> > -
> > +/* Ensure that the RA library NAME is loaded.
> > + * If FUNC is non-NULL, set *FUNC to the address of the svn_ra_NAME__init
> > + * function of the library.
> > + * If COMPAT_FUNC is non-NULL, set *COMPAT_FUNC to the address of the
> > + * svn_ra_NAME_init compatibility init function of the library. */
> > static svn_error_t *
> > -load_ra_module (svn_ra_init_func_t *func,
> > +load_ra_module (svn_ra__init_func_t *func,
> > + svn_ra_init_func_t *compat_func,
> > const char *ra_name, apr_pool_t *pool)
>
> Future RA implementations will not have today's compatibility
> requirements. Therefore, perhaps it would be wise to say ", if any."
> after the promise to set *COMPAT_FUNC, and document that if there is
> no such function available, it sets *COMPAT_FUNC to NULL?
>
I'm not sure here. The old documentation states that any RA implementation
will have to implement svn_ra_foo_init. But maybe that doesn't apply to
new implementations. In that case, I'll change this to loosen that
requirement.
> > @@ -115,184 +137,380 @@
> > }
> > /* note: the library will be unloaded at pool cleanup */
> >
> > - /* find the initialization routine */
> > - status = apr_dso_sym (&symbol, dso, funcname);
> > - if (status)
> > + /* find the initialization routines */
> > + if (func)
> > {
> > - return svn_error_wrap_apr (status, _("'%s' does not define '%s()'"),
> > - libname, funcname);
> > + status = apr_dso_sym (&symbol, dso, funcname);
> > + if (status)
> > + {
> > + return svn_error_wrap_apr (status,
> > + _("'%s' does not define '%s()'"),
> > + libname, funcname);
> > + }
> > +
> > + *func = (svn_ra__init_func_t) symbol;
> > }
> >
> > - *func = (svn_ra_init_func_t) symbol;
> > + if (compat_func)
> > + {
> > + status = apr_dso_sym (&symbol, dso, compat_funcname);
> > + if (status)
> > + {
> > + return svn_error_wrap_apr (status,
> > + _("'%s' does not define '%s()'"),
> > + libname, compat_funcname);
> > + }
> > +
> > + *compat_func = (svn_ra_init_func_t) symbol;
> > + }
> > }
> > #endif /* APR_HAS_DSO */
> >
> > return SVN_NO_ERROR;
> > }
> >
> > -
> > -/* -------------------------------------------------------------- */
> > -
> > -/*** Public Interfaces ***/
> > -
> > -svn_error_t *
> > -svn_ra_init_ra_libs (void **ra_baton,
> > - apr_pool_t *pool)
> > +/* If DEFN may support URL, return the scheme. Else, return NULL. */
> > +static const char *
> > +has_scheme_of (const struct ra_lib_defn *defn, const char *url)
> > {
> > - const struct ra_lib_defn *defn;
> > - apr_hash_t *ra_library_hash;
> > -
> > - /* Our baton is a hash table that maps repository URL schemes to the
> > - ra_plugin vtable that will handle it. */
> > - ra_library_hash = apr_hash_make (pool);
> > + const char * const *schemes;
> > + apr_size_t len;
> >
> > - for (defn = ra_libraries; defn->ra_name != NULL; ++defn)
> > + for (schemes = defn->schemes; *schemes != NULL; ++schemes)
> > {
> > - svn_ra_init_func_t initfunc = defn->initfunc;
> > -
> > - if (initfunc == NULL)
> > - {
> > - /* see if we can find a dynload module */
> > - SVN_ERR( load_ra_module (&initfunc, defn->ra_name, pool) );
> > - }
> > -
> > - if (initfunc != NULL)
> > - {
> > - /* linked in or successfully dynloaded */
> > -
> > - SVN_ERR( (*initfunc)(SVN_RA_ABI_VERSION, pool, ra_library_hash) );
> > -
> > - }
> > + len = strlen (*schemes);
> > + /* Case-insensitive comparison, per RFC 2396 section 3.1. Allow
> > + URL to contain a trailing "+foo" section in the scheme, since
> > + that's how we specify tunnel schemes in ra_svn. */
> > + if (strncasecmp (*schemes, url, len) == 0 &&
> > + (url[len] == ':' || url[len] == '+'))
> > + return *schemes;
> > }
>
> I'd say declare 'const char *scheme = *schemes;' at the top of this
> for-loop, and use it in place of '*schemes', for readability.
>
God point. I'll change.
> Now, I was *going* to claim that we should use apr_strncasecmp() not
> strncasecmp(). The latter is not in ISO C -- it made it into POSIX in
> 2001, I believe, but that's still not portable enough for Subversion.
> However, I was wrong. It turns out that strncasecmp() is provided in
> apr_general.h. So, it's okay here, and in libsvn_ra_svn/client.c
> where we also call it :-).
>
Nice to know. But we have to be careful with it in general, since it is
locale dependent.
> > + if (! initfunc)
> > + SVN_ERR (load_ra_module (&initfunc, NULL, defn->ra_name,
> > + pool));
> > + if (! initfunc)
> > + /* Library not found. */
> > + break;
> > +
> > + SVN_ERR (initfunc (my_version, &vtable));
> > +
> > + ra_version = vtable->get_version();
> > + if (!svn_ver_equal (my_version, ra_version))
> > return svn_error_createf (SVN_ERR_VERSION_MISMATCH, NULL,
> > - _("Mismatched RA plugin version"
> > + _("Mismatched RA version"
> > " for '%s':"
> > " found %d.%d.%d%s,"
> > " expected %d.%d.%d%s"),
> > - keystr,
> > + scheme,
> > my_version->major, my_version->minor,
> > my_version->patch, my_version->tag,
> > ra_version->major, ra_version->minor,
> > ra_version->patch, ra_version->tag);
> > - return SVN_NO_ERROR;
> > }
> > }
>
> Some thoughts:
>
> This version-checking code could be implemented inside 'initfunc'.
> And if it's not implemented in initfunc, then why are we passing
> 'my_version' to initfunc? :-) Since ra_version comes from vtable, and
> vtable comes from the call to initfunc, and we're passing my_version
> to initfunc anyway... You can see where I'm going with this.
>
I actually stole this strategy from libsvn_fs. The point is, if i'm not
mistaken, to keep the strict compatibility requirements in one place and
let the RA implementations just check the ABI, i.e. that it can use the
vtable argument and that the caller expects a get_version function first
in the returned struct.
> Another thought: it would also be nice if this version-checking
code > were abstracted out, since the code is virtually duplicated in
> svn_ra_get_ra_library() (see below).
>
Good point.
> > --- (empty file)
> > +++ trunk/subversion/libsvn_ra/ra_loader.h Thu Jan 20 15:05:59 2005
> > @@ -0,0 +1,218 @@
> > +/**
> > + * @copyright
> > + * ====================================================================
> > + * Copyright (c) 2000-2005 CollabNet. All rights reserved.
> > + *
> > + * This software is licensed as described in the file COPYING, which
> > + * you should have received as part of this distribution. The terms
> > + * are also available at http://subversion.tigris.org/license-1.html.
> > + * If newer versions of this license are posted there, you may use a
> > + * newer version instead, at your option.
> > + *
> > + * This software consists of voluntary contributions made by many
> > + * individuals. For exact contribution history, see the revision
> > + * history and logs, available at http://subversion.tigris.org/.
> > + * ====================================================================
> > + * @endcopyright
>
> Not "2000-2005", just "2005". This file didn't exist before 2005.
>
Will our semi-automatic copyright string updating handle that? It needs to
be 2005-2006 someday.
> > + /* All fields in SESSION, except priv, are valid. SESSION->priv
> > + * may be set by this function. */
> > + svn_error_t *(*open) (svn_ra_session_t *session,
> > + const char *repos_URL,
> > + const svn_ra_callbacks_t *callbacks,
> > + void *callback_baton,
> > + apr_hash_t *config,
> > + apr_pool_t *pool);
>
> What does that doc string mean, exactly? The word "valid" is a bit
> vague; we should describe the semantics more precisely.
>
Valid means initialized here. The intent is that the only field that
should be set by this function is session->priv. Everything else will be
taken care of by the caller.
> > +/* Each libsvn_ra_foo defines a function named svn_ra_foo__init of this type.
> > + *
> > + * The LOADER_VERSION parameter must remain first in the list, and the
> > + * function must use the C calling convention on all platforms, so that
> > + * the init functions can safely read the version parameter.
> > + *
> > + * ### need to force this to be __cdecl on Windows... how??
> > + */
>
> I don't know either. (Wasn't that helpful?)
>
Thank you. It was actually duplicated from somewhere else...
> > +/* Macro to create a compatibility wrapper for a RA library.
> > + * It creates an svn_ra_plugin_t named compat_plugin that implements the plugin
> > + * API in terms of the svn_ra__vtable_t VTBL. NAME and DESCRIPTION
> > + * are the two first fields of the plugin struct.
> > + * NOTE: This has to be a macro, so that each RA library can duplicate this.
> > + * An RA library can't use symbols from libsvn_ra, since that would introduce
> > + * circular dependencies. */
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +
> > +#endif
>
> Heh. The above comment ends the file -- it describes a macro, but
> there's no macro actually there :-).
>
There *was* a macro:-) You don't want to know... The whole compat wrapper
was first implemented as a 200 lines long macro. maxb suggested to change
it to a file with some magic defines. > > --- (empty file)
> > +++ trunk/subversion/libsvn_ra/wrapper_template.h Thu Jan 20 15:05:59 2005
> > @@ -0,0 +1,319 @@
> > +/**
> > + * @copyright
> > + * ====================================================================
> > + * Copyright (c) 2000-2005 CollabNet. All rights reserved.
> > + *
> > + * This software is licensed as described in the file COPYING, which
> > + * you should have received as part of this distribution. The terms
> > + * are also available at http://subversion.tigris.org/license-1.html.
> > + * If newer versions of this license are posted there, you may use a
> > + * newer version instead, at your option.
> > + *
> > + * This software consists of voluntary contributions made by many
> > + * individuals. For exact contribution history, see the revision
> > + * history and logs, available at http://subversion.tigris.org/.
> > + * ====================================================================
> > + * @endcopyright
>
> Same issue with the copyright year.
>
Same question.
> Also, this file should be called "wrapper_template.c", IMHO. It
> contains C source code. It's fine for other files to write
>
> #include "wrapper_template.c"
>
> ... there's nothing about #include that demands .h files instead of .c
> files.
>
I am not really sure about this. It contains function definitions, but it
is still meant for inclusion. So neither .h nor .c is appropriate. We use
.h for svn_error_coes, which actually contains a variable definition if
included in a particular way. Uh, and wouldn't the build system try to
compile it if it was named .c?
> > + * NAME The library name, i.e. "ra_local".
> > + * DESCRIPTION The short library description as a string constant.
> > + * VTBL The name of an svn_ra_vtable_t object for the library.
> > + * INITFUNC The init function for the library, i.e. svn_ra_local__init.
> > + * COMPAT_INITFUNC The compatibility init function, i.e. svn_ra_local_init.
> > + */
>
> All instances of "i.e." above should be "e.g.".
>
OH, well, I never understood the difference.
> Is there a reason we don't have to namespace-protect these macros?
> Shouldn't they be named
>
> SVN_RA__COMPAT_NAME
> SVN_RA__COMPAT_DESCRIPTION
> SVN_RA__COMPAT_VTABLE
> SVN_RA__COMPAT_INITFUNC
> SVN_RA__COMPAT_COMPAT_INITFUNC
>
> or something like that? It's true they will only be defined in the
> calling file and in wrapper_template.(h->c), but those files might one
> day import other symbols with which these interfere.
>
I really se this file as a part of the RA implementations. So, the same
problem applies to static symbols in those files. If we think this is a
problem, we'll have to consider fs-loader as well. It also declares
unprotected symbols in this way.
...and ofcoruse, the real reason is that VTBL had to written many times:-)
...
> > + compat_get_repos_root,
> > + compat_get_locations,
> > + compat_get_file_revs,
> > + compat_get_version,
> > +};
>
> Standard C syntax doesn't want a comma after that last field, although
> I guess some modern compilers are forgiving about this.
>
Common mistake, though valid C99 in fact. But we aren't C99, so that's
irrelevant.
> > +svn_error_t *
> > +COMPAT_INITFUNC (int abi_version,
> > + apr_pool_t *pool,
> > + apr_hash_t *hash)
> > +{
> > + const svn_ra__vtable_t *vtable;
> > + const char * const * schemes;
> > +
> > + if (abi_version < 1
> > + || abi_version > SVN_RA_ABI_VERSION)
> > + return svn_error_createf (SVN_ERR_RA_UNSUPPORTED_ABI_VERSION, NULL,
> > + _("Unsupported RA plugin ABI version (%d) "
> > + "for %s"), abi_version, NAME);
> > +
> > + /* We call the new init function so it can check library dependencies or
> > + do other initialization things. We fake the loader version, since we
> > + rely on the ABI version check instead. */
> > + SVN_ERR (INITFUNC (VTBL.get_version(), &vtable));
> > +
> > + /* Sanity check. */
> > + assert (&VTBL == vtable);
>
> Should we really require this? It would be possible to implement a
> working, bug-free RA compat layer without fulfilling the condition of
> this assert().
>
OK.
> > + schemes = VTBL.get_schemes (pool);
> > +
> > + for (; *schemes != NULL; ++schemes)
> > + apr_hash_set (hash, *schemes, APR_HASH_KEY_STRING, &compat_plugin);
> > +
> > + return SVN_NO_ERROR;
> > +}
>
> Hmmm. I'm tempted to ask for the 'scheme' variable here too, but I
> see how that would make the code a bit bigger. Well, one could write
> this:
>
> for (scheme = *schemes; scheme != NULL; ++schemes, scheme = *schemes)
> apr_hash_set (hash, scheme, APR_HASH_KEY_STRING, &compat_plugin);
>
> But I'm not sure that's really clearer! So forget I mentioned it :-).
>
Already done:-) (I mean forgotten.)
> > --- trunk/subversion/libsvn_ra_dav/fetch.c (original)
> > +++ trunk/subversion/libsvn_ra_dav/fetch.c Thu Jan 20 15:05:59 2005
> > @@ -1223,9 +1224,9 @@
> > START and END revisions. */
> > SVN_ERR( svn_ra_dav__get_baseline_info(NULL, &bc_url, &bc_relative, NULL,
> > ras->sess, ras->url, peg_revision,
> > - ras->pool) );
> > + session->pool) );
> > final_bc_url = svn_path_url_add_component(bc_url.data, bc_relative.data,
> > - ras->pool);
> > + session->pool);
> >
> > err = svn_ra_dav__parsed_request(ras->sess, "REPORT", final_bc_url,
> > request_body->data, NULL, NULL,
>
> Using session->pool instead of ras->pool is a semantic change, no?
> The old ras->pool meant the pool from session_baton. Isn't
> session->priv (i.e., ras) the new equivalent of the former
> session_baton parameter? If so, ras->pool should still be correct.
> And the log message didn't mention anything about a semantic change in
> the pool usage here.
>
> Quite likely I'm just misunderstanding something, though. Let me know.
>
They are the same pool actually. I added the session pool to
svn_ra_session_t, but had to keep it in the DAV session baton for some
functions.
> > --- trunk/subversion/libsvn_ra_dav/log.c (original)
> > +++ trunk/subversion/libsvn_ra_dav/log.c Thu Jan 20 15:05:59 2005
> > @@ -32,6 +32,7 @@
> > #include "svn_pools.h"
> > #include "svn_path.h"
> > #include "svn_xml.h"
> > +#include "../libsvn_ra/ra_loader.h"
> >
> > #include "ra_dav.h"
> >
> > @@ -292,16 +293,16 @@
> > }
> >
> >
> > -svn_error_t * svn_ra_dav__get_log2(void *session_baton,
> > - const apr_array_header_t *paths,
> > - svn_revnum_t start,
> > - svn_revnum_t end,
> > - int limit,
> > - svn_boolean_t discover_changed_paths,
> > - svn_boolean_t strict_node_history,
> > - svn_log_message_receiver_t receiver,
> > - void *receiver_baton,
> > - apr_pool_t *pool)
> > +svn_error_t * svn_ra_dav__get_log(svn_ra_session_t *session,
> > + const apr_array_header_t *paths,
> > + svn_revnum_t start,
> > + svn_revnum_t end,
> > + int limit,
> > + svn_boolean_t discover_changed_paths,
> > + svn_boolean_t strict_node_history,
> > + svn_log_message_receiver_t receiver,
> > + void *receiver_baton,
> > + apr_pool_t *pool)
>
> Heh. Good point -- there was never any need for a "2" suffix on this
> non-public function. It looks like r11155 was the culprit here.
>
There was a reason, since there had to be a wrapper and it seems the most
natural thing to call it. But now the RA layers doesn't have to provide
their own compat wrappers. Actually, that was the main motivation for this
whole thing:-)
> > --- trunk/subversion/libsvn_ra_dav/ra_dav.h (original)
> > +++ trunk/subversion/libsvn_ra_dav/ra_dav.h Thu Jan 20 15:05:59 2005
> > @@ -118,17 +118,16 @@
> >
> > typedef struct {
> > apr_pool_t *pool;
> > -
> > const char *url; /* original, unparsed session url */
> > ne_uri root; /* parsed version of above */
> > const char *repos_root; /* URL for repository root */
> >
> > ne_session *sess; /* HTTP session to server */
> > ne_session *sess2;
> > -
> > +
> > const svn_ra_callbacks_t *callbacks; /* callbacks to get auth data */
> > void *callback_baton;
> > -
> > +
> > svn_auth_iterstate_t *auth_iterstate; /* state of authentication retries */
> >
> > svn_boolean_t compression; /* should we use http compression? */
>
> These are all whitespace changes on blank links, not really needed in
> this commit.
>
> Also, your log message didn't mention this file at all. (I'm sure you
> just forgot -- all the other changes in the file were easily
> deriveable from the rest of the commit, so you must have made the
> adjustments almost unconsciously).
>
Ah, these whitespace changes were unnoticed. I blame Emacs, but that's
actually no good excuse. As you say, all other changes to this file were
covered by some "all RA methods changed to the new API" in the message.
> > --- trunk/subversion/libsvn_ra_dav/session.c (original)
> > +++ trunk/subversion/libsvn_ra_dav/session.c Thu Jan 20 15:05:59 2005
> > @@ -32,6 +32,7 @@
> >
> > #include "svn_error.h"
> > #include "svn_ra.h"
> > +#include "../libsvn_ra/ra_loader.h"
> > #include "svn_config.h"
> > #include "svn_delta.h"
> > #include "svn_version.h"
> > @@ -526,13 +527,32 @@
> > return 0;
> > }
> >
> > +#define RA_DAV_DESCRIPTION \
> > + N_("Module for accessing a repository via WebDAV (DeltaV) protocol.")
> > +
> > +static const char *
> > +ra_dav_get_description(void)
> > +{
> > + return _(RA_DAV_DESCRIPTION);
> > +}
> > +
> > +static const char * const *
> > +ra_dav_get_schemes (apr_pool_t *pool)
> > +{
> > + static const char *schemes_no_ssl[] = { "http", NULL };
> > + static const char *schemes_ssl[] = { "http", "https", NULL };
> > +
> > + return ne_supports_ssl() ? schemes_ssl : schemes_no_ssl;
> > +}
> > +
> > +
> >
> > /* ### need an ne_session_dup to avoid the second gethostbyname
> > * call and make this halfway sane. */
> >
> >
> > static svn_error_t *
> > -svn_ra_dav__open (void **session_baton,
> > +svn_ra_dav__open (svn_ra_session_t *session,
> > const char *repos_URL,
> > const svn_ra_callbacks_t *callbacks,
> > void *callback_baton,
> > @@ -753,17 +773,17 @@
> > }
> > }
> >
> > - *session_baton = ras;
> > + session->priv = ras;
> >
> > return SVN_NO_ERROR;
> > }
> >
>
> I've always wondered why these static functions have "svn_ra_dav__"
> prefixes on their names. I'm only quoting one above, but there are
> others in the file, of course.
>
I'm also wondering. I didn't take care of *that* in this commit. Just
didn't feel it was very important...
> > @@ -852,15 +873,18 @@
> > svn_ra_dav__do_get_uuid,
> > svn_ra_dav__get_repos_root,
> > svn_ra_dav__get_locations,
> > - svn_ra_dav__get_file_revs,
> > - ra_dav_version,
> > - svn_ra_dav__get_log2
> > + svn_ra_dav__get_file_revs
> > };
> >
> > +/* Check the versions of our library dependencies. */
> > +static svn_error_t *
> > +ver_check(void)
> > +{
> > +}
>
> Missing a "return SVN_NO_ERROR;" line here?
>
This was a left-over from an old out-factoring. Removed in r12803 by
philip.
> > @@ -869,20 +893,25 @@
> > { NULL, NULL }
> > };
> >
> > - if (abi_version < 1
> > - || abi_version > SVN_RA_ABI_VERSION)
> > - return svn_error_createf (SVN_ERR_RA_UNSUPPORTED_ABI_VERSION, NULL,
> > - _("Unsupported RA plugin ABI version (%d) "
> > - "for ra_dav"), abi_version);
> > SVN_ERR(svn_ver_check_list(ra_dav_version(), checklist));
> >
> > - apr_hash_set (hash, "http", APR_HASH_KEY_STRING, &dav_plugin);
> > + /* Simplified version check to make sure we can safely use the
> > + VTABLE parameter. The RA loader does a more exhaustive check. */
> > + if (loader_version->major != SVN_VER_MAJOR)
> > + return svn_error_createf (SVN_ERR_VERSION_MISMATCH, NULL,
> > + _("Unsupported RA loader version (%d) for "
> > + "ra_dav"),
> > + loader_version->major);
>
> Just put the first line break right after "svn_error_createf", that
> way you can shift things around so there's no need to divide the
> constant string across two lines.
>
Good point.
> > --- trunk/subversion/libsvn_ra_local/ra_local.h (original)
> > +++ trunk/subversion/libsvn_ra_local/ra_local.h Thu Jan 20 15:05:59 2005
> > @@ -37,10 +37,6 @@
> > /* A baton which represents a single ra_local session. */
> > typedef struct svn_ra_local__session_baton_t
> > {
> > - /* Each ra_local session does ALL allocation from this pool! Kind
> > - of like an Apache transaction, I guess. :) */
> > - apr_pool_t *pool;
> > -
>
> Nice to see *that* go away! :-)
>
> > --- trunk/subversion/libsvn_ra_local/ra_plugin.c (original)
> > +++ trunk/subversion/libsvn_ra_local/ra_plugin.c Thu Jan 20 15:05:59 2005
> > @@ -199,61 +201,74 @@
> >
> > /*----------------------------------------------------------------*/
> >
> > -/** The RA plugin routines **/
> > +/** The RA vtable routines **/
> >
> > +#define RA_LOCAL_DESCRIPTION \
> > + N_("Module for accessing a repository on local disk.")
> > +
> > +static const char *
> > +svn_ra_local__get_description (void)
> > +{
> > + static const char *desc = RA_LOCAL_DESCRIPTION;
> > + return _(desc);
> > +}
>
> Why bother with the intermediate 'desc' variable? Can't we just use
> the RA_LOCAL_DESCRIPTION directly? Or is there some subtle
> interaction with "_" here? (I don't really understand the use of "_"
> above, it would seem to me that the code as it stands wouldn't get
> localized anyway...)
>
That var is bogus. But the string gets marked for translation by the N_ in
the constant definition. _() is to actually call dgettext to get the
translation. Note that there is a small difference in the old and the new
API here. The old API has the description as a string constant in the
plugin struct, meaning that the "caller" has to translate it. The new API
has a function that actually returns a translated string. So a new RA
implementation could have its translation in some other .po file if it
wants to.
> > static svn_error_t *
> > -svn_ra_local__open (void **session_baton,
> > +svn_ra_local__open (svn_ra_session_t *session,
> > const char *repos_URL,
> > const svn_ra_callbacks_t *callbacks,
> > void *callback_baton,
> > apr_hash_t *config,
> > apr_pool_t *pool)
> > {
>
> Same point about static functions have thing "svn_ra_foo__" prefix.
> Not introduced by your change, I realize.
>
> > - svn_ra_local__session_baton_t *session;
> > + svn_ra_local__session_baton_t *baton;
> >
> > /* Allocate and stash the session_baton args we have already. */
> > - session = apr_pcalloc (pool, sizeof(*session));
> > - session->pool = pool;
> > - session->repository_URL = repos_URL;
> > + baton = apr_pcalloc (pool, sizeof(*baton));
> > + baton->repository_URL = repos_URL;
> > + baton->callbacks = callbacks;
> > + baton->callback_baton = callback_baton;
>
> Hmmm. Again, this wasn't introduced by your code, but I'm surprised
> we're not reallocating baton->repository_URL in pool. The interface
> documentation doesn't say anything about the 'repos_URL' parameter's
> lifetime, so shouldn't we copy it into the session's memory just to be
> safe?
>
The docstring actually says that it lives as long as the session. If we
want to change the API, we want to change the implementation here.
> > @@ -406,7 +415,7 @@
> > void *callback_baton,
> > apr_pool_t *pool)
> > {
> > - svn_ra_local__session_baton_t *sess = session_baton;
> > + svn_ra_local__session_baton_t *sess = session->priv;
> > struct deltify_etc_baton *db = apr_palloc (pool, sizeof(*db));
> >
> > db->fs = sess->fs;
>
> Might be clearer to call it 'baton' or something, instead of 'sess'.
> Having a parameter named "session" coexiting with a variable named
> "sess" could be a bit... confusing :-).
>
Ofcourse, you're right.
> > @@ -610,22 +619,21 @@
> > }
> >
> >
> > -/** @since New in 1.2. */
> > static svn_error_t *
> > -svn_ra_local__get_log2 (void *session_baton,
> > - const apr_array_header_t *paths,
> > - svn_revnum_t start,
> > - svn_revnum_t end,
> > - int limit,
> > - svn_boolean_t discover_changed_paths,
> > - svn_boolean_t strict_node_history,
> > - svn_log_message_receiver_t receiver,
> > - void *receiver_baton,
> > - apr_pool_t *pool)
> > +svn_ra_local__get_log (svn_ra_session_t *session,
> > + const apr_array_header_t *paths,
> > + svn_revnum_t start,
> > + svn_revnum_t end,
> > + int limit,
> > + svn_boolean_t discover_changed_paths,
> > + svn_boolean_t strict_node_history,
> > + svn_log_message_receiver_t receiver,
> > + void *receiver_baton,
> > + apr_pool_t *pool)
> > {
> > - svn_ra_local__session_baton_t *sbaton = session_baton;
> > + svn_ra_local__session_baton_t *sbaton = session->priv;
> > apr_array_header_t *abs_paths
> > - = apr_array_make (sbaton->pool, paths->nelts, sizeof (const char *));
> > + = apr_array_make (session->pool, paths->nelts, sizeof (const char *));
> > int i;
>
> Shouldn't we use pool instead of session->pool, for abs_paths?
>
> > for (i = 0; i < paths->nelts; i++)
> > @@ -635,7 +643,7 @@
> >
> > /* Append the relative paths to the base FS path to get an
> > absolute repository path. */
> > - abs_path = svn_path_join (sbaton->fs_path, relative_path, sbaton->pool);
> > + abs_path = svn_path_join (sbaton->fs_path, relative_path, session->pool);
> > (*((const char **)(apr_array_push (abs_paths)))) = abs_path;
> > }
>
> And shouldn't we use 'pool' for this svn_path_join() call?
>
> > @@ -649,42 +657,18 @@
> > NULL, NULL,
> > receiver,
> > receiver_baton,
> > - sbaton->pool);
> > + session->pool);
> > }
>
> And shouldn't we pass it as the last parameter of svn_repos_get_logs3
> here?
>
> In other words, the 'pool' parameter of svn_ra_local__get_log() is
> completely ignored, currently. Somehow that doesn't seem right.
>
Correct. I noticed that, didn't want to mess with that in *this* commit
and then forgot. I wonder which is worst, putting unrelated changes in
commits or forgetting bugs. This is a leak.
> > @@ -999,14 +981,26 @@
> > { NULL, NULL }
> > };
> >
> > - if (abi_version < 1
> > - || abi_version > SVN_RA_ABI_VERSION)
> > - return svn_error_createf (SVN_ERR_RA_UNSUPPORTED_ABI_VERSION, NULL,
> > - _("Unsupported RA plugin ABI version (%d) "
> > - "for ra_local"), abi_version);
> > +
> > + /* Simplified version check to make sure we can safely use the
> > + VTABLE parameter. The RA loader does a more exhaustive check. */
> > + if (loader_version->major != SVN_VER_MAJOR)
> > + return svn_error_createf (SVN_ERR_VERSION_MISMATCH, NULL,
> > + _("Unsupported RA loader version (%d) for "
> > + "ra_local"),
> > + loader_version->major);
> > +
> > SVN_ERR (svn_ver_check_list (ra_local_version(), checklist));
>
> Same thing as before about the line breaking. Also, since this code
> is so similar, I wonder if it could be abstracted for all the RA
> layers.
>
Question is only where to put it. A macro is possible but ugly. libsvn_ra
isn't a solution since that would be a circular dependency. libsvn_subr
seems odd.
> > --- trunk/subversion/libsvn_ra_svn/client.c (original)
> > +++ trunk/subversion/libsvn_ra_svn/client.c Thu Jan 20 15:05:59 2005
> > @@ -556,7 +558,25 @@
> > return SVN_NO_ERROR;
> > }
> >
> > -static svn_error_t *ra_svn_open(void **baton, const char *url,
> > +#define RA_SVN_DESCRIPTION \
> > + N_("Module for accessing a repository using the svn network protocol.")
> > +
> > +static const char *ra_svn_get_description(void)
> > +{
> > + return _(RA_SVN_DESCRIPTION);
> > +}
> > +
> > +static const char * const *
> > +ra_svn_get_schemes (apr_pool_t *pool)
> > +{
> > + static const char *schemes[] = { "svn", NULL };
> > +
> > + return schemes;
> > +}
>
> Sanity check: "svn+ssh://" still parses out to "svn" as the scheme,
> right?
>
Yes. See has_scheme_of in ra_loader.c.
> > @@ -645,14 +665,14 @@
> > "server"));
> > }
> >
> > - *baton = sess;
> > + session->priv = sess;
> > return SVN_NO_ERROR;
> > }
> >
> > -static svn_error_t *ra_svn_get_latest_rev(void *baton, svn_revnum_t *rev,
> > - apr_pool_t *pool)
> > +static svn_error_t *ra_svn_get_latest_rev(svn_ra_session_t *session,
> > + svn_revnum_t *rev, apr_pool_t *pool)
> > {
> > - ra_svn_session_baton_t *sess = baton;
> > + ra_svn_session_baton_t *sess = session->priv;
> > svn_ra_svn_conn_t *conn = sess->conn;
>
> Not introduced by you, but again, do we really want to call it "sess",
> given that there's a parameter named "session"? (I won't bother with
> the other instances, just take them as mentioned.)
>
Good point.
> > @@ -1010,15 +1031,15 @@
> > }
> >
> > /** @since New in 1.2. */
> > -static svn_error_t *ra_svn_log2(void *baton, const apr_array_header_t *paths,
> > - svn_revnum_t start, svn_revnum_t end,
> > - int limit,
> > - svn_boolean_t discover_changed_paths,
> > - svn_boolean_t strict_node_history,
> > - svn_log_message_receiver_t receiver,
> > - void *receiver_baton, apr_pool_t *pool)
> > +static svn_error_t *ra_svn_log(svn_ra_session_t *session, const apr_array_header_t *paths,
> > + svn_revnum_t start, svn_revnum_t end,
> > + int limit,
> > + svn_boolean_t discover_changed_paths,
> > + svn_boolean_t strict_node_history,
> > + svn_log_message_receiver_t receiver,
> > + void *receiver_baton, apr_pool_t *pool)
>
> Shouldn't the "@since New in 1.2" line go away also?
>
Yes.
The old plugin is created by the wrapper as compat_plugin and returned by
the generated svn_fo_init function.
OK, I'll wait for you to take care of what you think is trivialia. Then
I'll take care of the rest.
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Feb 5 00:07:24 2005