> -----Original Message-----
> From: Greg Stein [mailto:gstein_at_gmail.com]
> Sent: Wednesday, January 28, 2009 12:26 AM
> To: dev_at_subversion.tigris.org
> Subject: Re: svn commit: r35512 - in trunk/subversion: include
> libsvn_client svn
>
> 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.
Hi,
This specific example was discussed on this list a few months ago as a
specific example when to introduce an args structure. The work just wasn't
completed before.
See: [RFC] Introducing svn_client_make_diff_args? (Was: ...)
http://svn.haxx.se/dev/archive-2008-09/0674.shtml (Google reference, but url
is down right now)
We are on svn_client_status4 right now, with only 6 point releases,
svn_client_log5, svn_client_diff5.. Let's go to svn_client_status99 then and
let all 98 others be deprecated but be part of our code before 2.0.
Providing functions with dozens of Boolean arguments doesn't give us a clean
api. Using an args object with a good set of defaults makes the choices
explicit in code and commit logs without referring to the help (or
intellisense of your favorite editor) to see what the seventh Boolean is
about.
See also the forwarding of the args inside the status call to the next call
for the externals. It reduced extra arguments there again.
Bert
>
> 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/depreca
> ted.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/externa
> ls.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&dsMessageI
> d=1060547
> >
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageI
> d=1060782
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1061023
Received on 2009-01-28 01:44:47 CET