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

[Greg Stein <gs...@gmail.com>]

On Tue, Mar 16, 2010 at 09:07,  <ph...@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

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

[Philip Martin <ph...@wandisco.com>]

Greg Stein <gs...@gmail.com> writes:

>> +/* 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.

Oops. I wrote the doc string and reverted the patch several times,
obviously I got a bit sloppy with the final version.

> 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). 

It's a very weird function now because in 1.6 it was a wrapper around
svn_wc_adm_retrieve and the interface kind of matches that function.
I'll look at whether checking WC_CTX in the caller is better.

-- 
Philip