You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@gmail.com> on 2009/01/27 23:25:30 UTC
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.
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 <rh...@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
RE: svn commit: r35512 - in trunk/subversion: include libsvn_client
svn
Posted by Bert Huijben <rh...@sharpsvn.net>.
> -----Original Message-----
> From: Bert Huijben [mailto:rhuijben@sharpsvn.net]
> Sent: woensdag 28 januari 2009 17:15
> To: 'C. Michael Pilato'; 'Bert Huijben'
> Cc: 'Greg Stein'; dev@subversion.tigris.org
> Subject: RE: svn commit: r35512 - in trunk/subversion: include
> libsvn_client svn
> Which others would you like to revert too?
> Feel free to revert..
>
> svn_client_status4: r35518,r35515-r35514,r35512
> svn_client_log5: r35242,r35245
I added a non blocking note to TODO-1.6 with the 4 options of which we
should choose one before branching 1.6:
* revert the svn_client_status set
* revert the svn_client_status and svn_client_log set under the assumption
that we just create new functions later for both cases.
And the last options:
* switch to bitflags for the booleans. (I don't think we use bitflags
anywhere, but these have the same documentation advantage).
* move more arguments to svn_client_status_args_t to make the struct more
usefull than just keeping 3 booleans.
I would like some response in this thread from others before just choosing
the first (status only revert) option myself. Julian and Hyrum were +1 on
the initial ideas and Hyrum introduced svn_client_log_args_t.
Bert
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1069120
RE: svn commit: r35512 - in trunk/subversion: include libsvn_client
svn
Posted by Bert Huijben <rh...@sharpsvn.net>.
> -----Original Message-----
> From: C. Michael Pilato [mailto:cmpilato@collab.net]
> Sent: woensdag 28 januari 2009 16:16
> To: Bert Huijben
> Cc: 'Greg Stein'; dev@subversion.tigris.org
> Subject: Re: svn commit: r35512 - in trunk/subversion: include
> libsvn_client svn
>
> Bert Huijben wrote:
> >> -----Original Message-----
> >> From: Greg Stein [mailto:gstein@gmail.com]
> >> Sent: Wednesday, January 28, 2009 12:26 AM
> >> To: dev@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, I gotta say I that I, too, disapprove of this change, and
> absolutely
> do not want to go down this path. The closest to this that I would
> agree to
> go would be to combine all the booleans into a field of bitflags. But
> generic structures of function args are, as Greg as noted, just a lot
> of
> pain for precious little gain.
>
> As for svn_client_status99() ... that's a tad over-dramatic, don't you
> think?
They give an initial pain, but they document exactly what the users ask from an API (Out of your memory: What is the third boolean passed to svn_client_status3 ?) and will need no further updating later on when new options are added that have a logical default.
(In 99% of the cases the logical default is just what svn would do without any arguments.. as you don't have to think twice about that default)
I just came to 74 SVN_DEPRECATED methods in svn_client.h, most of which could have been avoided by providing a little bit of extensibility via either a bitflags parameter or an args object. (35% of the file is filled with deprecated methods.. and this is of course after trimming their comments when rev'ving).
And svn_status is now the third/fourth method to provide an args object. With three options (I decided not to add the update argument, (I did add it to the args object in SharpSvn).. but most depth parameters are on the args object there too).
Which others would you like to revert too?
Feel free to revert..
svn_client_status4: r35518,r35515-r35514,r35512
svn_client_log5: r35242,r35245
Adding args objects is not the normal way to code for extensibility in C, but for .Net libraries it is explicitly documented as the way to design libraries for future extensibility and it as added benefit it works to get better readable/maintainable code. (Google can certainly give you some references to the class library design guidelines and the reasoning behind this and related rules).
It's better maintainable than default values on functions calls, as used in many programming languages; as you can never change the default value of these default values. (E.g. In C++ the default is compiled in at the caller; so it becomes part of your ABI)
Bert
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1063558
Re: svn commit: r35512 - in trunk/subversion: include libsvn_client
svn
Posted by Greg Stein <gs...@gmail.com>.
On Thu, Jan 29, 2009 at 16:14, Bert Huijben <rh...@sharpsvn.net> wrote:
>> -----Original Message-----
>> From: Bert Huijben [mailto:rhuijben@sharpsvn.net]
>> Sent: woensdag 28 januari 2009 17:15
>> To: 'C. Michael Pilato'; 'Bert Huijben'
>> Cc: 'Greg Stein'; dev@subversion.tigris.org
>> Subject: RE: svn commit: r35512 - in trunk/subversion: include
>> libsvn_client svn
>
>> Which others would you like to revert too?
>> Feel free to revert..
>>
>> svn_client_status4: r35518,r35515-r35514,r35512
>> svn_client_log5: r35242,r35245
>
> I added a non blocking note to TODO-1.6 with the 4 options of which we
> should choose one before branching 1.6:
> * revert the svn_client_status set
> * revert the svn_client_status and svn_client_log set under the assumption
> that we just create new functions later for both cases.
Ugh. I missed that one. Just as bad.
> And the last options:
> * switch to bitflags for the booleans. (I don't think we use bitflags
> anywhere, but these have the same documentation advantage).
We do, but they suck. Using a flag to create wildly variant behavior
is just nonsense. There should be *two* functions rather than one with
two behaviors.
> * move more arguments to svn_client_status_args_t to make the struct more
> usefull than just keeping 3 booleans.
>
> I would like some response in this thread from others before just choosing
> the first (status only revert) option myself. Julian and Hyrum were +1 on
> the initial ideas and Hyrum introduced svn_client_log_args_t.
Alright, but I maintain that all you're doing is making things more
complicated. You are NOT reducing the number of arguments to the
function. You're simply pushing those arguments off to an entirely
different parameter-passing mechanism. So now you have "some as
parameters" and "some buried in a struct, passed as a parameter." But
you're not reducing the total count, and certainly not simplifying.
Adding new parameters to functions (and creating new APIs) is simply
part of the evolution of the software. It is always going to happen.
Callers will always need to be aware of the new parameters -- they
were introduced *for a reason*. Shoving them off into a structure is
not going to magically make clients smarter. The structure is not
going to change the fact that clients will need to learn about the new
"parameters" and adjust their code to take advantage of them. And this
"future proofing" does nothing for *removing* parameters, like I did
in many of the svn_io.h functions.
Adding another API is not a problem. Clients can continue to use the
old APIs, so new ones have no impact on them. I don't see any stigma
or issue or other problems with adding new APIs. What *problem* are we
solving by trying to prevent svn_client_status5() and
svn_client_log6() from being introduced at some point in the future?
Why is it a problem if we add those APIs? How do we benefit by *not*
adding those APIs?
IOW, I'm seeing a "solution" for a "problem" that I don't fully understand.
You complained about the size and amount of deprecated functions. I
offered a solution for that a while back (move them to a separate
header), but people didn't like that. Said they wanted the old calls
right there next to the "proper ones to use". Maybe that idea can be
revisited. I certainly wouldn't mind culling deprecated stuff out of
the headers, shoving them all into svn_deprecated.h.
Cheers,
-g
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1069158
Re: svn commit: r35512 - in trunk/subversion: include libsvn_client
svn
Posted by "C. Michael Pilato" <cm...@collab.net>.
Bert Huijben wrote:
>> -----Original Message-----
>> From: Greg Stein [mailto:gstein@gmail.com]
>> Sent: Wednesday, January 28, 2009 12:26 AM
>> To: dev@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, I gotta say I that I, too, disapprove of this change, and absolutely
do not want to go down this path. The closest to this that I would agree to
go would be to combine all the booleans into a field of bitflags. But
generic structures of function args are, as Greg as noted, just a lot of
pain for precious little gain.
As for svn_client_status99() ... that's a tad over-dramatic, don't you think?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1063414
RE: svn commit: r35512 - in trunk/subversion: include libsvn_client
svn
Posted by Bert Huijben <rh...@sharpsvn.net>.
> -----Original Message-----
> From: Greg Stein [mailto:gstein@gmail.com]
> Sent: Wednesday, January 28, 2009 12:26 AM
> To: dev@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 <rh...@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