You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Fuhrmann <st...@apache.org> on 2016/11/05 12:52:49 UTC
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
On 31.10.2016 01:45, Daniel Shahaf wrote:
> 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
[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.
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
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Fuhrmann wrote on Sat, Nov 05, 2016 at 22:01:03 +0100:
> On 05.11.2016 17:41, Daniel Shahaf wrote:
> >Stefan Fuhrmann wrote on Sat, Nov 05, 2016 at 13:52:49 +0100:
> >>On 31.10.2016 01:45, Daniel Shahaf wrote:
> >>>stefan2@apache.org wrote on Sun, Oct 30, 2016 at 23:43:07 -0000:
> >>>>+ 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.
> >
> >I don't quite follow, sorry. Let me just clarify my idea:
> >
> >We'll use the spec I gave above, with �[ foo:bool ]�, which is an
> >optional bool. At the protocol level, that suffices; it is equivalent
> >to a non-optional �foo:tristate�, but without needing to add
> >a "tristate" type to the wire protocol. At the API level, we'd provide
> >two parsing options: '3' to parse the optional bool into an
> >svn_tristate_t, and 'B' to parse the optional bool into an
> >svn_boolean_t, converting svn_tristate_unknown to FALSE. Using 'b' to
> >parse an optional error should be a fatal error (an SVN_ERR_ASSERT).
>
> Well, B does not write to a svn_boolean_t but to an uint64.
>
Well, okay. My point was just that the parser function could let its
caller decide how an absent optional bool would be represented, by
specifying one format character for "wire boolean as svn_boolean_t
('absent' maps to FALSE)" and another format character for "wire boolean
as svn_tristate_t".
> >Makes sense? Could you clarify your idea as well?
>
> I was more looking at the parser / writer code and its format
> specification options.
>
> Not allowing b to be part of e.g. an optional struct makes
> the parser API slightly less easy to use. That does not take
> away from the fact that optional bools are effectively tri-
> states and the API user needs to decide how to treat missing
> data by specifying the suitable data format string.
>
The idea behind forbidding 'b' to be optional in the parser function was
to force callers to choose a behaviour for when the boolean was absent.
(As PEP 20 says \u2014 "Explicit is better than implicit".)
> >>And while we are at it, we should make vwrite_tuple()
> >>actually support optional tristates and numbers.
> >
> >What do you mean by "optional tristate"? It sounds like you're talking
> >about a �[ foo:tristate ]�, which doesn't appear in the current protocol.
> >If the question is, how would one call vwrite_tuple() to generate
> >a tuple specified as �[ foo:bool ]�, then I think there are three ways:
> >
> >* vwrite_tuple("()")
> >* vwrite_tuple("(b)", (svn_boolean_t)foo)
> >* vwrite_tuple("3", (svn_tristate_t)bar)
>
> The last one is currently not supported. That is not symmetric
> to the receiver side where "3" is a support format spec.
>
Right. I see now that the writer function already supports "?", so
adding parentheses to the third case would make sense:
vwrite_tuple("(?X)", (svn_tristate_t)baz)
would generate either �( ) � or �( false ) � or �( true ) �.
> Worse, trying to write an optional number using a "(?n)" format
> causes a malfunction today.
>
That's just a bug. The attached patch should fix it (untested).
> Where they make sense, I'd like to support all format specs and
> combinations in the parser and write - even if we might not use
> them right now.
Personally, I wouldn't worry about this too much; we can implement these
things as and when we need them with little effort.
> The only thing we can't support meaningfully
> on the sender side is a "?b" because we don't have a n/a value
> to check for.
So we'd need some new format code which means "take an svn_tristate_t
argument and render it as a wire bool"; something like the "(?X)"
hereinabove.
The catch is that the 'X' format character should be prohibited in the
*non*-optional part of a tuple, since it's supposed to be marshalled as
a bool, and svn_tristate_unknown is then unrepresentable.
Proof of concept attached. (I used 'X' as a placeholder; we can pick
some more suitable format character)
Cheers,
Daniel
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
Posted by Stefan Fuhrmann <st...@apache.org>.
On 05.11.2016 17:41, Daniel Shahaf wrote:
> Stefan Fuhrmann wrote on Sat, Nov 05, 2016 at 13:52:49 +0100:
>> On 31.10.2016 01:45, Daniel Shahaf wrote:
>>> stefan2@apache.org wrote on Sun, Oct 30, 2016 at 23:43:07 -0000:
>>>> + 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.
>
> I don't quite follow, sorry. Let me just clarify my idea:
>
> We'll use the spec I gave above, with �[ foo:bool ]�, which is an
> optional bool. At the protocol level, that suffices; it is equivalent
> to a non-optional �foo:tristate�, but without needing to add
> a "tristate" type to the wire protocol. At the API level, we'd provide
> two parsing options: '3' to parse the optional bool into an
> svn_tristate_t, and 'B' to parse the optional bool into an
> svn_boolean_t, converting svn_tristate_unknown to FALSE. Using 'b' to
> parse an optional error should be a fatal error (an SVN_ERR_ASSERT).
Well, B does not write to a svn_boolean_t but to an uint64.
> My point is that we don't need the wire protocol to have a notion of "a
> missing bool is interpreted as FALSE"; we can leave that decision with
> each individual call to the parser function.
I think we are in agreement that your proposed representation
of dirent does not require a change to the wire protocol.
> Makes sense? Could you clarify your idea as well?
I was more looking at the parser / writer code and its format
specification options.
Not allowing b to be part of e.g. an optional struct makes
the parser API slightly less easy to use. That does not take
away from the fact that optional bools are effectively tri-
states and the API user needs to decide how to treat missing
data by specifying the suitable data format string.
>> 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.
>>
>
> I'm not sure what you're trying to say. Of course old code will cope
> just fine with optional booleans; from that code's perspective, the
> optional booleans will occur "past the end of a tuple", so it will
> ignore them entirely. The old code just needs to be able to tokenize
> optional bools, so it can skip past them. That's how the protocol is
> designed.
Well, yes. I just wanted to confirm that the wire protocol
parser (tokenizer) implementation is fine and actually supports
the full range of structures that could be expressed in the
wire protocol. So, that won't bail out on optional data
unless it introduces a new type of structural element.
The actual token -> data parser and more so the writer API
side are less complete.
>> And while we are at it, we should make vwrite_tuple()
>> actually support optional tristates and numbers.
>
> What do you mean by "optional tristate"? It sounds like you're talking
> about a �[ foo:tristate ]�, which doesn't appear in the current protocol.
> If the question is, how would one call vwrite_tuple() to generate
> a tuple specified as �[ foo:bool ]�, then I think there are three ways:
>
> * vwrite_tuple("()")
> * vwrite_tuple("(b)", (svn_boolean_t)foo)
> * vwrite_tuple("3", (svn_tristate_t)bar)
The last one is currently not supported. That is not symmetric
to the receiver side where "3" is a support format spec.
Worse, trying to write an optional number using a "(?n)" format
causes a malfunction today.
> where the third option generates an empty tuple for svn_tristate_unknown
> and a single-element tuple for svn_tristate_false and svn_tristate_true.
Yes, that is how I would like to implement it.
Where they make sense, I'd like to support all format specs and
combinations in the parser and write - even if we might not use
them right now. The only thing we can't support meaningfully
on the sender side is a "?b" because we don't have a n/a value
to check for.
-- Stefan^2.
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
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Fuhrmann wrote on Sat, Nov 05, 2016 at 13:52:49 +0100:
> On 31.10.2016 01:45, Daniel Shahaf wrote:
> >stefan2@apache.org wrote on Sun, Oct 30, 2016 at 23:43:07 -0000:
> >>+ 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.
I don't quite follow, sorry. Let me just clarify my idea:
We'll use the spec I gave above, with �[ foo:bool ]�, which is an
optional bool. At the protocol level, that suffices; it is equivalent
to a non-optional �foo:tristate�, but without needing to add
a "tristate" type to the wire protocol. At the API level, we'd provide
two parsing options: '3' to parse the optional bool into an
svn_tristate_t, and 'B' to parse the optional bool into an
svn_boolean_t, converting svn_tristate_unknown to FALSE. Using 'b' to
parse an optional error should be a fatal error (an SVN_ERR_ASSERT).
My point is that we don't need the wire protocol to have a notion of "a
missing bool is interpreted as FALSE"; we can leave that decision with
each individual call to the parser function.
Makes sense? Could you clarify your idea as well?
> 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.
>
I'm not sure what you're trying to say. Of course old code will cope
just fine with optional booleans; from that code's perspective, the
optional booleans will occur "past the end of a tuple", so it will
ignore them entirely. The old code just needs to be able to tokenize
optional bools, so it can skip past them. That's how the protocol is
designed.
> And while we are at it, we should make vwrite_tuple()
> actually support optional tristates and numbers.
What do you mean by "optional tristate"? It sounds like you're talking
about a �[ foo:tristate ]�, which doesn't appear in the current protocol.
If the question is, how would one call vwrite_tuple() to generate
a tuple specified as �[ foo:bool ]�, then I think there are three ways:
* vwrite_tuple("()")
* vwrite_tuple("(b)", (svn_boolean_t)foo)
* vwrite_tuple("3", (svn_tristate_t)bar)
where the third option generates an empty tuple for svn_tristate_unknown
and a single-element tuple for svn_tristate_false and svn_tristate_true.
Cheers,
Daniel