Re: svn commit: r1767190 - in /subversion/trunk/subversion: include/svn_ra.h libsvn_client/list.c libsvn_ra/ra_loader.c libsvn_ra/ra_loader.h libsvn_ra_local/ra_plugin.c libsvn_ra_serf/serf.c libsvn_ra_svn/client.c

[Daniel Shahaf <d....@daniel.shahaf.name>]

stefan2@apache.org wrote on Sun, Oct 30, 2016 at 22:13:57 -0000:
> Author: stefan2
> Date: Sun Oct 30 22:13:57 2016
> New Revision: 1767190
> 
> URL: http://svn.apache.org/viewvc?rev=1767190&view=rev
> Log:
> Add a RA-level function for svn_repos_list and use that to implement
> svn_client_list, if the server should support the new API.
> 
> Right now, no RA layer actually implements the new API; this will be
> done in follow-up commits.
> 
> * subversion/include/svn_ra.h
>   (svn_ra_dirent_receiver_t,
>    svn_ra_list):  Declare the new interface.
>   (SVN_RA_CAPABILITY_LIST): Declare a new server capability.
> 
> * subversion/libsvn_ra/ra_loader.h
>   (svn_ra__vtable_t): Add LIST function.
> 
> * subversion/libsvn_ra/ra_loader.c
>   (svn_ra_list): Implement the new API and check for its availability.
> 
> * subversion/libsvn_client/list.c
>   (receiver_baton_t,
>    list_receiver): RA-layer-compatible wrapper around the client callback.
>   (list_internal): If we don't have to fetch any properties, use the new
>                    RA-layer API, if available.  Explicitly checking the
>                    existance of the base path is only needed if we use
>                    the client-side code.
> 
> * subversion/libsvn_ra_local/ra_plugin.c
>   (ra_local_vtable): Update vtable.
> 
> * subversion/libsvn_ra_serf/serf.c
>   (serf_vtable): Same.
> 
> * subversion/libsvn_ra_svn/client.c
>   (ra_svn_vtable): Same.

> +++ subversion/trunk/subversion/libsvn_ra/ra_loader.c Sun Oct 30 22:13:57 2016
> @@ -645,6 +645,29 @@ svn_error_t *svn_ra_get_dir2(svn_ra_sess
>                                    path, revision, dirent_fields, pool);
>  }
>  
> +svn_error_t *
> +svn_ra_list(svn_ra_session_t *session,
> +            const char *path,
> +            svn_revnum_t revision,
> +            apr_array_header_t *patterns,
> +            svn_depth_t depth,
> +            apr_uint32_t dirent_fields,
> +            svn_ra_dirent_receiver_t receiver,
> +            void *receiver_baton,
> +            apr_pool_t *scratch_pool)
> +{
> +  SVN_ERR_ASSERT(svn_relpath_is_canonical(path));
> +  if (!session->vtable->list)
> +    return svn_error_create(SVN_ERR_RA_NOT_IMPLEMENTED, NULL, NULL);

Judging by its error message, SVN_ERR_RA_NOT_IMPLEMENTED is supposed to
mean "protocol scheme not recognised", not "feature not supported".

I think this should use SVN_ERR_UNSUPPORTED_FEATURE instead, like
svn_ra__assert_capable_server() does.

(There are a few more instances of this mixup in libsvn_ra_svn/client.c.)

> +
> +  SVN_ERR(svn_ra__assert_capable_server(session, SVN_RA_CAPABILITY_LIST,
> +                                        NULL, scratch_pool));
> +
> +  return session->vtable->list(session, path, revision, patterns, depth,
> +                               dirent_fields, receiver, receiver_baton,
> +                               scratch_pool);
> +}

> +++ subversion/trunk/subversion/libsvn_client/list.c Sun Oct 30 22:13:57 2016
> @@ -221,6 +221,53 @@ get_dir_contents(apr_uint32_t dirent_fie
> @@ -324,6 +365,33 @@ list_internal(const char *path_or_url,
> +  /* Try to use the efficient and fully authz-filtered code path. */
> +      err = svn_ra_list(ra_session, "", loc->rev, patterns, depth,
> +                        dirent_fields, list_receiver, &receiver_baton, pool);
> +
> +      if (   svn_error_find_cause(err, SVN_ERR_UNSUPPORTED_FEATURE)
> +          || svn_error_find_cause(err, SVN_ERR_RA_NOT_IMPLEMENTED))

... and then the second disjunct here will be able to be removed.

Cheers,

Daniel

Re: svn commit: r1767190 - in /subversion/trunk/subversion: include/svn_ra.h libsvn_client/list.c libsvn_ra/ra_loader.c libsvn_ra/ra_loader.h libsvn_ra_local/ra_plugin.c libsvn_ra_serf/serf.c libsvn_ra_svn/client.c

[Stefan Fuhrmann <st...@apache.org>]

On 31.10.2016 02:05, Daniel Shahaf wrote:
> stefan2@apache.org wrote on Sun, Oct 30, 2016 at 22:13:57 -0000:
>> Author: stefan2
>> Date: Sun Oct 30 22:13:57 2016
>> New Revision: 1767190

[snip]

>> +svn_error_t *
>> +svn_ra_list(svn_ra_session_t *session,
>> +            const char *path,
>> +            svn_revnum_t revision,
>> +            apr_array_header_t *patterns,
>> +            svn_depth_t depth,
>> +            apr_uint32_t dirent_fields,
>> +            svn_ra_dirent_receiver_t receiver,
>> +            void *receiver_baton,
>> +            apr_pool_t *scratch_pool)
>> +{
>> +  SVN_ERR_ASSERT(svn_relpath_is_canonical(path));
>> +  if (!session->vtable->list)
>> +    return svn_error_create(SVN_ERR_RA_NOT_IMPLEMENTED, NULL, NULL);
>
> Judging by its error message, SVN_ERR_RA_NOT_IMPLEMENTED is supposed to
> mean "protocol scheme not recognised", not "feature not supported".
>
> I think this should use SVN_ERR_UNSUPPORTED_FEATURE instead, like
> svn_ra__assert_capable_server() does.
>
> (There are a few more instances of this mixup in libsvn_ra_svn/client.c.)

There there are also a few instances in ra_serf where we pass on
SVN_ERR_RA_NOT_IMPLEMENTED upon receiving SVN_ERR_UNSUPPORTED_FEATURE.

But you are right that SVN_ERR_UNSUPPORTED_FEATURE is the better
choice here, so I changed it in r1768311.

Thanks for the reviews!

-- Stefan^2.