Re: svn commit: r20307 - in trunk/subversion: include libsvn_client libsvn_wc svn tests/cmdline/getopt_tests_data

[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

Re: svn commit: r20307 - in trunk/subversion: include libsvn_client libsvn_wc svn tests/cmdline/getopt_tests_data

[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