Some nits/thoughts. No problems that I see tho... excellent cleanup overall.
On Wed, Jan 16, 2002 at 12:46:44PM -0600, cmpilato@tigris.org wrote:
>...
> +svn_client_mkdir (svn_client_commit_info_t **commit_info,
> + svn_stringbuf_t *path,
> svn_client_auth_baton_t *auth_baton,
> svn_stringbuf_t *message,
> apr_pool_t *pool);
That could be "const svn_client_commit_info_t **commit_info".
>...
> +++ NEW/trunk/subversion/libsvn_client/delete.c Wed Jan 16 12:46:44 2002
>...
> + /* Allocate (and populate) the commit_info */
> + if ((committed_date != NULL)
> + || (committed_author != NULL)
> + || (SVN_IS_VALID_REVNUM (committed_rev)))
> + {
> + svn_client_commit_info_t *info;
> +
> + info = apr_pcalloc (pool, sizeof (**commit_info));
> + if (committed_date)
> + info->date = apr_pstrdup (pool, committed_date);
> + if (committed_author)
> + info->date = apr_pstrdup (pool, committed_author);
> + info->revision = committed_rev;
> + *commit_info = info;
> + }
This code block is replicated a number of times in the client code. It's a
good candidate for a general utility function.
That utility function could also do two new things:
* store NULL into *commit_info if it isn't going to alloc a structure
* allow a person to pass commit_info==NULL if they don't want the data
Teeny nit: the standard pattern is to have sizeof of the target variable
(info in this case rather than commit_info).
>...
> +++ NEW/trunk/subversion/clients/cmdline/mkdir-cmd.c Wed Jan 16 12:46:44 2002
> @@ -43,6 +43,7 @@
> svn_client_auth_baton_t *auth_baton = NULL;
> svn_stringbuf_t *message = NULL;
> int i;
> + svn_client_commit_info_t *commit_info = NULL;
That =NULL would be obsoleted if the utility function for populating
commit_info would store NULL if it didn't allocate.
> targets = svn_cl__args_to_target_array (os, pool);
>
> @@ -59,7 +60,11 @@
> for (i = 0; i < targets->nelts; i++)
> {
> svn_stringbuf_t *target = ((svn_stringbuf_t **) (targets->elts))[i];
> - SVN_ERR (svn_client_mkdir (target, auth_baton, message, pool));
> + commit_info = NULL;
double-init
>...
> +++ NEW/trunk/subversion/clients/cmdline/move-cmd.c Wed Jan 16 12:46:44 2002
> @@ -43,6 +43,7 @@
> svn_stringbuf_t *src_path, *dst_path;
> svn_client_auth_baton_t *auth_baton = NULL;
> svn_stringbuf_t *message = NULL;
> + svn_client_commit_info_t *commit_info = NULL;
same
>...
> +++ NEW/trunk/subversion/clients/cmdline/copy-cmd.c Wed Jan 16 12:46:44 2002
> @@ -47,6 +47,7 @@
> const svn_delta_edit_fns_t *trace_editor = NULL;
> void *trace_edit_baton = NULL;
> svn_boolean_t src_is_url, dst_is_url;
> + svn_client_commit_info_t *commit_info = NULL;
again
>...
> +++ NEW/trunk/subversion/clients/cmdline/commit-cmd.c Wed Jan 16 12:46:44 2002
> @@ -48,9 +48,7 @@
> const svn_delta_edit_fns_t *trace_editor;
> void *trace_edit_baton;
> svn_client_auth_baton_t *auth_baton;
> - svn_revnum_t new_rev; /* The revision resulting from the commit. */
> - const char *date; /* Server-side date of the commit. */
> - const char *author; /* Server-side author of the commit. */
> + svn_client_commit_info_t *commit_info = NULL;
one more
>...
> +++ NEW/trunk/subversion/clients/cmdline/delete-cmd.c Wed Jan 16 12:46:44 2002
> @@ -43,6 +43,7 @@
> svn_client_auth_baton_t *auth_baton = NULL;
> svn_stringbuf_t *message = NULL;
> int i;
> + svn_client_commit_info_t *commit_info = NULL;
okay. two more.
> targets = svn_cl__args_to_target_array (os, pool);
>
> @@ -59,8 +60,11 @@
> for (i = 0; i < targets->nelts; i++)
> {
> svn_stringbuf_t *target = ((svn_stringbuf_t **) (targets->elts))[i];
> - SVN_ERR (svn_client_delete (target, opt_state->force,
> + commit_info = NULL;
double-init
>...
> +++ NEW/trunk/subversion/clients/cmdline/import-cmd.c Wed Jan 16 12:46:44 2002
>...
> + svn_client_commit_info_t *commit_info = NULL;
okay. I lied. :-)
>...
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Oct 21 14:36:56 2006