> -----Original Message-----
> From: Hyrum K. Wright [mailto:hyrum_wright{a}mail.utexas.edu]
> Sent: woensdag 17 september 2008 15:36
> To: Bert Huijben
> Cc: dev_at_subversion.tigris.org
> Subject: Re: [RFC] Introducing svn_client_make_diff_args? (Was: RE: svn
> commit: r33116 - in branches/ignore-mergeinfo/subversion: include
> libsvn_client svn)
>
> Bert Huijben wrote:
> >> @@ -2225,8 +2228,36 @@ svn_client_blame(const char *path_or_url
> >> * @note @a relative_to_dir doesn't affect the path index generated
> by
> >> * external diff programs.
> >> *
> >> + * @since New in 1.6.
> >> + */
> >> +svn_error_t *
> >> +svn_client_diff5(const apr_array_header_t *diff_options,
> >> + const char *path1,
> >> + const svn_opt_revision_t *revision1,
> >> + const char *path2,
> >> + const svn_opt_revision_t *revision2,
> >> + const char *relative_to_dir,
> >> + svn_depth_t depth,
> >> + svn_boolean_t ignore_ancestry,
> >> + svn_boolean_t no_diff_deleted,
> >> + svn_boolean_t ignore_content_type,
> >> + svn_boolean_t ignore_mergeinfo,
> >> + const char *header_encoding,
> >> + apr_file_t *outfile,
> >> + apr_file_t *errfile,
> >> + const apr_array_header_t *changelists,
> >> + svn_client_ctx_t *ctx,
> >> + apr_pool_t *pool);
> >
> > This method has 5 different versions over 7 subversion point
> releases, each adding just on or a few 'optional' parameters.
> >
> > Shouldn't we create some kind of context/args object that we can
> extend in future versions for methods that probably have yet another
> version in the next release?
> >
> > Adding boolean number 4 to a function doesn't make user code more
> understandable, and with a context/args struct with a constructor
> function we can just extend that structure in the next release without
> breaking the ABI.
>
> I thought about this same issue when extending the status API on the
> ignore-mergeinfo branch. I suspect there are several APIs we could do
> this too
> next time we rev them. If we identify those, we should put a note in
> their
> comments.
>
> Bert, could you write the patch (against the ignore-mergeinfo branch,
> of course)
> for this?
I will write a patch for this.
Then on to the bikeshed:
How should we call these structs, if we are going to use them as a new standard pattern?
(Once answered the result should probably be added to hacking.html before we release 1.6)
Just some partial example:
/** Argument container for @c svn_client_diff5
*
* In order to avoid backwards compatibility problems clients should
* use @ svn_client_diff_args_create() to allocate and initialize this
* structure instead of doing so themselves.
*
* @since New in 1.6.
*/
typedef struct svn_client_diff_args_t
{
/** Don't include any merge info in the diff output. Defaulting to
* false for backwards compatibility. */
svn_boolean_t ignore_merge_info;
// <snip>
} svn_client_diff_args_t;
/** Allocate and initialize a diff argument container in @a pool.
*
* @since New in 1.6.
*/
svn_client_diff_args_t *
svn_client_diff_args_create(apr_pool_t *pool);
/**
* Produce diff output which describes the delta between
* @a path1/@a revision1 and @a path2/@a revision2. Print the output
* of the diff to @a outfile. @a path1 and @a path2 can be either
* working-copy paths or URLs.
*
<snip>
* @since New in 1.6.
*/
svn_error_t *
svn_client_diff5(const char *path1,
const svn_opt_revision_t *revision1,
const char *path2,
const svn_opt_revision_t *revision2,
apr_file_t *outfile,
const svn_client_diff_args_t* args,
svn_client_ctx_t *ctx,
apr_pool_t *pool);
I don't think args creators should ever error return an error. (The only valid error would be out of memory. Returning a svn_error_t* is impossible in that case).
I like the idea of keeping the 'primary/required' parameters on the method and only moving the 'optional' ones to the args object. That way you won't forget to add a required value.
For SharpSvn I defaulted to: What must you pass to the cli to get a minimalistic useful answer. (most defaults follow the cli)
Before I start coding:
Is there something we can change on this design to make it easier for the client bindings (swig/python?) to make it easier to use this solution there?
Thanks,
Bert
>
> Thanks,
> -Hyrum
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-09-17 17:34:38 CEST