On Wed, 10 Aug 2005, Madan U Sreenivasan wrote:
> Pl. find attached the step-2 of the patch for issue #443. These steps
There is one thing that I see as a programming error, else just style
nits. So, it is nearly ready.
> Partial fix for Issue #443: post-commit hook script (error) output lost
> This is step 2 : Use svn_client_commit_info2_t everywhere
> and the svn_client_create_commit_info() constructor for
> creating svn_client_commit_info2_t objects.
>
Good summary!
> * subversion/include/svn_client.h
> (svn_client_mkdir)
> (svn_client_delete)
> (svn_client_commit2)
> (svn_client_copy)
> (svn_client_move2): Deprecate
>
You can make this more compact by including all function names in the same parens, separated by commas and more than one per line. That's how we usually do it.
> (svn_client_import2): Modified. Use svn_client_commit_info2_t.
>
"Modified." is redundant.
> * subversion/libsvn_client/commit.c
> (get_ra_editor): Modified. Use svn_client_commit_info2_t.
> (svn_client_import2): Modified. Now use svn_client_commit_info2_t
> and the svn_client_create_commit_info() constructor.
> (svn_client_import): Modified. Typecase commit_info to
> svn_client_commit_info2_t while calling svn_client_import2.
You only need to describe the change, not the implementation. "Wrap
svn_client_import2()" would be enough.
> (svn_client_commit3): New version of svn_client_commit2. Now use
> svn_client_commit_info2_t.
> (svn_client_commit2): Deprecate.
>
Did you deprecate it in the .c file? :-) "Wrap..."
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 15659)
> +++ subversion/include/svn_client.h (working copy)
> +/** Same as svn_client_mkdir2, but takes the svn_client_commit_info_t
When naming a function in a doxygen docstring, add a pair of
parenthesis at the end. This makes doxygen link to the documentation
for that function. And use @c before a type like
svn_client_commit_info_t. These two comments apply in more places in
this file; I won't pick on them all...
I think you could deprecate svn_client_commit_info_t in this step as
well, or is there something that still uses it?
> Index: subversion/libsvn_client/delete.c
> ===================================================================
> --- subversion/libsvn_client/delete.c (revision 15659)
> +++ subversion/libsvn_client/delete.c (working copy)
> +
> +
> +svn_error_t *
> +svn_client_delete (svn_client_commit_info_t **commit_info,
> + const apr_array_header_t *paths,
> + svn_boolean_t force,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool)
> +{
> + /* Memory alloc inside function - typecast and pass on */
> + return svn_client_delete2 ( (svn_client_commit_info2_t **) commit_info,
> + paths,
> + force,
> + ctx,
> + pool);
This probably works, but in general you can't cast pointer-to-pointer
to pointer-to-pointer of another type.
svn_client_commit_info2_t *commit_info2 = NULL;
SVN_ERR (svn_client_delete2 (&commit_info2, ...
/* These structs have the same layout for the common fields. */
*commit_info = commit_info2;
return err;
avoids this. This applies to all wrapper functions.
> +}
> Index: subversion/libsvn_client/commit.c
> ===================================================================
> --- subversion/libsvn_client/commit.c (revision 15659)
> +++ subversion/libsvn_client/commit.c (working copy)
> @@ -588,7 +588,7 @@
> svn_wc_adm_access_t *base_access,
> const char *log_msg,
> apr_array_header_t *commit_items,
> - svn_client_commit_info_t **commit_info,
> + svn_client_commit_info2_t **commit_info,
> svn_boolean_t is_commit,
> apr_hash_t *lock_tokens,
> svn_boolean_t keep_locks,
> @@ -633,7 +633,7 @@
> /*** Public Interfaces. ***/
>
> svn_error_t *
> -svn_client_import2 (svn_client_commit_info_t **commit_info,
> +svn_client_import2 (svn_client_commit_info2_t **commit_info,
> const char *path,
> const char *url,
> svn_boolean_t nonrecursive,
> @@ -770,9 +770,9 @@
> /* Transfer *COMMIT_INFO from the subpool to the callers pool */
> if (*commit_info)
> {
> - svn_client_commit_info_t *tmp_commit_info;
> + svn_client_commit_info2_t *tmp_commit_info;
>
> - tmp_commit_info = apr_palloc(pool, sizeof(*tmp_commit_info));
> + tmp_commit_info = svn_client_create_commit_info(pool);
Space before paren. (Yes, it was inconsistent before...)
Else it looks great. Would you resubmit with the wrappers fixed (and
preferrably the style nits as well)?
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Aug 11 10:09:38 2005