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

Thanks!

> 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

"@c svn_client_diff5" -> "svn_client_diff5()"
Doxygen will find the function if you postpend the parenthesis.

> *
> * 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. */

"Defaulting to false" -> "Defaults to @c FALSE"

> svn_boolean_t ignore_merge_info;
>
> // <snip>

Add a note here about always adding new members at the end of the struct, and to
be sure to update the constructor when doing so.

> } 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).

Agreed. That would also follow our typical constructor pattern.

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

+1 The implementation probably has some ideas of which are required parameters,
and which are optional.

Generally, the interface looks good. Usually when we have constructors, we also
implement duplicator functions, but I'm not sure we need them in this case.

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

I'm not familiar enough with the bindings mechanisms to comment on this.

-Hyrum