Hey, Peter. I know I said I was going to send a lot of small mails,
instead of one big mail, to review this commit, but it turns out to be
easier to do it in one big mail. Uh, easier for me anyway :-).
Quick summary: Nice job! It was a *very* thorough change, many little
details were gotten right. I saw no major problems.
Furthermore, of the issues I did see, the majority were in the code
already, and I just happened to notice them while reviewing your
change. So please take this review as a set of questions about our
code, not so much about your commit per se.
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.
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.
> --- trunk/subversion/include/svn_ra.h (original)
> +++ trunk/subversion/include/svn_ra.h Thu Jan 20 15:05:59 2005
> @@ -277,16 +277,551 @@
>
> [...]
>
> +/**
> + * @since New in 1.2.
> + *
> + * Set the property @a name to @a value on revision @a rev in the repository
> + * of @a session.
> + *
> + * If @a value is @c NULL, delete the named revision property.
> + *
> + * Please note that properties attached to revisions are **unversioned**.
> + *
> + * Use @a pool for memory allocation.
> + */
Huh. There's a Doxygen way to get emphasis, right? Is it two
asterisks, like above, or is it something else?
> +/**
> + * @since New in 1.2.
> + *
> + * Set @a *props to the list of unversioned properties attached to revision
> + * @a rev in the repository of @a session. The hash maps
> + * (<tt>const char *</tt>) names to (<tt>@c svn_string_t *</tt>) values.
> + *
> + * Use @a pool for memory allocation.
> + */
> +svn_error_t *svn_ra_rev_proplist (svn_ra_session_t *session,
> + svn_revnum_t rev,
> + apr_hash_t **props,
> + apr_pool_t *pool);
I think it's nice to say "Allocate @a *props in @a pool." in
situations like this. We do that in a lot of places, but aren't
entirely consistent about it.
> +/**
> + * @since New in 1.2.
> + *
> + * Set @a *value to the value of unversioned property @a name attached to
> + * revision @a rev in the repository of @a session. If @a rev has no
> + * property by that name, set @a *value to @c NULL.
> + *
> + * Use @a pool for memory allocation.
> + */
> +svn_error_t *svn_ra_rev_prop (svn_ra_session_t *session,
> + svn_revnum_t rev,
> + const char *name,
> + svn_string_t **value,
> + apr_pool_t *pool);
Ditto for "*value".
> +/**
> + * @since New in 1.2.
> + *
> + * Set @a *editor and @a *edit_baton to an editor for committing changes
> + * to the repository of @a session, using @a log_msg as the log message. The
> + * revisions being committed against are passed to the editor
> + * functions, starting with the rev argument to @c open_root. The path
> + * root of the commit is in the @a session's URL.
> + *
> + * Before @c close_edit returns, but after the commit has succeeded,
> + * it will invoke @a callback with the new revision number, the
> + * commit date (as a <tt>const char *</tt>), commit author (as a
> + * <tt>const char *</tt>), and @a callback_baton as arguments. If
> + * @a callback returns an error, that error will be returned from @c
> + * close_edit, otherwise @c close_edit will return successfully
> + * (unless it encountered an error before invoking @a callback).
> + *
> + * The callback will not be called if the commit was a no-op
> + * (i.e. nothing was committed);
> + *
> + * The caller may not perform any RA operations using @a session before
> + * finishing the edit.
> + *
> + * Use @a pool for memory allocation.
> + */
> +svn_error_t *svn_ra_get_commit_editor (svn_ra_session_t *session,
> + const svn_delta_editor_t **editor,
> + void **edit_baton,
> + const char *log_msg,
> + svn_commit_callback_t callback,
> + void *callback_baton,
> + apr_pool_t *pool);
Ditto for "*editor" and "*edit_baton".
> +/**
> + * @since New in 1.2.
> + *
> + * Fetch the contents and properties of file @a path at @a revision.
> + * Interpret @a path relative to the URL in @a session. Use
> + * @a pool for all allocations.
> + *
> + * If @a revision is @c SVN_INVALID_REVNUM (meaning 'head') and
> + * @a *fetched_rev is not @c NULL, then this function will set
> + * @a *fetched_rev to the actual revision that was retrieved. (Some
> + * callers want to know, and some don't.)
> + *
> + * If @a stream is non @c NULL, push the contents of the file at @a stream.
> + *
> + * If @a props is non @c NULL, set @a *props to contain the properties of
> + * the file. This means *all* properties: not just ones controlled by
> + * the user and stored in the repository fs, but non-tweakable ones
> + * generated by the SCM system itself (e.g. 'wcprops', 'entryprops',
> + * etc.) The keys are <tt>const char *</tt>, values are
> + * <tt>@c svn_string_t *</tt>.
Yeah, so here we're using one asterisk per side, instead of two, for
emphasis (see the word "*all*"). We should figure out the Doxygen-ish
way and use that.
> +/**
> + * @since New in 1.2.
> + *
> + * Ask the RA layer to update a working copy.
> + *
> + * The client initially provides an @a update_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 update_editor, ending with
> + * @c close_edit(), to update the working copy.
> + *
> + * @a update_target is an optional single path component will restrict
> + * the scope of things affected by the update to an entry in the
> + * directory represented by the @a session's URL, or empty if the
> + * entire directory is meant to be updated.
Whups, grammar problem, need "that" before "will", or else a rewording
to make it clearer overall.
> + * The working copy will be updated to @a revision_to_update_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 from within the editing operations of @a update_editor.
> + *
> + * Use @a pool for memory allocation.
> + */
> +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.
> +/**
> + * @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 :-).
> +/**
> + * @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.
*/
> +/**
> + * @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.
> +/**
> + * @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
> + * an open RA session. @a pool is used for all allocations. See
> + * @c svn_fs_history_prev for a discussion of interesting revisions.
We should use the active voice: "Use @a pool for all allocations.",
not "@a pool is used...".
I think the clarification of the definition of "interesting" comes a
bit late. The reader is already confused by the end of the paragraph.
The fixes are obvious, I won't list them here, just making a note to
myself (or whoever).
> + * If there is an interesting revision of the file that is less than or
> + * equal to start, the iteration will start at that revision. Else, the
The first "start" should be "@a start", the second should be "begin",
just to avoid confusion.
> + * iteration will start at the first revision of the file in the repository,
> + * whic has to be less than or equal to end. Note that if the function
The "start" should be "begin", the "end" should be "@a end", and the
"whic" should be "which", of course :-).
> + * succeeds, @a handler will have been called at least once.
> + *
> + * In a series of calls, the file contents for the first interesting revision
> + * will be provided as a text delta against the empty file. In the following
> + * calls, the delta will be against the contents for the previous call.
"In a series of calls," should be "In a series of calls to handler,"
to make it clear that we're not talking about svn_ra_get_file_revs()
there.
We should probably say "fulltext contents" not just "contents", to
make it clear we're talking about the reconstructed file, not the data
that got passed to handler in the previous call (which was a delta).
> +/** 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?
> [...]
I haven't quoted the code, but in general, the way you deprecated
svn_ra_plugin_t was really carefully done. In particular the marking
of the entire structure as deprecated, removing comments that were
made obsolete by the deprecation while preserving those (such as "New
in 1.1") that were not made obsolete. Nice!
The documentation for the now-deprecated svn_ra_init_func_t() should
have mentioned the types of the hash keys, as well as the values.
However, I'm not sure that matters now that the whole function is
obsolete anyay.
> --- trunk/subversion/libsvn_client/blame.c (original)
> +++ trunk/subversion/libsvn_client/blame.c Thu Jan 20 15:05:59 2005
> @@ -514,8 +513,7 @@
> apr_pool_t *pool)
> {
> struct file_rev_baton frb;
> - svn_ra_plugin_t *ra_lib;
> - void *session;
> + svn_ra_session_t *ra_session;
> const char *url;
> svn_revnum_t start_revnum, end_revnum;
> struct blame *walk;
> @@ -530,11 +528,11 @@
> (SVN_ERR_CLIENT_BAD_REVISION, NULL, NULL);
>
> /* Get an RA plugin for this filesystem object. */
> - SVN_ERR (svn_client__ra_lib_from_path (&ra_lib, &session, &end_revnum,
> + SVN_ERR (svn_client__ra_session_from_path (&ra_session, &end_revnum,
> &url, target, peg_revision, end,
> ctx, pool));
Indentation.
> --- trunk/subversion/libsvn_client/checkout.c (original)
> +++ trunk/subversion/libsvn_client/checkout.c Thu Jan 20 15:05:59 2005
> @@ -72,17 +72,16 @@
> URL = svn_path_canonicalize (url, pool);
>
> {
> - void *session;
> - svn_ra_plugin_t *ra_lib;
> + svn_ra_session_t *ra_session;
> svn_node_kind_t kind;
> const char *uuid;
>
> /* Get the RA connection. */
> - SVN_ERR (svn_client__ra_lib_from_path (&ra_lib, &session, &revnum,
> + SVN_ERR (svn_client__ra_session_from_path (&ra_session, &revnum,
> &URL, url, peg_revision,
> revision, ctx, pool));
Indentation.
> --- trunk/subversion/libsvn_client/client.h (original)
> +++ trunk/subversion/libsvn_client/client.h Thu Jan 20 15:05:59 2005
> @@ -163,25 +157,23 @@
> that it is the same node in both PEG_REVISION and REVISON. If it
> is not, then @c SVN_ERR_CLIENT_UNRELATED_RESOURCES is returned.
>
> - The resulting ra_plugin is stored in *RA_LIB_P along with its
> - session baton in *SESSION_P. The actual revision number of the
> - object is stored in *REV_P and the final resulting url is stored in
> - *URL_P.
> + The resulting ra_session is stored in *RA_SESSION_P. The actual
> + revision number of the object is stored in *REV_P and the final
> + resulting url is stored in *URL_P.
Active voice, not passive:
"Store the resulting ra_session in *RA_SESSION_P. Store the actual
revision number of the object in *REV_P, and the final resulting
url in *URL_P."
The same probably goes for the rest of the doc string. And countless
others. But I am unrepentant, for my cause is righteous and just.
> @@ -213,8 +205,7 @@
> } svn_client__callback_baton_t;
>
>
> -/* Open an RA session, returning the session baton in SESSION_BATON. The
> - RA library to use is specified by RA_LIB.
> +/* Open an RA session, returning it in RA_SESSION.
Should be "*RA_SESSION."
> --- trunk/subversion/libsvn_client/copy.c (original)
> +++ trunk/subversion/libsvn_client/copy.c Thu Jan 20 15:05:59 2005
> @@ -763,17 +754,13 @@
> svn_boolean_t same_repositories;
> svn_opt_revision_t revision;
>
> - /* Get the RA vtable that matches URL. */
> - SVN_ERR (svn_ra_init_ra_libs (&ra_baton, pool));
> - SVN_ERR (svn_ra_get_ra_library (&ra_lib, ra_baton, src_url, pool));
> -
> /* Open a repository session to the given URL. We do not (yet) have a
> working copy, so we don't have a corresponding path and tempfiles
> cannot go into the admin area. */
> - SVN_ERR (svn_client__open_ra_session (&sess, ra_lib, src_url, NULL,
> + SVN_ERR (svn_client__open_ra_session (&ra_session, src_url, NULL,
> NULL, NULL, FALSE, TRUE,
> ctx, pool));
> -
> +
Whups, that was just a no-op whitespace change. It totally confused
me, because this was supposed to be a code change, not a formatting
change, and hence I became unable to read the rest of the diff.
Just kidding :-).
> --- trunk/subversion/libsvn_client/ls.c (original)
> +++ trunk/subversion/libsvn_client/ls.c Thu Jan 20 15:05:59 2005
> @@ -84,25 +79,24 @@
> svn_client_ctx_t *ctx,
> apr_pool_t *pool)
> {
> - svn_ra_plugin_t *ra_lib;
> - void *session;
> + svn_ra_session_t *ra_session;
> svn_revnum_t rev;
> svn_node_kind_t url_kind;
> const char *url;
>
> /* Get an RA plugin for this filesystem object. */
> - SVN_ERR (svn_client__ra_lib_from_path (&ra_lib, &session, &rev,
> + SVN_ERR (svn_client__ra_session_from_path (&ra_session, &rev,
> &url, path_or_url, peg_revision,
> revision, ctx, pool));
Indentation.
> --- trunk/subversion/libsvn_client/prop_commands.c (original)
> +++ trunk/subversion/libsvn_client/prop_commands.c Thu Jan 20 15:05:59 2005
> @@ -739,14 +729,14 @@
>
> if (kind == svn_node_dir)
> {
> - SVN_ERR (ra_lib->get_dir (session, target_relative, revnum,
> - (recurse ? &dirents : NULL),
> - NULL, &prop_hash, scratchpool));
> + SVN_ERR (svn_ra_get_dir (ra_session, target_relative, revnum,
> + (recurse ? &dirents : NULL),
> + NULL, &prop_hash, scratchpool));
Indentation.
> --- trunk/subversion/libsvn_client/ra.c (original)
> +++ trunk/subversion/libsvn_client/ra.c Thu Jan 20 15:05:59 2005
> @@ -635,7 +630,7 @@
> /* Let the RA layer drive our log information handler, which will do
> the work of finding the actual locations for our resource.
> Notice that we always run on the youngest rev of the 3 inputs. */
> - SVN_ERR (ra_lib->get_log (ra_session, targets, youngest, 1,
> + SVN_ERR (svn_ra_get_log (ra_session, targets, youngest, 1, 0,
> TRUE, FALSE, log_receiver, &lrb, pool));
Indentation.
> @@ -729,32 +723,32 @@
> don't need to do anything more here in that case. */
>
> /* Open a RA session to this URL. */
> - SVN_ERR (svn_client__open_ra_session (&ra_session, ra_lib, url, NULL,
> + SVN_ERR (svn_client__open_ra_session (&ra_session, url, NULL,
> NULL, NULL, FALSE, TRUE,
> ctx, subpool));
>
> /* Resolve the opt_revision_ts. */
> if (peg_revnum == SVN_INVALID_REVNUM)
> - SVN_ERR (svn_client__get_revision_number (&peg_revnum, ra_lib,
> + SVN_ERR (svn_client__get_revision_number (&peg_revnum,
> ra_session, revision, path,
> pool));
>
> - SVN_ERR (svn_client__get_revision_number (&start_revnum, ra_lib,
> + SVN_ERR (svn_client__get_revision_number (&start_revnum,
> ra_session, start, path, pool));
> if (end->kind == svn_opt_revision_unspecified)
> end_revnum = start_revnum;
> else
> - SVN_ERR (svn_client__get_revision_number (&end_revnum, ra_lib,
> + SVN_ERR (svn_client__get_revision_number (&end_revnum,
> ra_session, end, path, pool));
>
> - SVN_ERR (ra_lib->get_repos_root (ra_session, &repos_url, subpool));
> + SVN_ERR (svn_ra_get_repos_root (ra_session, &repos_url, subpool));
>
> revs = apr_array_make (subpool, 2, sizeof (svn_revnum_t));
> APR_ARRAY_PUSH (revs, svn_revnum_t) = start_revnum;
> if (end_revnum != start_revnum)
> APR_ARRAY_PUSH (revs, svn_revnum_t) = end_revnum;
>
> - if (! (err = ra_lib->get_locations (ra_session, &rev_locs, "", peg_revnum,
> + if (! (err = svn_ra_get_locations (ra_session, &rev_locs, "", peg_revnum,
> revs, subpool)))
Indentation (last line only, before "revs").
> --- trunk/subversion/libsvn_ra/ra_loader.c (original)
> +++ trunk/subversion/libsvn_ra/ra_loader.c Thu Jan 20 15:05:59 2005
> @@ -80,12 +92,20 @@
> { NULL }
> };
>
> -
> +/* 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?
> @@ -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.
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 :-).
> +/* -------------------------------------------------------------- */
> +
> +/*** Public Interfaces ***/
> +svn_error_t *svn_ra_open (svn_ra_session_t **session_p,
> + const char *repos_URL,
> + const svn_ra_callbacks_t *callbacks,
> + void *callback_baton,
> + apr_hash_t *config,
> + apr_pool_t *pool)
> {
> - apr_hash_index_t *this;
> - apr_hash_t *hash = ra_baton;
> -
> - /* Figure out which RA library key matches URL. */
> - for (this = apr_hash_first (pool, hash); this; this = apr_hash_next (this))
> + svn_ra_session_t *session;
> + const struct ra_lib_defn *defn;
> + const svn_ra__vtable_t *vtable = NULL;
> +
> + /* Find the library. */
> + for (defn = ra_libraries; defn->ra_name != NULL; ++defn)
> {
> - const void *key;
> - void *val;
> - apr_ssize_t keylen;
> - const char *keystr;
> -
> - /* Get key and val. */
> - apr_hash_this (this, &key, &keylen, &val);
> - keystr = key;
> -
> - /* 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 (keystr, URL, keylen) == 0 &&
> - (URL[keylen] == ':' || URL[keylen] == '+'))
> + const char *scheme;
> +
> + if ((scheme = has_scheme_of (defn, repos_URL)))
> {
> const svn_version_t *my_version = svn_ra_version();
> const svn_version_t *ra_version;
> + svn_ra__init_func_t initfunc;
>
> - *library = (svn_ra_plugin_t *) val;
> - ra_version = (*library)->get_version();
> - if (!svn_ver_compatible (my_version, ra_version))
> + initfunc = defn->initfunc;
Why not combine declaration and initialization?
svn_ra__init_func_t initfunc = defn->initfunc;
> + 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.
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).
> @@ -303,3 +521,72 @@
> {
> SVN_VERSION_BODY;
> }
> +
> +
> +/*** Compatibility Interfaces **/
> +svn_error_t *
> +svn_ra_init_ra_libs (void **ra_baton,
> + apr_pool_t *pool)
> +{
> + *ra_baton = pool;
> + return SVN_NO_ERROR;
> +}
> +
> +svn_error_t *
> +svn_ra_get_ra_library (svn_ra_plugin_t **library,
> + void *ra_baton,
> + const char *url,
> + apr_pool_t *pool)
> +{
> + const struct ra_lib_defn *defn;
> + apr_pool_t *load_pool = ra_baton;
> + apr_hash_t *ht = apr_hash_make (pool);
> +
> + /* Figure out which RA library key matches URL. */
> + for (defn = ra_libraries; defn->ra_name != NULL; ++defn)
> + {
> + const char *scheme;
> + if ((scheme = has_scheme_of (defn, url)))
> + {
> + const svn_version_t *my_version = svn_ra_version();
> + const svn_version_t *ra_version;
> + svn_ra_init_func_t initfunc;
> +
> + initfunc = defn->compat_initfunc;
Let's please call the variable 'compat_initfunc' then, to avoid
confusion in case someone has recently read the earlier code. And of
course, we could combine declaration and initialization here too.
> + if (! initfunc)
> + SVN_ERR (load_ra_module (NULL, &initfunc, defn->ra_name,
> + load_pool));
> + if (! initfunc)
> + break;
> +
> + SVN_ERR (initfunc (SVN_RA_ABI_VERSION, load_pool, ht));
> +
> + *library = apr_hash_get (ht, scheme, APR_HASH_KEY_STRING);
> +
> + /* The library may support just a subset of the schemes listed,
> + so we have to check here too. */
> + if (! *library)
> + break;
> +
> + ra_version = (*library)->get_version();
> + if (!svn_ver_equal (my_version, ra_version))
> + return svn_error_createf (SVN_ERR_VERSION_MISMATCH, NULL,
> + _("Mismatched RA plugin version"
> + " for '%s':"
> + " found %d.%d.%d%s,"
> + " expected %d.%d.%d%s"),
> + 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;
> + }
> + }
That conditional and the error-creation that follows are what I was
mentioning could be abstracted out, in my earlier comment (see above).
> --- (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.
> + * @file ra_loader.h
> + * @brief structures related to repository access, private to libsvn_ra and the
> + * RA implementation libraries.
> + */
> +
> +
> +
> +#ifndef LIBSVN_RA_RA_LOADER_H
> +#define LIBSVN_RA_RA_LOADER_H
> +
> +#include "svn_ra.h"
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/* The RA layer vtable. */
> +typedef struct svn_ra__vtable_t {
> + /* This field should always remain first in the vtable. */
> + const svn_version_t *(*get_version) (void);
> +
> + /* Return a short description of the RA implementation, as a localized
> + * string. */
> + const char *(*get_description) (void);
> +
> + /* Return a list of actual URI schemes supported by this implementation.
> + * The returned array is NULL terminated. */
> + const char * const *(*get_schemes)(apr_pool_t *pool);
Technically, that should have a hyphen: "NULL-terminated". Now you
know what nitpicker I really am, I will never be able to deceive you
again :-).
> + /* Implementations of the public API functions. */
> +
> + /* 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.
> +/* 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?)
> +/* 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 :-).
> --- (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.
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 really like this macro trick, by the way! Nice thinking.
> + * @file svn_ra.h
> + * @brief structures related to repository access, private to libsvn_ra and the
> + * RA implementation libraries.
> + */
> +
> +/* This file is a template for compatibility wrapper for an RA library.
> + * It contains an svn_ra_plugin_t and wrappers for all of its functions
> + * implemented in terms of svn_ra__vtable_t functions. It also contains
> + * the omplementations of an svn_ra_FOO_init for the FOO RA library.
Grammar: "...is a template for a compatibility wrapper for...".
Typo: "implementations" not "omplementations".
> + * A file in the RA library includes this files providing the following macros
> + * before inclusion:
Typo fix, etc: "...includes this file, defining the...".
> + * 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.".
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.
> +/* Check that all our "arguments" are defined. */
> +#if ! defined(NAME) || ! defined(DESCRIPTION) || ! defined(VTBL) \
> + || ! defined(INITFUNC) || ! defined(COMPAT_INITFUNC)
> +#error Missing define for RA compatibility wrapper.
> +#endif
> +
> +static svn_error_t *compat_open (void **session_baton,
> + const char *repos_URL,
> + const svn_ra_callbacks_t *callbacks,
> + void *callback_baton,
> + apr_hash_t *config,
> + apr_pool_t *pool)
> +{
> + svn_ra_session_t *sess = apr_pcalloc (pool, sizeof (svn_ra_session_t));
> + sess->vtable = &VTBL;
> + sess->pool = pool;
> + SVN_ERR (VTBL.open (sess, repos_URL, callbacks, callback_baton,
> + config, pool));
Indentation.
> + *session_baton = sess;
> + return SVN_NO_ERROR;
> +}
> +
> +static svn_error_t *compat_get_latest_revnum (void *session_baton,
> + svn_revnum_t *latest_revnum,
> + apr_pool_t *pool)
> +{
> + return VTBL.get_latest_revnum (session_baton, latest_revnum, pool);
> +}
> +
> +static svn_error_t *compat_get_dated_revision (void *session_baton,
> + svn_revnum_t *revision,
> + apr_time_t tm,
> + apr_pool_t *pool)
> +{
> + return VTBL.get_dated_revision (session_baton, revision, tm, pool);
> +}
> +
> +static svn_error_t *compat_change_rev_prop (void *session_baton,
> + svn_revnum_t rev,
> + const char *propname,
> + const svn_string_t *value,
> + apr_pool_t *pool)
> +{
> + return VTBL.change_rev_prop (session_baton, rev, propname, value, pool);
> +}
> +
> +static svn_error_t *compat_rev_proplist (void *session_baton,
> + svn_revnum_t rev,
> + apr_hash_t **props,
> + apr_pool_t *pool)
> +{
> + return VTBL.rev_proplist (session_baton, rev, props, pool);
> +}
> +
> +static svn_error_t *compat_rev_prop (void *session_baton,
> + svn_revnum_t rev,
> + const char *propname,
> + svn_string_t **value,
> + apr_pool_t *pool)
> +{
> + return VTBL.rev_prop (session_baton, rev, propname, value, pool);
> +}
> +
> +static svn_error_t *compat_get_commit_editor (void *session_baton,
> + const svn_delta_editor_t
> + **editor,
> + void **edit_baton,
> + const char *log_msg,
> + svn_commit_callback_t callback,
> + void *callback_baton,
> + apr_pool_t *pool)
> +{
> + return VTBL.get_commit_editor (session_baton, editor, edit_baton, log_msg,
> + callback, callback_baton, pool);
> +}
> +
> +static svn_error_t *compat_get_file (void *session_baton,
> + const char *path,
> + svn_revnum_t revision,
> + svn_stream_t *stream,
> + svn_revnum_t *fetched_rev,
> + apr_hash_t **props,
> + apr_pool_t *pool)
> +{
> + return VTBL.get_file (session_baton, path, revision, stream, fetched_rev,
> + props, pool);
> +}
> +
> +static svn_error_t *compat_get_dir (void *session_baton,
> + const char *path,
> + svn_revnum_t revision,
> + apr_hash_t **dirents,
> + svn_revnum_t *fetched_rev,
> + apr_hash_t **props,
> + apr_pool_t *pool)
> +{
> + return VTBL.get_dir (session_baton, path, revision, dirents, fetched_rev,
> + props, pool);
> +}
> +
> +static svn_error_t *compat_do_update (void *session_baton,
> + 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 *editor,
> + void *update_baton,
> + apr_pool_t *pool)
> +{
> + return VTBL.do_update (session_baton, reporter, report_baton,
> + revision_to_update_to, update_target, recurse,
> + editor, update_baton, pool);
> +}
> +
> +static svn_error_t *compat_do_switch (void *session_baton,
> + 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 *editor,
> + void *switch_baton,
> + apr_pool_t *pool)
> +{
> + return VTBL.do_switch (session_baton, reporter, report_baton,
> + revision_to_switch_to, switch_target, recurse,
> + switch_url, editor, switch_baton, pool);
> +}
> +
> +static svn_error_t *compat_do_status (void *session_baton,
> + 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 *editor,
> + void *status_baton,
> + apr_pool_t *pool)
> +{
> + return VTBL.do_status (session_baton, reporter, report_baton,
> + status_target,
> + revision, recurse, editor, status_baton, pool);
> +}
Indentation (and weird line-breaking, too).
> +static svn_error_t *compat_do_diff (void *session_baton,
> + 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)
> +{
> + return VTBL.do_diff (session_baton, reporter, report_baton, revision,
> + diff_target, recurse, ignore_ancestry, versus_url,
> + diff_editor, diff_baton, pool);
> +}
> +
> +static svn_error_t *compat_get_log (void *session_baton,
> + const apr_array_header_t *paths,
> + svn_revnum_t start,
> + svn_revnum_t end,
> + 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)
> +{
> + return VTBL.get_log (session_baton, paths, start, end, 0, /* limit */
> + discover_changed_paths, strict_node_history,
> + receiver, receiver_baton, pool);
> +}
> +
> +static svn_error_t *compat_check_path (void *session_baton,
> + const char *path,
> + svn_revnum_t revision,
> + svn_node_kind_t *kind,
> + apr_pool_t *pool)
> +{
> + return VTBL.check_path (session_baton, path, revision, kind, pool);
> +}
> +
> +static svn_error_t *compat_get_uuid (void *session_baton,
> + const char **uuid,
> + apr_pool_t *pool)
> +{
> + return VTBL.get_uuid (session_baton, uuid, pool);
> +}
> +
> +static svn_error_t *compat_get_repos_root (void *session_baton,
> + const char **url,
> + apr_pool_t *pool)
> +{
> + return VTBL.get_repos_root (session_baton, url, pool);
> +}
> +
> +static svn_error_t *compat_get_locations (void *session_baton,
> + apr_hash_t **locations,
> + const char *path,
> + svn_revnum_t peg_revision,
> + apr_array_header_t *location_revs,
> + apr_pool_t *pool)
> +{
> + return VTBL.get_locations (session_baton, locations, path, peg_revision,
> + location_revs, pool);
> +}
> +
> +static svn_error_t *compat_get_file_revs (void *session_baton,
> + const char *path,
> + svn_revnum_t start,
> + svn_revnum_t end,
> + svn_ra_file_rev_handler_t handler,
> + void *handler_baton,
> + apr_pool_t *pool)
> +{
> + return VTBL.get_file_revs (session_baton, path, start, end, handler,
> + handler_baton, pool);
> +}
> +
> +static const svn_version_t *compat_get_version (void)
> +{
> + return VTBL.get_version ();
> +}
> +
> +static const svn_ra_plugin_t compat_plugin = {
> + NAME,
> + DESCRIPTION,
> + compat_open,
> + compat_get_latest_revnum,
> + compat_get_dated_revision,
> + compat_change_rev_prop,
> + compat_rev_proplist,
> + compat_rev_prop,
> + compat_get_commit_editor,
> + compat_get_file,
> + compat_get_dir,
> + compat_do_update,
> + compat_do_switch,
> + compat_do_status,
> + compat_do_diff,
> + compat_get_log,
> + compat_check_path,
> + compat_get_uuid,
> + 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.
> +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().
> + 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 :-).
> --- 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.
> --- 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.
> --- 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).
> @@ -713,25 +713,12 @@
> /*
> * Implements the get_locations RA layer function. */
> svn_error_t *
> -svn_ra_dav__get_locations (void *session_baton,
> +svn_ra_dav__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);
> -
> -/** @since New in 1.2. */
> -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);
>
> #ifdef __cplusplus
> }
Heh, more fallout from r11155, yeah.
> --- 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.
> @@ -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?
> @@ -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.
> --- 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...)
> 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?
> @@ -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 :-).
> @@ -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.
> @@ -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.
> --- 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?
(I suppose I could just go look at the code myself, but I'm getting a
bit woozy now. I'm on page 117 of the printout of your commit, 7 more
pages to go...)
> @@ -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.)
> @@ -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?
Also, the first line of the function definition line now exceeds 80
characters.
> @@ -1337,15 +1346,26 @@
> { "svn_delta", svn_delta_version },
> { 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_svn."), abi_version);
> +
> SVN_ERR(svn_ver_check_list(svn_ra_svn_version(), checklist));
>
> - apr_hash_set(hash, "svn", APR_HASH_KEY_STRING, &ra_svn_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_svn"),
> + loader_version->major);
> +
> + *vtable = &ra_svn_vtable;
Same thing about line break, and about abstraction of this whole
routine.
> return SVN_NO_ERROR;
> }
> +
> +/* Compatibility wrapper for the 1.1 and before API. */
> +#define NAME "ra_svn"
> +#define DESCRIPTION RA_SVN_DESCRIPTION
> +#define VTBL ra_svn_vtable
> +#define INITFUNC svn_ra_svn__init
> +#define COMPAT_INITFUNC svn_ra_svn_init
> +#include "../libsvn_ra/wrapper_template.h"
Okay, now it's time for me to confess to a bit of confusion about the
general plan here.
I see that the COMPAT_INITFUNC causes wrapper_template.(h->c) to
define the function svn_ra_svn_init(). And I can see that we define
VTBL to ra_svn_vtable, a structure which starts out with 'description'
followed by 'version', then other stuff -- in other words, that
structure has the new order, not the old ra_plugin vtable order.
So, who is providing the old vtable with its old ordering? If someone
is writing code against that API, where do we satisfy their need for
that old structure layout?
I'm sure you've got it covered, and I'm just not seeing it. Sorry to
have to ask, I tried to puzzle it out myself but (so far) have failed.
Okay, that's the whole review.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Feb 2 20:53:39 2005