[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

RE: [RFC] Introducing svn_client_make_diff_args? (Was: RE: svn commit: r33116 - in branches/ignore-mergeinfo/subversion: include libsvn_client svn)

From: Bert Huijben <bert_at_vmoo.com>
Date: Wed, 17 Sep 2008 17:34:25 +0200

> -----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

This is an archived mail posted to the Subversion Dev mailing list.