Re: [PATCH] Issue 443 : post-commit hook (error) output lost: Step 5

On 10/7/05, Madan US <madan@collab.net> wrote:
> >> On Thu, 2005-10-06 at 02:21, Peter N. Lundblad wrote:
> >> And here's the review...
> >
> > Pl. find the revised patch attached.
> >
>
> Pl. find attached the same patch, but the dup function changed to allocate
> and copy.
Hi Madan,

Thanks for your patch! It looks pretty good. I'd like to suggest few
small improvements.

> --- subversion/include/svn_types.h (revision 16478)
> +++ subversion/include/svn_types.h (working copy)
> @@ -339,6 +339,25 @@
> svn_create_commit_info (apr_pool_t *pool);
>
> 
> +/**
> + * Perform a deep copy of the @c svn_commit_info_t structure
> + * from @a src_commit_info to @a dst_commit_info.
> + *
> + * @note Bothe @a src_commit_info and @a dst_commit_info are

s/Bothe/Both/.

> + * to be pre-allocated. But the internal memory requirements
> + * are handled from the @a pool.
> + *
> + * This is to provide for extending the svn_commit_info_t in
> + * the future.
> + *
> + * @since New in 1.4
> + */
> +void
> +svn_dup_commit_info (const svn_commit_info_t *src_commit_info,
> + svn_commit_info_t **dst_commit_info,
> + apr_pool_t *pool);

I'd like to make the svn_dup_commit_info function consistent with our
other "deep copy" functions.
- Rename this function to "svn_commit_info_dup"
- Instead of accepting dst_commit_info as an argument, return a
svn_commit_info_t * pointer.

Do you think this is a good idea? This is a small change, so if you
like the change, we can apply it when we commit the patch.

Cheers,

David

--
David James -- http://www.cs.toronto.edu/~james