You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Malcolm Rowe <ma...@farside.org.uk> on 2006/06/30 16:36:14 UTC
Re: svn commit: r20307 - in trunk/subversion: include libsvn_client libsvn_wc svn tests/cmdline/getopt_tests_data
On Fri, Jun 30, 2006 at 06:28:22AM -0700, sussman@tigris.org wrote:
> Merge changelist-feature branch to trunk.
Wheeeeeeeeeeeeeeeeeeee! Drive-by review follows:
> --- trunk/subversion/include/svn_client.h (original)
> +++ trunk/subversion/include/svn_client.h Fri Jun 30 06:28:20 2006
> @@ -1077,6 +1081,22 @@
> * @since New in 1.3.
New in 1.5.
> */
> svn_error_t *
> +svn_client_commit4(svn_commit_info_t **commit_info_p,
> + const apr_array_header_t *targets,
> + svn_boolean_t recurse,
> + svn_boolean_t keep_locks,
> + const char *changelist_name,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool);
> +
> +/** Similar to svn_client_commit4(), but always passes NULL @a
> + * changelist_name.
> + *
> + * @deprecated Provided for backward compatibility with the 1.3 API.
1.4 API.
> + *
> + * @since New in 1.3.
> + */
> +svn_error_t *
> svn_client_commit3(svn_commit_info_t **commit_info_p,
> const apr_array_header_t *targets,
> svn_boolean_t recurse,
> @@ -2455,6 +2475,44 @@
> svn_client_ctx_t *ctx,
> apr_pool_t *pool);
>
> +
> +/**
> + * Associate @a path with changelist @a changelist. If CLEAR is set,
> + * then ignore @a changelist and deassociate any existing changelist
> + * from @a path.
CLEAR should be in Doxygen style. But why have a separate variable at
all - why not just take a NULL changelist to be a 'clear' operation?
(I'm not disagreeing that the UI shouldn't use 'svn changelist --clear',
but the API has no need to follow that pattern).
This comment should make it clear what happens if you try to associate
a changelist with a path that's already associated with a changelist -
is it additive, or does it replace the changelist associated with it?
> + *
> + * Note: this metadata is purely a client-side "bookkeeping"
> + * convenience, and is entirely managed by the working copy.
> + *
> + * @since New in 1.5.
> + */
> +svn_error_t *
> +svn_client_changelist(const char *path,
I can't say I'm too happy about the name - 'changelist' isn't a verb,
the UI command notwithstanding.
We have 'retrieve_changelist' elsewhere - why not something like
set_changelist for associating paths? (Oh, and how _do_ you find out
programmatically what changelist (changelists?) are associated with a
given path - ah, svn_client_info?)
Do we need a new version of svn_client_info() that guarantees to call
the receiver with a version of the svn_info_t structure that includes
'changelist'? Or do we assume that if the caller wants to access that
member, they should be clever enough to check that the current runtime
library version is >= 1.5? (via, I guess, something like
svn_subr_version()).
> --- trunk/subversion/include/svn_wc.h (original)
> +++ trunk/subversion/include/svn_wc.h Fri Jun 30 06:28:20 2006
> @@ -3461,6 +3468,24 @@
> void *cancel_baton,
> apr_pool_t *pool);
>
> +
> +/**
> + * Associate @a path with changelist @a changelist by setting the
> + * 'changelist' attribute in its entry. If @a clear is set, then
> + * ignore @a changelist and remove any 'changelist' attribute in @a
> + * path's entry.
> + *
> + * Note: this metadata is purely a client-side "bookkeeping"
> + * convenience, and is entirely managed by the working copy.
> + *
> + * @since New in 1.5.
> + */
> +svn_error_t *
> +svn_wc_changelist(const char *path,
> + const char *changelist,
> + svn_boolean_t clear,
Similar question as to why we don't allow a NULL changelist to indicate
a disassociation.
> svn_error_t *
> +svn_wc__loggy_delete_changelist(svn_stringbuf_t **log_accum,
> + svn_wc_adm_access_t *adm_access,
> + const char *path,
> + apr_pool_t *pool)
Yeah, we really should have some way to clear wc attributes (locks,
changelists) without having to create separate log operations for each.
> --- trunk/subversion/svn/main.c (original)
> +++ trunk/subversion/svn/main.c Fri Jun 30 06:28:20 2006
> @@ -1169,6 +1180,11 @@
> case svn_cl__summarize:
> opt_state.summarize = TRUE;
> break;
> + case svn_cl__clear_opt:
> + opt_state.clear = TRUE;
break;
> + case svn_cl__changelist_opt:
> + opt_state.changelist = apr_pstrdup(pool, opt_arg);
> + break;
> default:
> /* Hmmm. Perhaps this would be a good place to squirrel away
> opts that commands like svn diff might need. Hmmm indeed. */
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r20307 - in trunk/subversion: include libsvn_client libsvn_wc svn tests/cmdline/getopt_tests_data
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Sun, Jul 02, 2006 at 06:43:49AM -0500, Ben Collins-Sussman wrote:
> >(Oh, and how _do_ you find out
> >programmatically what changelist (changelists?) are associated with a
> >given path - ah, svn_client_info?)
>
> A path can only be associated with one changelist. To find out
> programmatically, just look at entry->changelist. To find out as a
> user, use 'svn info' to look at the entry. (Or use 'svn status' to
> see the whole changelist.)
>
Yeah, I figured that out as I kept reading, but it might be a good idea
to xref the programmatic way in the doc-comments (though I've not re-read
them - they may be clearer now). Certainly, providing a set_changelist()
function without a get_changelist() equivalent is odd enough that I'd
expect some kind of explanation.
> >
> >Do we need a new version of svn_client_info() that guarantees to call
> >the receiver with a version of the svn_info_t structure that includes
> >'changelist'? Or do we assume that if the caller wants to access that
> >member, they should be clever enough to check that the current runtime
> >library version is >= 1.5? (via, I guess, something like
> >svn_subr_version()).
>
> I'm not aware of any applications that check the version of
> libsvn_client at runtime. Typically a new version of the application
> is released, and it simply *requires* libsvn_cllient 1.5 at
> compile-time, end of story. My impression is that the only time apps
> need to start worrying about run-time version checks are for loaded RA
> modules, where it really is possible to stumble across an old library
> during dlopen().
>
> So I think that even though svn_info_t is a public, transparent
> structure, I've done the usual recommended practice of just adding a
> new field to the end of it. Isn't that how we've always done things
> for non-RA structures?
>
Thinking about it some more, yes, I think what we currently have is fine.
The only reason I was a little worried is because unlike the normal
situation - where the new symbols won't exist in the old library - it's
perfectly possible to run a compiled-with-1.5 caller of svn_client_info
on a 1.4 library, and the only difference is that the ->changelist member
is complete junk.
But I don't think the potential for an uncautious developer making this
mistake is worth revving the API - there isn't really any way to make
it clearer other than writing "This field of the structure will be junk
in pre-1.5 clients", and that seem to me to be over the top.
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r20307 - in trunk/subversion: include libsvn_client libsvn_wc svn tests/cmdline/getopt_tests_data
Posted by Ben Collins-Sussman <su...@red-bean.com>.
On 6/30/06, Malcolm Rowe <ma...@farside.org.uk> wrote:
> On Fri, Jun 30, 2006 at 06:28:22AM -0700, sussman@tigris.org wrote:
> > Merge changelist-feature branch to trunk.
>
> Wheeeeeeeeeeeeeeeeeeee! Drive-by review follows:
>
> > --- trunk/subversion/include/svn_client.h (original)
> > +++ trunk/subversion/include/svn_client.h Fri Jun 30 06:28:20 2006
> > @@ -1077,6 +1081,22 @@
> > * @since New in 1.3.
>
> New in 1.5.
>
Fixed.
> > */
> > svn_error_t *
> > +svn_client_commit4(svn_commit_info_t **commit_info_p,
> > + const apr_array_header_t *targets,
> > + svn_boolean_t recurse,
> > + svn_boolean_t keep_locks,
> > + const char *changelist_name,
> > + svn_client_ctx_t *ctx,
> > + apr_pool_t *pool);
> > +
> > +/** Similar to svn_client_commit4(), but always passes NULL @a
> > + * changelist_name.
> > + *
> > + * @deprecated Provided for backward compatibility with the 1.3 API.
>
> 1.4 API.
>
Fixed.
> > + *
> > + * @since New in 1.3.
> > + */
> > +svn_error_t *
> > svn_client_commit3(svn_commit_info_t **commit_info_p,
> > const apr_array_header_t *targets,
> > svn_boolean_t recurse,
> > @@ -2455,6 +2475,44 @@
> > svn_client_ctx_t *ctx,
> > apr_pool_t *pool);
> >
> > +
> > +/**
> > + * Associate @a path with changelist @a changelist. If CLEAR is set,
> > + * then ignore @a changelist and deassociate any existing changelist
> > + * from @a path.
>
> CLEAR should be in Doxygen style. But why have a separate variable at
> all - why not just take a NULL changelist to be a 'clear' operation?
> (I'm not disagreeing that the UI shouldn't use 'svn changelist --clear',
> but the API has no need to follow that pattern).
Hm, that may be a friendier API, yes. I've updated both
svn_wc_changelist() and svn_client_changelist() to be this way.
>
> This comment should make it clear what happens if you try to associate
> a changelist with a path that's already associated with a changelist -
> is it additive, or does it replace the changelist associated with it?
>
Fixed docstrings.
> > + *
> > + * Note: this metadata is purely a client-side "bookkeeping"
> > + * convenience, and is entirely managed by the working copy.
> > + *
> > + * @since New in 1.5.
> > + */
> > +svn_error_t *
> > +svn_client_changelist(const char *path,
>
> I can't say I'm too happy about the name - 'changelist' isn't a verb,
> the UI command notwithstanding.
>
> We have 'retrieve_changelist' elsewhere - why not something like
> set_changelist for associating paths?
Good idea. I've changed the APIs to 'set_changelist'.
> (Oh, and how _do_ you find out
> programmatically what changelist (changelists?) are associated with a
> given path - ah, svn_client_info?)
A path can only be associated with one changelist. To find out
programmatically, just look at entry->changelist. To find out as a
user, use 'svn info' to look at the entry. (Or use 'svn status' to
see the whole changelist.)
>
> Do we need a new version of svn_client_info() that guarantees to call
> the receiver with a version of the svn_info_t structure that includes
> 'changelist'? Or do we assume that if the caller wants to access that
> member, they should be clever enough to check that the current runtime
> library version is >= 1.5? (via, I guess, something like
> svn_subr_version()).
I'm not aware of any applications that check the version of
libsvn_client at runtime. Typically a new version of the application
is released, and it simply *requires* libsvn_cllient 1.5 at
compile-time, end of story. My impression is that the only time apps
need to start worrying about run-time version checks are for loaded RA
modules, where it really is possible to stumble across an old library
during dlopen().
So I think that even though svn_info_t is a public, transparent
structure, I've done the usual recommended practice of just adding a
new field to the end of it. Isn't that how we've always done things
for non-RA structures?
>
> > --- trunk/subversion/include/svn_wc.h (original)
> > +++ trunk/subversion/include/svn_wc.h Fri Jun 30 06:28:20 2006
> > @@ -3461,6 +3468,24 @@
> > void *cancel_baton,
> > apr_pool_t *pool);
> >
> > +
> > +/**
> > + * Associate @a path with changelist @a changelist by setting the
> > + * 'changelist' attribute in its entry. If @a clear is set, then
> > + * ignore @a changelist and remove any 'changelist' attribute in @a
> > + * path's entry.
> > + *
> > + * Note: this metadata is purely a client-side "bookkeeping"
> > + * convenience, and is entirely managed by the working copy.
> > + *
> > + * @since New in 1.5.
> > + */
> > +svn_error_t *
> > +svn_wc_changelist(const char *path,
> > + const char *changelist,
> > + svn_boolean_t clear,
>
> Similar question as to why we don't allow a NULL changelist to indicate
> a disassociation.
>
Fixed.
>
> > svn_error_t *
> > +svn_wc__loggy_delete_changelist(svn_stringbuf_t **log_accum,
> > + svn_wc_adm_access_t *adm_access,
> > + const char *path,
> > + apr_pool_t *pool)
>
> Yeah, we really should have some way to clear wc attributes (locks,
> changelists) without having to create separate log operations for each.
>
Yes we should!
> > --- trunk/subversion/svn/main.c (original)
> > +++ trunk/subversion/svn/main.c Fri Jun 30 06:28:20 2006
> > @@ -1169,6 +1180,11 @@
> > case svn_cl__summarize:
> > opt_state.summarize = TRUE;
> > break;
> > + case svn_cl__clear_opt:
> > + opt_state.clear = TRUE;
>
> break;
>
Fixed!
Thanks for the review!
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org