On 6/30/06, Malcolm Rowe <malcolm-svn-dev@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
Received on Sun Jul 2 13:44:25 2006