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

Re: svn commit: r1767197 - in /subversion/trunk/subversion: include/private/svn_log.h include/svn_ra_svn.h libsvn_ra_svn/client.c libsvn_ra_svn/protocol libsvn_subr/log.c svnserve/serve.c

From: Stefan Fuhrmann <stefan2_at_apache.org>
Date: Sat, 5 Nov 2016 13:52:49 +0100

On 31.10.2016 01:45, Daniel Shahaf wrote:
> stefan2_at_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

[snip]

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

Fixed in r1768185.

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

Yes, that would be better.

It would require to either make has_props a tristate
or extending the protocol to allow for optional bools.

I think we should do the latter because regardless of
whether we have them at the protocol level or not, we
must specify a value in the output data struct at the
receiver side. So, we might as well define it to FALSE
for n/a data.

This does not effect any existing protocol usage or
code as the format patterns of the existing commands
do not change. They are represented as "words" in
the pre-parsed "item" structure, so even old code
receiving an optional boolean would parse them fine -
in case we were to add optional booleans to existing
command responses in the future.

And while we are at it, we should make vwrite_tuple()
actually support optional tristates and numbers.

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

Hm, makes sense. I guess that usage of "| word" should be
commented on in the file. I for one, mis-interpreted it.

I'll make all these changes later today.

-- Stefan^2.
Received on 2016-11-05 13:52:58 CET

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