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:
> >>>stef...@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)"
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
Index: subversion/libsvn_ra_svn/marshal.c
===================================================================
--- subversion/libsvn_ra_svn/marshal.c (revision 1767714)
+++ subversion/libsvn_ra_svn/marshal.c (working copy)
@@ -913,6 +913,15 @@ vwrite_tuple_revision_opt(svn_ra_svn_conn_t *conn,
}
static svn_error_t *
+vwrite_tuple_number_opt(svn_ra_svn_conn_t *conn, apr_pool_t *pool, va_list *ap)
+{
+ apr_uint64_t number = va_arg(*ap, apr_uint64_t);
+ if (number != SVN_RA_SVN_UNSPECIFIED_NUMBER)
+ SVN_ERR(svn_ra_svn__write_number(conn, pool, va_arg(*ap, apr_uint64_t)));
+ return SVN_NO_ERROR;
+}
+
+static svn_error_t *
vwrite_tuple_number(svn_ra_svn_conn_t *conn, apr_pool_t *pool, va_list *ap)
{
return svn_ra_svn__write_number(conn, pool, va_arg(*ap, apr_uint64_t));
@@ -1157,7 +1166,8 @@ static svn_error_t *vwrite_tuple(svn_ra_svn_conn_t
SVN_ERR(opt ? vwrite_tuple_revision_opt(conn, pool, ap)
: vwrite_tuple_revision(conn, pool, ap));
else if (*fmt == 'n' && !opt)
- SVN_ERR(vwrite_tuple_number(conn, pool, ap));
+ SVN_ERR(opt ? vwrite_tuple_number_opt(conn, pool, ap)
+ : vwrite_tuple_number(conn, pool, ap));
else if (*fmt == 'b' && !opt)
SVN_ERR(vwrite_tuple_boolean(conn, pool, ap));
else if (*fmt == '!' && !*(fmt + 1))
Index: subversion/libsvn_ra_svn/marshal.c
===================================================================
--- subversion/libsvn_ra_svn/marshal.c (revision 1767714)
+++ subversion/libsvn_ra_svn/marshal.c (working copy)
@@ -925,6 +925,29 @@ vwrite_tuple_boolean(svn_ra_svn_conn_t *conn, apr_
}
static svn_error_t *
+vwrite_tuple_boolean_opt(svn_ra_svn_conn_t *conn, apr_pool_t *pool, va_list *ap)
+{
+ switch (va_arg(*ap, svn_tristate_t))
+ {
+ case svn_tristate_unknown:
+ break;
+
+ case svn_tristate_false:
+ SVN_ERR(svn_ra_svn__write_boolean(conn, pool, FALSE));
+ break;
+
+ case svn_tristate_true:
+ SVN_ERR(svn_ra_svn__write_boolean(conn, pool, TRUE));
+ break;
+
+ default:
+ SVN_ERR_MALFUNCTION();
+ }
+
+ return SVN_NO_ERROR;
+}
+
+static svn_error_t *
write_tuple_cstring(svn_ra_svn_conn_t *conn,
apr_pool_t *pool,
const char *cstr)
@@ -1160,6 +1183,8 @@ static svn_error_t *vwrite_tuple(svn_ra_svn_conn_t
SVN_ERR(vwrite_tuple_number(conn, pool, ap));
else if (*fmt == 'b' && !opt)
SVN_ERR(vwrite_tuple_boolean(conn, pool, ap));
+ else if (*fmt == 'X' && opt)
+ SVN_ERR(vwrite_tuple_boolean_opt(conn, pool, ap));
else if (*fmt == '!' && !*(fmt + 1))
return SVN_NO_ERROR;
else
