[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r923720 - in /subversion/trunk/subversion/libsvn_client: client.h diff.c repos_diff.c

From: Greg Stein <gstein_at_gmail.com>
Date: Tue, 16 Mar 2010 12:31:50 -0400

On Tue, Mar 16, 2010 at 09:07, <philip_at_apache.org> wrote:
>...
> +++ subversion/trunk/subversion/libsvn_client/client.h Tue Mar 16 13:07:40 2010
> @@ -655,7 +655,8 @@ svn_client__switch_internal(svn_revnum_t
>    TARGET is a working-copy path, the base of the hierarchy to be
>    compared.  It corresponds to the URL opened in RA_SESSION below.
>
> -   WC_CTX is a context for the working copy.
> +   WC_CTX is a context for the working copy and should be NULL for
> +   operations that do not involve a working copy.

This kind of comment should be added to all functions where this is
allowed. Normally wc_ctx!=NULL throughout libsvn_client, so it is a
very different departure to allow this.

>...
> +++ subversion/trunk/subversion/libsvn_client/repos_diff.c Tue Mar 16 13:07:40 2010
> @@ -51,7 +51,7 @@ struct edit_baton {
>   const char *target;
>
>   /* ADM_ACCESS is an access baton that includes the TARGET directory */
> -  svn_wc_adm_access_t *adm_access;
> +  svn_wc_context_t *wc_ctx;

The comment should be adjusted to reference WC_CTX, and to note the
NULL possibility.

> @@ -321,56 +321,65 @@ get_dirprops_from_ra(struct dir_baton *b
>  }
>
>
> -/* Return in *LOCAL_DIR_ABSPATH the absolute path for the directory PATH by
> -   searching the access baton set of ADM_ACCESS.  If ADM_ACCESS is NULL then
> -   *LOCAL_DIR_ABSPATH will be NULL.  If LENIENT is TRUE then failure to find
> -   an access baton will not return an error but will set *LOCAL_DIR_ABSPATH to
> -   NULL instead. */
> +/* Return in *LOCAL_DIR_ABSPATH the absolute path for the directory
> +   PATH if PATH is a versioned directory.  If PATH is not a versioned
> +   directory and LENIENT is FALSE then return an error
> +   SVN_ERR_WC_NOT_WORKING_COPY.  If LENIENT is TRUE then failure to
> +   find an access baton will not return an error but will set
> +   *LOCAL_DIR_ABSPATH to NULL instead.
> +
> +   This rather odd interface was originally designed around searching
> +   an access baton set. */

"failure to find an access baton" ?? That certainly doesn't make any
sense. The docstring should also mention that if WC_CTX is NULL, then
nothing will ever be returned (*LOCAL_DIR_ABSPATH is set to NULL).
IMO, this function shouldn't even be called in that situation. Callers
should not even be trying functionality which doesn't make sense (ie.
push the WC_CTX check out).

>...
> -/* Like get_path_access except the returned access baton, in
> -   *PARENT_ACCESS, is for the parent of PATH rather than for PATH
> -   itself. */
> +/* Like get_path_access except the returned path, in
> +   *LOCAL_PARENT_DIR_ABSPATH, is for the parent of PATH rather than
> +   for PATH itself. */

Add comment about WC_CTX.

>...

Cheers,
-g
Received on 2010-03-16 17:35:14 CET

This is an archived mail posted to the Subversion Dev mailing list.