[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: Daniel Shahaf <d.s_at_daniel.shahaf.name>
Date: Sun, 6 Nov 2016 01:15:53 +0000

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_at_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 — "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)"

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)



Received on 2016-11-06 02:18:13 CET

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

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.