I disagree with this change.
You're adding a bunch of complexity in order to save *three*
arguments? That is a very poor tradeoff. Now we have a structure,
constructors, people have to create/init that structure to call the
function, the function will just unpack its values, etc.
Now, if there were *ten* arguments, then... well, probably still "no".
Packing arguments into a struct to be unpacked within the function
generally does not help anything. It just makes it harder to use.
Rev'ing a function in the API is not a difficult task. Callbacks are
hard, but this is a front-facing API that is easy to change.
-g
On Tue, Jan 27, 2009 at 23:23, Bert Huijben <rhuijben_at_sharpsvn.net> wrote:
> Author: rhuijben
> Date: Tue Jan 27 14:23:31 2009
> New Revision: 35512
>
> Log:
> Following up on r32970, introduce a structure to hold arguments to
> svn_client_status4(), hoping that in the future we can just add members to
> the structure instead of rev'ing the entire API.
>
> * subversion/include/svn_client.h
> (svn_client_status_args_t): New.
> (svn_client_status_args_create): New.
> (svn_client_status4): Update prototype to use svn_client_status_args_t.
> * subversion/libsvn_client/client.h
> (svn_client__do_external_status): Forward svn_client_status_args_t instead
> of individual values.
> * subversion/libsvn_client/delete.c
> (svn_client__can_delete): Updated for calling svn_client_status4.
> * subversion/libsvn_client/deprecated.c
> (svn_client_status3): Updated for calling svn_client_status4.
> * subversion/libsvn_client/externals.c
> (svn_client__do_external_status): Forward args to svn_client_status4.
> * subversion/libsvn_client/status.c
> (svn_client_status4): Arguments updated.
> (svn_client_status_args_create): New.
> * subversion/svn/status-cmd.c
> (svn_cl__status): Updated for calling svn_client_status4.
>
> Modified:
> trunk/subversion/include/svn_client.h
> trunk/subversion/libsvn_client/client.h
> trunk/subversion/libsvn_client/delete.c
> trunk/subversion/libsvn_client/deprecated.c
> trunk/subversion/libsvn_client/externals.c
> trunk/subversion/libsvn_client/status.c
> trunk/subversion/svn/status-cmd.c
>
> Modified: trunk/subversion/include/svn_client.h
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/include/svn_client.h?pathrev=35512&r1=35511&r2=35512
> ==============================================================================
> --- trunk/subversion/include/svn_client.h Tue Jan 27 14:15:38 2009 (r35511)
> +++ trunk/subversion/include/svn_client.h Tue Jan 27 14:23:31 2009 (r35512)
> @@ -1784,6 +1784,46 @@ svn_client_commit(svn_client_commit_info
> */
>
> /**
> + * A structure to optional arguments for svn_client_status4(). It can grow as
> + * needed to avoid rev'ing the API. Never allocate this structure directly,
> + * as its size may change in future versions of Subversion. Use
> + * svn_client_status_args_create() instead.
> + *
> + * @since New in 1.6.
> + */
> +typedef struct svn_client_status_args_t
> +{
> + /** If set, retrieve all entries; otherwise, retrieve only
> + "interesting" entries (local mods and/or out of date). */
> + svn_boolean_t get_all;
> +
> + /** If set, also retrieve ignored files and directories. */
> + svn_boolean_t no_ignore;
> +
> + /** If not set, then recurse into externals definitions (if any exist)
> + after handling the main target. This calls the client notification
> + function (in @a ctx) with the @c svn_wc_notify_status_external action
> + before handling each externals definition, and with
> + @c svn_wc_notify_status_completed after each */
> + svn_boolean_t ignore_externals;
> +
> + /* Add new members here, and update svn_client_status_args_create(). */
> +} svn_client_status_args_t;
> +
> +/**
> + * Create a @c svn_client_status_args_t structure, for use with
> + * svn_client_status4().
> + * Values of structure members are as follows:
> + * @c get_all: FALSE
> + * @c no_ignore: FALSE
> + * @c ignore_externals: FALSE
> + *
> + * @since New in 1.6.
> + */
> +svn_client_status_args_t *
> +svn_client_status_args_create(apr_pool_t *pool);
> +
> +/**
> * Given @a path to a working copy directory (or single file), call
> * @a status_func/status_baton with a set of @c svn_wc_status_t *
> * structures which describe the status of @a path, and its children
> @@ -1800,13 +1840,6 @@ svn_client_commit(svn_client_commit_info
> * working copy was compared (@a *result_rev is not meaningful unless
> * @a update is set).
> *
> - * If @a ignore_externals is not set, then recurse into externals
> - * definitions (if any exist) after handling the main target. This
> - * calls the client notification function (in @a ctx) with the @c
> - * svn_wc_notify_status_external action before handling each externals
> - * definition, and with @c svn_wc_notify_status_completed
> - * after each.
> - *
> * @a changelists is an array of <tt>const char *</tt> changelist
> * names, used as a restrictive filter on items whose statuses are
> * reported; that is, don't report status about any item unless
> @@ -1822,10 +1855,8 @@ svn_client_status4(svn_revnum_t *result_
> svn_wc_status_func3_t status_func,
> void *status_baton,
> svn_depth_t depth,
> - svn_boolean_t get_all,
> svn_boolean_t update,
> - svn_boolean_t no_ignore,
> - svn_boolean_t ignore_externals,
> + svn_client_status_args_t *args,
> const apr_array_header_t *changelists,
> svn_client_ctx_t *ctx,
> apr_pool_t *pool);
>
> Modified: trunk/subversion/libsvn_client/client.h
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_client/client.h?pathrev=35512&r1=35511&r2=35512
> ==============================================================================
> --- trunk/subversion/libsvn_client/client.h Tue Jan 27 14:15:38 2009 (r35511)
> +++ trunk/subversion/libsvn_client/client.h Tue Jan 27 14:23:31 2009 (r35512)
> @@ -1048,9 +1048,8 @@ svn_client__do_external_status(svn_wc_tr
> svn_wc_status_func3_t status_func,
> void *status_baton,
> svn_depth_t depth,
> - svn_boolean_t get_all,
> svn_boolean_t update,
> - svn_boolean_t no_ignore,
> + svn_client_status_args_t *args,
> svn_client_ctx_t *ctx,
> apr_pool_t *pool);
>
>
> Modified: trunk/subversion/libsvn_client/delete.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_client/delete.c?pathrev=35512&r1=35511&r2=35512
> ==============================================================================
> --- trunk/subversion/libsvn_client/delete.c Tue Jan 27 14:15:38 2009 (r35511)
> +++ trunk/subversion/libsvn_client/delete.c Tue Jan 27 14:23:31 2009 (r35512)
> @@ -76,7 +76,10 @@ svn_client__can_delete(const char *path,
> apr_pool_t *pool)
> {
> svn_opt_revision_t revision;
> + svn_client_status_args_t *args;
> +
> revision.kind = svn_opt_revision_unspecified;
> + args = svn_client_status_args_create(pool);
>
> /* Use an infinite-depth status check to see if there's anything in
> or under PATH which would make it unsafe for deletion. The
> @@ -85,7 +88,7 @@ svn_client__can_delete(const char *path,
> be deleted. */
> return svn_client_status4
> (NULL, path, &revision, find_undeletables, NULL,
> - svn_depth_infinity, FALSE, FALSE, FALSE, FALSE, NULL, ctx, pool);
> + svn_depth_infinity, FALSE, args, NULL, ctx, pool);
> }
>
>
>
> Modified: trunk/subversion/libsvn_client/deprecated.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_client/deprecated.c?pathrev=35512&r1=35511&r2=35512
> ==============================================================================
> --- trunk/subversion/libsvn_client/deprecated.c Tue Jan 27 14:15:38 2009 (r35511)
> +++ trunk/subversion/libsvn_client/deprecated.c Tue Jan 27 14:23:31 2009 (r35512)
> @@ -1296,10 +1296,14 @@ svn_client_status3(svn_revnum_t *result_
> apr_pool_t *pool)
> {
> struct status3_wrapper_baton swb = { status_func, status_baton };
> + svn_client_status_args_t* args = svn_client_status_args_create(pool);
> +
> + args->get_all = get_all;
> + args->no_ignore = no_ignore;
> + args->ignore_externals = ignore_externals;
>
> return svn_client_status4(result_rev, path, revision, status3_wrapper_func,
> - &swb, depth, get_all, update, no_ignore,
> - ignore_externals, changelists, ctx, pool);
> + &swb, depth, update, args, changelists, ctx, pool);
>
> }
>
>
> Modified: trunk/subversion/libsvn_client/externals.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_client/externals.c?pathrev=35512&r1=35511&r2=35512
> ==============================================================================
> --- trunk/subversion/libsvn_client/externals.c Tue Jan 27 14:15:38 2009 (r35511)
> +++ trunk/subversion/libsvn_client/externals.c Tue Jan 27 14:23:31 2009 (r35512)
> @@ -1279,9 +1279,8 @@ svn_client__do_external_status(svn_wc_tr
> svn_wc_status_func3_t status_func,
> void *status_baton,
> svn_depth_t depth,
> - svn_boolean_t get_all,
> svn_boolean_t update,
> - svn_boolean_t no_ignore,
> + svn_client_status_args_t *args,
> svn_client_ctx_t *ctx,
> apr_pool_t *pool)
> {
> @@ -1351,8 +1350,8 @@ svn_client__do_external_status(svn_wc_tr
> SVN_ERR(svn_client_status4(NULL, fullpath,
> &(external->revision),
> status_func, status_baton,
> - depth, get_all, update,
> - no_ignore, FALSE, NULL, ctx, iterpool));
> + depth, update, args,
> + NULL, ctx, iterpool));
> }
> }
>
>
> Modified: trunk/subversion/libsvn_client/status.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_client/status.c?pathrev=35512&r1=35511&r2=35512
> ==============================================================================
> --- trunk/subversion/libsvn_client/status.c Tue Jan 27 14:15:38 2009 (r35511)
> +++ trunk/subversion/libsvn_client/status.c Tue Jan 27 14:23:31 2009 (r35512)
> @@ -213,10 +213,8 @@ svn_client_status4(svn_revnum_t *result_
> svn_wc_status_func3_t status_func,
> void *status_baton,
> svn_depth_t depth,
> - svn_boolean_t get_all,
> svn_boolean_t update,
> - svn_boolean_t no_ignore,
> - svn_boolean_t ignore_externals,
> + svn_client_status_args_t *args,
> const apr_array_header_t *changelists,
> svn_client_ctx_t *ctx,
> apr_pool_t *pool)
> @@ -271,10 +269,10 @@ svn_client_status4(svn_revnum_t *result_
> SVN_ERR(svn_wc_get_default_ignores(&ignores, ctx->config, pool));
> SVN_ERR(svn_wc_get_status_editor4(&editor, &edit_baton, &set_locks_baton,
> &edit_revision, anchor_access, target,
> - depth, get_all, no_ignore, ignores,
> - tweak_status, &sb, ctx->cancel_func,
> - ctx->cancel_baton, traversal_info,
> - pool));
> + depth, args->get_all, args->no_ignore,
> + ignores, tweak_status, &sb,
> + ctx->cancel_func, ctx->cancel_baton,
> + traversal_info, pool));
>
> /* If we want to know about out-of-dateness, we crawl the working copy and
> let the RA layer drive the editor for real. Otherwise, we just close the
> @@ -399,10 +397,19 @@ svn_client_status4(svn_revnum_t *result_
> depth anyway, so the code will DTRT if we change the conditional
> in the future.
> */
> - if (SVN_DEPTH_IS_RECURSIVE(depth) && (! ignore_externals))
> + if (SVN_DEPTH_IS_RECURSIVE(depth) && (! args->ignore_externals))
> SVN_ERR(svn_client__do_external_status(traversal_info, status_func,
> - status_baton, depth, get_all,
> - update, no_ignore, ctx, pool));
> + status_baton, depth,
> + update, args, ctx, pool));
>
> return SVN_NO_ERROR;
> }
> +
> +svn_client_status_args_t *
> +svn_client_status_args_create(apr_pool_t *pool)
> +{
> + svn_client_status_args_t *args = apr_pcalloc(pool, sizeof(*args));
> +
> + return args;
> +}
> +
>
> Modified: trunk/subversion/svn/status-cmd.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/svn/status-cmd.c?pathrev=35512&r1=35511&r2=35512
> ==============================================================================
> --- trunk/subversion/svn/status-cmd.c Tue Jan 27 14:15:38 2009 (r35511)
> +++ trunk/subversion/svn/status-cmd.c Tue Jan 27 14:23:31 2009 (r35512)
> @@ -174,6 +174,7 @@ svn_cl__status(apr_getopt_t *os,
> int i;
> svn_opt_revision_t rev;
> struct status_baton sb;
> + svn_client_status_args_t *args;
>
> SVN_ERR(svn_cl__args_to_target_array_print_reserved(&targets, os,
> opt_state->targets,
> @@ -218,6 +219,11 @@ svn_cl__status(apr_getopt_t *os,
> sb.cached_changelists = master_cl_hash;
> sb.cl_pool = pool;
>
> + args = svn_client_status_args_create(pool);
> + args->get_all = opt_state->verbose;
> + args->no_ignore = opt_state->no_ignore;
> + args->ignore_externals = opt_state->ignore_externals;
> +
> for (i = 0; i < targets->nelts; i++)
> {
> const char *target = APR_ARRAY_IDX(targets, i, const char *);
> @@ -236,10 +242,8 @@ svn_cl__status(apr_getopt_t *os,
> SVN_ERR(svn_cl__try(svn_client_status4(&repos_rev, target, &rev,
> print_status, &sb,
> opt_state->depth,
> - opt_state->verbose,
> opt_state->update,
> - opt_state->no_ignore,
> - opt_state->ignore_externals,
> + args,
> opt_state->changelists,
> ctx, subpool),
> NULL, opt_state->quiet,
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=495&dsMessageId=1060547
>
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1060782
Received on 2009-01-28 00:25:49 CET