[prev in list] [next in list] [prev in thread] [next in thread] 

List:       subversion-commits
Subject:    Re: svn commit: r1767197 - in /subversion/trunk/subversion: include/private/svn_log.h include/svn_ra
From:       Daniel Shahaf <d.s () daniel ! shahaf ! name>
Date:       2016-10-31 0:45:42
Message-ID: 20161031004542.GB25716 () fujitsu ! shahaf ! local2
[Download RAW message or body]

stefan2@apache.org wrote on Sun, Oct 30, 2016 at 23:43:07 -0000:
> Author: stefan2
> Date: Sun Oct 30 23:43:06 2016
> New Revision: 1767197
> 
> URL: http://svn.apache.org/viewvc?rev=1767197&view=rev
> Log:
> Implement svn_ra_list in ra_svn.  The wire protocol for this command
> has been insprired by the get-log and get-dir commands alike.
> 
> * subversion/libsvn_ra_svn/protocol
> (2.1 Capabilities): Add the 'list' capability.
> (3.1.1. Main Command Set): Add the protocol of the 'list' command.
> 
> * subversion/include/svn_ra_svn.h
> (SVN_RA_CAPABILITY_LIST): Declare this new capability in code.
> 
> * subversion/svnserve/serve.c
> (list_receiver_baton_t,
> list_receiver,
> list): Implement the new command.
> (main_commands): Register the new command handler.
> (construct_server_baton): Advertise the new capability.
> 
> * subversion/libsvn_ra_svn/client.c
> (ra_svn_has_capability): Check for the new capability as well.
> (ra_svn_list): Client-side implementation of the protocol.
> (ra_svn_vtable): Register the new list function.
> 
> * subversion/include/private/svn_log.h
> (svn_log__list): Add a new internal API to allow for logging the
> new list command.
> 
> * subversion/libsvn_subr/log.c
> (svn_log__list): Implement.
> 
> Modified: subversion/trunk/subversion/libsvn_ra_svn/protocol
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_svn/protocol?rev=1767197&r1=1767196&r2=1767197&view=diff
>  ==============================================================================
> --- subversion/trunk/subversion/libsvn_ra_svn/protocol (original)
> +++ subversion/trunk/subversion/libsvn_ra_svn/protocol Sun Oct 30 23:43:06 2016
> @@ -206,6 +206,8 @@ capability and C indicates a client capa
> retrieval of inherited properties via the get-dir and
> get-file commands and also supports the get-iprops
> command (see section 3.1.1).
> +[S]  list              If the server presents this capability, it supports the
> +                       list command (see section 3.1.1).
> 
> 3. Commands
> -----------
> @@ -487,6 +489,18 @@ second place for auth-request point as n
> response: ( inherited-props:iproplist )
> New in svn 1.8.  If rev is not specified, the youngest revision is used.
> 
> +  list
> +    params:   ( path:string [ rev:number ] depth:word
> +                ( field:dirent-field ... ) ( pattern:string ... ) )
> +    Before sending response, server sends dirent, ending with "done".

Typo: s/dirent/dirents/

> +    dirent:   ( rel-path:string kind:node-kind ? ( size:number
> +                has-props:bool created-rev:number
> +                [ created-date:string ] [ last-author:string ] ) )
> +              | done

Why should size,has-props,created-rev be all present or all absent?
Wouldn't it be better to let each element (other than the first two) be
optional, i.e., 

       dirent:   ( rel-path:string kind:node-kind
                   ? [ size:number ]
                     [ has-props:bool ]
		     [ created-rev:number ]
                     [ created-date:string ]
		     [ last-author:string ] )

?  This decouples the protocol from the backend (specifically, from what
bits the backend has cheaply available).  

Typo: there is one ")" too many.

> +    dirent-field: kind | size | has-props | created-rev | time | last-author

Don't you need here a "| word" alternative, like get-dir has?  I think
that's not a type documentation, but a forward compatibility indicator,
i.e., indicating that more words may be added in the future.

> +    response: ( )
> +    New in svn 1.10.  If rev is not specified, the youngest revision is used.
> +


[prev in list] [next in list] [prev in thread] [next in thread]