You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Madan U Sreenivasan <ma...@collab.net> on 2005/08/10 17:02:28 UTC
[PATCH] Issue 443: post-commit hook (error) output lost: Step 2
Hi,
Pl. find attached the step-2 of the patch for issue #443. These steps
were discussed on the list at...
http://svn.haxx.se/dev/archive-2005-07/0281.shtml
Regards,
Madan.
Re: [PATCH] Issue 443: post-commit hook (error) output lost: Step 2
Posted by Madan U Sreenivasan <ma...@collab.net>.
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
Re: [PATCH] Issue 443: post-commit hook (error) output lost: Step 2
Posted by Madan U Sreenivasan <ma...@collab.net>.
On Fri, 2005-08-12 at 15:25, Peter N. Lundblad wrote:
> On Fri, 12 Aug 2005, Madan U Sreenivasan wrote:
>
> > oops, my bad... sorry. Sending the log herewith...
> >
> OK. r15694. Just very small style tweaks by me. Well done!
Thank you, Peter... will start working on step 3 right away!
>
> Regards,
> //Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue 443: post-commit hook (error) output lost: Step
2
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Fri, 12 Aug 2005, Madan U Sreenivasan wrote:
> oops, my bad... sorry. Sending the log herewith...
>
OK. r15694. Just very small style tweaks by me. Well done!
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue 443: post-commit hook (error) output lost: Step 2
Posted by Madan U Sreenivasan <ma...@collab.net>.
On Thu, 2005-08-11 at 19:01, Peter N. Lundblad wrote:
> On Thu, 11 Aug 2005, Madan U Sreenivasan wrote:
>
> > Pl. find attached, the patch with changes suggested....
> > also fixed a warning that occurred with v1 of this patch.
> >
> The patch is there, but no log message AFAICT. Seems like you accidentally
> sent another patch instead of the log. Can you just send the log message?
oops, my bad... sorry. Sending the log herewith...
>
> Thanks,
> //Peter
Re: [PATCH] Issue 443: post-commit hook (error) output lost: Step
2
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Thu, 11 Aug 2005, Madan U Sreenivasan wrote:
> Pl. find attached, the patch with changes suggested....
> also fixed a warning that occurred with v1 of this patch.
>
The patch is there, but no log message AFAICT. Seems like you accidentally
sent another patch instead of the log. Can you just send the log message?
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue 443: post-commit hook (error) output lost: Step 2
Posted by Madan U Sreenivasan <ma...@collab.net>.
Pl. find attached, the patch with changes suggested....
also fixed a warning that occurred with v1 of this patch.
Regards,
Madan.
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
Re: [PATCH] Issue 443: post-commit hook (error) output lost: Step
2
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
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