You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Bert Huijben <be...@vmoo.com> on 2008/09/17 10:47:13 UTC
[RFC] Introducing svn_client_make_diff_args? (Was: RE: svn commit: r33116 - in branches/ignore-mergeinfo/subversion: include libsvn_client svn)
> @@ -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.
We do use argument structures in a few places in the subversion code (think user context, status, authorization, ...), but I think diff would be a good candidate to follow this construction. And there are probably a few other places that could benefit from such argument structures.
=====================
In SharpSvn I use the convention of args objects for most optional parameters, so I don't have to create another override for every future version of subversion that adds just another boolean parameter like this.
Using the old NSvn that followed the subversion arguments like most other bindings, you would use something like:
[[
Client.Diff("a", "b", diffStream, true, true, false, false, true);
]]
Completely unreadable without intellisense or documentation open.
While a SharpSvn user would write something like:
[[
SvnDiffArgs args = new SvnDiffArgs();
args.IgnoreContentType = true;
args.IgnoreMergeInfo = true;
Client.Diff("a", "b", diffStream, args);
]]
A little bit more code, but no need to set default values. I think everybody can understand it without looking at the documentation.
(Using C# 3.0 instead of 2.0 would allow you to use a more compact syntax)
Adding a new property to the args object does not break the ABI and does not require a new compatibility wrapper to maintain forever.
Bert
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [RFC] Introducing svn_client_make_diff_args? (Was: RE: svn
commit: r33116 - in branches/ignore-mergeinfo/subversion: include
libsvn_client svn)
Posted by Julian Foad <ju...@btopenworld.com>.
Bert Huijben wrote:
> > +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?
Yes, please!
> [[
> Client.Diff("a", "b", diffStream, true, true, false, false, true);
> ]]
> Completely unreadable without intellisense or documentation open.
>
> [[
> SvnDiffArgs args = new SvnDiffArgs();
> args.IgnoreContentType = true;
> args.IgnoreMergeInfo = true;
>
> Client.Diff("a", "b", diffStream, args);
> ]]
> Adding a new property to the args object does not break the ABI and
> does not require a new compatibility wrapper to maintain forever.
+1.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [RFC] Introducing svn_client_make_diff_args? (Was: RE:
svn commit: r33116 - in branches/ignore-mergeinfo/subversion:
include libsvn_client svn)
Posted by Julian Foad <ju...@btopenworld.com>.
On Wed, 2008-09-17 at 09:46 -0500, Hyrum K. Wright wrote:
> Julian Foad wrote:
[...]
> >> Bert, could you write the patch (against the ignore-mergeinfo branch, of course)
> >> for this?
> >
> > I think it would be sensible to do that on trunk. Although there's an
> > overlap, it's a logically independent change that can happen before or
> > after the ignore-mergeinfo changes.
>
> The reason I suggested it should happen on the branch is because such changes
> should probably happen if and when we rev and API for something else, i.e. let's
> not gratuitously rev the API just to add an arguments structure. We're already
> rev'ing this particular API on the ignore-mergeinfo branch, so it seems logical
> to do the work there.
>
> I've no real opposition to doing it on trunk, excepting the fact that merging
> back to the branch could be painful (but not impossible).
Fair arguments. Bert, your choice!
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [RFC] Introducing svn_client_make_diff_args? (Was: RE: svn commit:
r33116 - in branches/ignore-mergeinfo/subversion: include libsvn_client svn)
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Julian Foad wrote:
> Hyrum K. Wright wrote:
>> Bert Huijben wrote:
>>> 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 think it would be sensible to do that on trunk. Although there's an
> overlap, it's a logically independent change that can happen before or
> after the ignore-mergeinfo changes.
The reason I suggested it should happen on the branch is because such changes
should probably happen if and when we rev and API for something else, i.e. let's
not gratuitously rev the API just to add an arguments structure. We're already
rev'ing this particular API on the ignore-mergeinfo branch, so it seems logical
to do the work there.
I've no real opposition to doing it on trunk, excepting the fact that merging
back to the branch could be painful (but not impossible).
-Hyrum
Re: [RFC] Introducing svn_client_make_diff_args? (Was: RE: svn
commit: r33116 - in branches/ignore-mergeinfo/subversion: include
libsvn_client svn)
Posted by Julian Foad <ju...@btopenworld.com>.
Hyrum K. Wright wrote:
> Bert Huijben wrote:
> > 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 think it would be sensible to do that on trunk. Although there's an
overlap, it's a logically independent change that can happen before or
after the ignore-mergeinfo changes.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [RFC] Introducing svn_client_make_diff_args? (Was: RE: svn
commit: r33116 - in branches/ignore-mergeinfo/subversion: include libsvn_client
svn)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
> > "Defaulting to false" -> "Defaults to @c FALSE"
>
> Bikeshed.
+1, but:
I prefer "@c FALSE" because it is syntax highlighted.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [RFC] Introducing svn_client_make_diff_args? (Was: RE: svn
commit: r33116 - in branches/ignore-mergeinfo/subversion: include
libsvn_client svn)
Posted by Julian Foad <ju...@btopenworld.com>.
Hyrum K. Wright wrote:
> Bert Huijben wrote:
[...]
> > 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.
Yes.
> > *
> > * 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"
Bikeshed. Several of our committers seem to prefer "@c FALSE". I much
prefer "false" because it's easier on the eyes, it's not a requirement
that the caller use this particular preprocessor macro, and it's not a
definition that we need to call attention to or that the reader needs to
follow a cross-reference to.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [RFC] Introducing svn_client_make_diff_args? (Was: RE: svn commit:
r33116 - in branches/ignore-mergeinfo/subversion: include libsvn_client svn)
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
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@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
RE: [RFC] Introducing svn_client_make_diff_args? (Was: RE: svn commit: r33116 - in branches/ignore-mergeinfo/subversion: include libsvn_client svn)
Posted by Bert Huijben <be...@vmoo.com>.
> -----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@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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [RFC] Introducing svn_client_make_diff_args? (Was: RE: svn commit:
r33116 - in branches/ignore-mergeinfo/subversion: include libsvn_client svn)
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
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?
Thanks,
-Hyrum
> We do use argument structures in a few places in the subversion code (think user context, status, authorization, ...), but I think diff would be a good candidate to follow this construction. And there are probably a few other places that could benefit from such argument structures.
>
> =====================
> In SharpSvn I use the convention of args objects for most optional parameters, so I don't have to create another override for every future version of subversion that adds just another boolean parameter like this.
>
> Using the old NSvn that followed the subversion arguments like most other bindings, you would use something like:
> [[
> Client.Diff("a", "b", diffStream, true, true, false, false, true);
> ]]
> Completely unreadable without intellisense or documentation open.
>
>
> While a SharpSvn user would write something like:
>
> [[
> SvnDiffArgs args = new SvnDiffArgs();
> args.IgnoreContentType = true;
> args.IgnoreMergeInfo = true;
>
> Client.Diff("a", "b", diffStream, args);
> ]]
>
> A little bit more code, but no need to set default values. I think everybody can understand it without looking at the documentation.
>
> (Using C# 3.0 instead of 2.0 would allow you to use a more compact syntax)
>
>
> Adding a new property to the args object does not break the ABI and does not require a new compatibility wrapper to maintain forever.