Madan US wrote:
> [[[
> Partial Fix for Issue #443: post-commit hook script (error) output lost
>
Please begin the log message with a description of what the patch does (and
why, if that's not obvious).
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 14776)
> +++ subversion/include/svn_client.h (working copy)
[...]
> @@ -653,60 +679,90 @@
> svn_client_ctx_t *ctx,
> apr_pool_t *pool);
>
> -/** Create a directory, either in a repository or a working copy.
> - *
> - * If @a paths contains URLs, use the authentication baton in @a ctx
> - * and @a message to immediately attempt to commit the creation of the
> - * directories in @a paths in the repository. If the commit succeeds,
> - * allocate (in @a pool) and populate @a *commit_info.
[...]
> +/** @since New in 1.3.
> + * Create a directory, either in a repository or a working copy.
> + *
> + * If @a paths contains URLs, use the authentication baton in @a ctx
> + * and @a message to immediately attempt to commit the creation of the
> + * directories in @a paths in the repository. If the commit succeeds,
> + * allocate (in @a pool) and populate @a *commit_info.
[...]
Please don't change the indentation. This is part of the reason why this patch
is so big.
> Index: subversion/libsvn_client/copy.c
> ===================================================================
> --- subversion/libsvn_client/copy.c (revision 14776)
> +++ subversion/libsvn_client/copy.c (working copy)
> @@ -269,7 +269,235 @@
> return SVN_NO_ERROR;
> }
>
> +/** @since New in 1.3.
> + *
> + * Now uses svn_client_commit_info2_t which is post-commit
> + * hook's stderr aware.
> + */
> +static svn_error_t *
> +repos_to_repos_copy2 (svn_client_commit_info2_t **commit_info,
> + const char *src_url,
> + const svn_opt_revision_t *src_revision,
> + const char *dst_url,
> + svn_client_ctx_t *ctx,
> + svn_boolean_t is_move,
> + apr_pool_t *pool)
> +{
[about 200 lines]
> +}
I don't think it's acceptable to duplicate several large functions like this
one just to change two lines in them. You will have to find a way to factor
out the common parts.
> +/** @deprecated Provided for backward compatibility
> + *
> + * Similar to repos_to_repos_copy2, but uses svn_client_commit_info_t
> + * for @a commit_info type
> + */
> static svn_error_t *
> repos_to_repos_copy (svn_client_commit_info_t **commit_info,
> const char *src_url,
> @@ -579,6 +807,11 @@
>
>
>
> +/** @deprecated Provided for backward compatibility.
> + *
> + * Similar to wc_to_repos_copy but uses svn_client_commit_info_t
> + * for the @a commit_info parameter.
> + */
> static svn_error_t *
> wc_to_repos_copy (svn_client_commit_info_t **commit_info,
> const char *src_path,
These comments are inappropriate. A static function isn't deprecated, it's
either used or deleted; in this case is it used (by a deprecated function). We
only use Doxygen mark-up in public API headers. "Similar to wc_to_repos_copy"?
... this is "wc_to_repos_copy".
This message was a bit big for the mailing list at 161 KB. When you have a big
patch, please consider making it smaller or posting a URL to it instead, or
take it as a sign that there may be something wrong with the patch.
I haven't reviewed your implementation. I'm glad you're attacking the problem,
though.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat May 21 19:41:15 2005