Noted the points, Peter. Thanks for the review.
Wrap... I agree... would do that
typecast removal... I think yours is a better idea... would do that too.
Will take of these issues, run a test and send them back. Thanks again
for the response.
On Thu, 2005-08-11 at 13:38, Peter N. Lundblad wrote:
> 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:33:59 2005