On Wed, 5 Oct 2005, Madan US wrote:
> > Hi,
> > Pl. find attached step 5 of the issue #443. This would be the last of
> > the preparatory steps towards the same.
> >
> > Thanks to Peter Lundblad for the excellent inputs about wrapper batons.
>
> More thanks to Peter Lundblad... we were able to use the same wrapper
> baton and callbacks everywhere by putting them in libsvn_subr....
>
> Pl. find the patch and log attached...
>
And here's the review...
> Index: subversion/libsvn_ra/wrapper_template.h
> ===================================================================
> --- subversion/libsvn_ra/wrapper_template.h (revision 16478)
> +++ subversion/libsvn_ra/wrapper_template.h (working copy)
> @@ -37,6 +37,7 @@
> #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,
> @@ -128,8 +129,14 @@
> void *callback_baton,
> apr_pool_t *pool)
> {
> + void *wrapper_baton = commit_wrapper_baton_create (callback_baton,
> + callback,
> + pool);
> +
> + /* Use the wrapper callback and baton */
I find this comment pretty useless. I think it's commenting the
obvious from the code below. (Same applies in similar places.)
> return VTBL.get_commit_editor (session_baton, editor, edit_baton, log_msg,
> - callback, callback_baton, NULL, TRUE, pool);
> + commit_wrapper_callback, wrapper_baton,
> + NULL, TRUE, pool);
> }
>
> static svn_error_t *compat_get_file (void *session_baton,
> Index: subversion/include/svn_repos.h
> ===================================================================
> --- subversion/include/svn_repos.h (revision 16478)
> +++ subversion/include/svn_repos.h (working copy)
> @@ -678,7 +678,7 @@
> const char *base_path,
> const char *user,
> const char *log_msg,
> - svn_commit_callback_t callback,
> + svn_commit_callback2_t callback2,
> void *callback_baton,
> svn_repos_authz_callback_t authz_callback,
> void *authz_baton,
> @@ -686,7 +686,8 @@
>
> /**
> * Similar to svn_repos_get_commit_editor3(), but with @a
> - * authz_callback and @a authz_baton set to @c NULL.
> + * authz_callback and @a authz_baton set to @c NULL
> + * and without the @a callback2 parameter.
> *
> * @deprecated Provided for backward compatibility with the 1.2 API.
> */
Since this is going into 1.4 now, you can't just change this public
API. You need to revise it.
> Index: subversion/include/svn_types.h
> ===================================================================
> --- subversion/include/svn_types.h (revision 16478)
> +++ subversion/include/svn_types.h (working copy)
> @@ -407,7 +407,19 @@
> * When a commit succeeds, an instance of this is invoked on the @a
> * new_revision, @a date, and @a author of the commit, along with the
> * @a baton closure.
> + *
> + * @since New in 1.3.
1.3 that is.
> */
> +typedef svn_error_t * (*svn_commit_callback2_t) (
> + svn_commit_info_t *commit_info,
Should be pointer to const.
> + void *baton);
> +
> +
> +/** Same as svn_commit_callback2_t, but uses individual
> + * data elements instead of the svn_commit_info_t structure
> + *
> + * @deprecated Provided for backward compatibility with the 1.2 API.
...1.3 API.
> + */
> typedef svn_error_t * (*svn_commit_callback_t) (
> svn_revnum_t new_revision,
> const char *date,
> @@ -415,6 +427,22 @@
> void *baton);
>
>
> +/* for compatibility with svn_commit_callback_t */
> +struct commit_wrapper_baton {
> + void *baton;
> + svn_commit_callback_t callback;
> +};
> +
This doesn't need to be in a public header.
> +/* commit wrapper baton creation function */
> +void *commit_wrapper_baton_create (void *callback_baton,
> + svn_commit_callback_t callback,
> + apr_pool_t *pool);
> +
> +/* commit wrapper callback function */
> +svn_error_t *commit_wrapper_callback (svn_commit_info_t *commit_info,
> + void *baton);
> +
YOu can't use names without the svn_ prefix in public headers. Instead
of the above, I suggest creating one public API:
void svn_compat_wrap_commit_callback (svn_commit_callback_t **callback,
void **baton,
svn_commit_callback2_t
*callback2,
void **baton2,
apr_pool_t *pool);
That's only one API and keeping the rest internal.
[To others following this thread: this is ugly, but this wrapper is
used in three places, so we have to put it in libsvn_subr.]
> +
> /** The maximum amount we (ideally) hold in memory at a time when
> * processing a stream of data.
> *
> Index: subversion/include/svn_ra.h
> ===================================================================
> --- subversion/include/svn_ra.h (revision 16478)
> +++ subversion/include/svn_ra.h (working copy)
> @@ -576,8 +576,23 @@
> *
> * Use @a pool for memory allocation.
> *
> - * @since New in 1.2.
> + * @since New in 1.3.
1.3 :-)
> */
> +svn_error_t *svn_ra_get_commit_editor2 (svn_ra_session_t *session,
> + const svn_delta_editor_t **editor,
> + void **edit_baton,
> + const char *log_msg,
> + svn_commit_callback2_t callback,
> + void *callback_baton,
> + apr_hash_t *lock_tokens,
> + svn_boolean_t keep_locks,
> + apr_pool_t *pool);
> +
> +/**
> + * Same as svn_ra_get_commit_editor2, but uses svn_commit_callback_t.
> + *
> + * @deprecated Provided for backward compatibility with the 1.2 API.
1.3 API.
> Index: subversion/libsvn_client/commit_util.c
> ===================================================================
> --- subversion/libsvn_client/commit_util.c (revision 16478)
> +++ subversion/libsvn_client/commit_util.c (working copy)
> @@ -1337,18 +1337,21 @@
> return SVN_NO_ERROR;
> }
>
> -svn_error_t *svn_client__commit_callback (svn_revnum_t revision,
> - const char *date,
> - const char *author,
> +svn_error_t *svn_client__commit_callback (svn_commit_info_t *commit_info,
> void *baton)
> {
> struct commit_baton *cb = baton;
> svn_commit_info_t **info = cb->info;
>
> *info = svn_create_commit_info (cb->pool);
> - (*info)->date = date ? apr_pstrdup (cb->pool, date) : NULL;
> - (*info)->author = author ? apr_pstrdup (cb->pool, author) : NULL;
> - (*info)->revision = revision;
> + if (commit_info)
Can commit_info really be NULL?
> + {
> + (*info)->date = commit_info->date ?
> + apr_pstrdup (cb->pool, commit_info->date) : NULL;
> + (*info)->author = commit_info->author ?
> + apr_pstrdup (cb->pool, commit_info->author) : NULL;
> + (*info)->revision = commit_info->revision;
It looks like we need an svn_dup_commit_info function for deep-copying
a commit_info struct. The problem with this code is that it needs to
be updated when we add a new field to svn_commit_info_t, which is easy
to forget.
> Index: subversion/libsvn_repos/commit.c
> ===================================================================
> --- subversion/libsvn_repos/commit.c (revision 16478)
> +++ subversion/libsvn_repos/commit.c (working copy)
> @@ -50,7 +50,7 @@
> const char *log_msg;
>
> /* Callback to run when the commit is done. */
> - svn_commit_callback_t commit_callback;
> + svn_commit_callback2_t commit_callback2;
You shouldn't rename the field. The '2' is just an artifact of the API
revving. 'callback' is still fine.
> @@ -770,7 +774,7 @@
> const char *base_path,
> const char *user,
> const char *log_msg,
> - svn_commit_callback_t callback,
> + svn_commit_callback2_t callback2,
Same here.
> Index: subversion/libsvn_ra_svn/client.c
> ===================================================================
> --- subversion/libsvn_ra_svn/client.c (revision 16478)
> +++ subversion/libsvn_ra_svn/client.c (working copy)
> @@ -56,7 +56,7 @@
> ra_svn_session_baton_t *sess_baton;
> apr_pool_t *pool;
> svn_revnum_t *new_rev;
> - svn_commit_callback_t callback;
> + svn_commit_callback2_t callback;
> void *callback_baton;
> } ra_svn_commit_callback_baton_t;
>
> @@ -802,18 +802,17 @@
> static svn_error_t *ra_svn_end_commit(void *baton)
> {
> ra_svn_commit_callback_baton_t *ccb = baton;
> - svn_revnum_t new_rev;
> - const char *committed_date;
> - const char *committed_author;
> + svn_commit_info_t *commit_info = NULL;
>
> + commit_info = svn_create_commit_info (ccb->pool);
> +
You could initialize the variable at the declaration.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Oct 5 22:53:41 2005