You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2016/10/30 23:43:07 UTC
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
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/include/private/svn_log.h
subversion/trunk/subversion/include/svn_ra_svn.h
subversion/trunk/subversion/libsvn_ra_svn/client.c
subversion/trunk/subversion/libsvn_ra_svn/protocol
subversion/trunk/subversion/libsvn_subr/log.c
subversion/trunk/subversion/svnserve/serve.c
Modified: subversion/trunk/subversion/include/private/svn_log.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/private/svn_log.h?rev=1767197&r1=1767196&r2=1767197&view=diff
==============================================================================
--- subversion/trunk/subversion/include/private/svn_log.h (original)
+++ subversion/trunk/subversion/include/private/svn_log.h Sun Oct 30 23:43:06 2016
@@ -253,6 +253,17 @@ const char *
svn_log__get_inherited_props(const char *path,
svn_revnum_t rev,
apr_pool_t *pool);
+
+/**
+ * Return a log string for a list action.
+ *
+ * @since New in 1.10.
+ */
+const char *
+svn_log__list(const char *path, svn_revnum_t revision,
+ apr_array_header_t *patterns, svn_depth_t depth,
+ apr_uint32_t dirent_fields, apr_pool_t *pool);
+
#ifdef __cplusplus
}
#endif /* __cplusplus */
Modified: subversion/trunk/subversion/include/svn_ra_svn.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_ra_svn.h?rev=1767197&r1=1767196&r2=1767197&view=diff
==============================================================================
--- subversion/trunk/subversion/include/svn_ra_svn.h (original)
+++ subversion/trunk/subversion/include/svn_ra_svn.h Sun Oct 30 23:43:06 2016
@@ -68,6 +68,8 @@ extern "C" {
#define SVN_RA_SVN_CAP_EPHEMERAL_TXNPROPS "ephemeral-txnprops"
/* maps to SVN_RA_CAPABILITY_GET_FILE_REVS_REVERSE */
#define SVN_RA_SVN_CAP_GET_FILE_REVS_REVERSE "file-revs-reverse"
+/* maps to SVN_RA_CAPABILITY_LIST */
+#define SVN_RA_SVN_CAP_LIST "list"
/** ra_svn passes @c svn_dirent_t fields over the wire as a list of
Modified: subversion/trunk/subversion/libsvn_ra_svn/client.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_svn/client.c?rev=1767197&r1=1767196&r2=1767197&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_svn/client.c (original)
+++ subversion/trunk/subversion/libsvn_ra_svn/client.c Sun Oct 30 23:43:06 2016
@@ -2838,6 +2838,7 @@ ra_svn_has_capability(svn_ra_session_t *
SVN_RA_SVN_CAP_EPHEMERAL_TXNPROPS},
{SVN_RA_CAPABILITY_GET_FILE_REVS_REVERSE,
SVN_RA_SVN_CAP_GET_FILE_REVS_REVERSE},
+ {SVN_RA_CAPABILITY_LIST, SVN_RA_SVN_CAP_LIST},
{NULL, NULL} /* End of list marker */
};
@@ -2927,6 +2928,77 @@ ra_svn_get_inherited_props(svn_ra_sessio
return SVN_NO_ERROR;
}
+static svn_error_t *
+ra_svn_list(svn_ra_session_t *session,
+ const char *path,
+ svn_revnum_t revision,
+ apr_array_header_t *patterns,
+ svn_depth_t depth,
+ apr_uint32_t dirent_fields,
+ svn_ra_dirent_receiver_t receiver,
+ void *receiver_baton,
+ apr_pool_t *scratch_pool)
+{
+ svn_ra_svn__session_baton_t *sess_baton = session->priv;
+ svn_ra_svn_conn_t *conn = sess_baton->conn;
+ int i;
+ apr_pool_t *iterpool = svn_pool_create(scratch_pool);
+
+ /* Send the list request. */
+ SVN_ERR(svn_ra_svn__write_tuple(conn, scratch_pool, "w(c(?r)w(!", "list",
+ path, revision, svn_depth_to_word(depth)));
+ SVN_ERR(send_dirent_fields(conn, dirent_fields, scratch_pool));
+ SVN_ERR(svn_ra_svn__write_tuple(conn, scratch_pool, "!)(!"));
+
+ for (i = 0; i < patterns->nelts; ++i)
+ {
+ const char *pattern = APR_ARRAY_IDX(patterns, i, const char *);
+ SVN_ERR(svn_ra_svn__write_cstring(conn, scratch_pool, pattern));
+ }
+
+ SVN_ERR(svn_ra_svn__write_tuple(conn, scratch_pool, "!))"));
+
+ /* Handle auth request by server */
+ SVN_ERR(handle_auth_request(sess_baton, scratch_pool));
+
+ /* Read and process list response. */
+ while (1)
+ {
+ svn_ra_svn__item_t *item;
+ const char *dirent_path;
+ const char *kind_word, *date;
+ svn_dirent_t dirent = { 0 };
+
+ svn_pool_clear(iterpool);
+
+ /* Read the next dirent or bail out on "done", respectively */
+ SVN_ERR(svn_ra_svn__read_item(conn, iterpool, &item));
+ if (is_done_response(item))
+ break;
+ if (item->kind != SVN_RA_SVN_LIST)
+ return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
+ _("List entry not a list"));
+ SVN_ERR(svn_ra_svn__parse_tuple(&item->u.list,
+ "cw?(nbr(?c)(?c))",
+ &dirent_path, &kind_word, &dirent.size,
+ &dirent.has_props, &dirent.created_rev,
+ &date, &dirent.last_author));
+
+ /* Convert data. */
+ dirent.kind = svn_node_kind_from_word(kind_word);
+ if (date)
+ SVN_ERR(svn_time_from_cstring(&dirent.time, date, iterpool));
+
+ /* Invoke RECEIVER */
+ SVN_ERR(receiver(dirent_path, &dirent, receiver_baton, iterpool));
+ }
+ svn_pool_destroy(iterpool);
+
+ /* Read the actual command response. */
+ SVN_ERR(svn_ra_svn__read_cmd_response(conn, scratch_pool, ""));
+ return SVN_NO_ERROR;
+}
+
static const svn_ra__vtable_t ra_svn_vtable = {
svn_ra_svn_version,
ra_svn_get_description,
@@ -2966,7 +3038,7 @@ static const svn_ra__vtable_t ra_svn_vta
ra_svn_get_deleted_rev,
ra_svn_get_inherited_props,
NULL /* ra_set_svn_ra_open */,
- NULL /* svn_ra_list */,
+ ra_svn_list,
ra_svn_register_editor_shim_callbacks,
NULL /* commit_ev2 */,
NULL /* replay_range_ev2 */
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".
+ dirent: ( rel-path:string kind:node-kind ? ( size:number
+ has-props:bool created-rev:number
+ [ created-date:string ] [ last-author:string ] ) )
+ | done
+ dirent-field: kind | size | has-props | created-rev | time | last-author
+ response: ( )
+ New in svn 1.10. If rev is not specified, the youngest revision is used.
+
3.1.2. Editor Command Set
An edit operation produces only one response, at close-edit or
Modified: subversion/trunk/subversion/libsvn_subr/log.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/log.c?rev=1767197&r1=1767196&r2=1767197&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/log.c (original)
+++ subversion/trunk/subversion/libsvn_subr/log.c Sun Oct 30 23:43:06 2016
@@ -395,3 +395,28 @@ svn_log__get_inherited_props(const char
log_path = "/";
return apr_psprintf(pool, "get-inherited-props %s r%ld", log_path, rev);
}
+
+const char *
+svn_log__list(const char *path, svn_revnum_t revision,
+ apr_array_header_t *patterns, svn_depth_t depth,
+ apr_uint32_t dirent_fields, apr_pool_t *pool)
+{
+ svn_stringbuf_t *pattern_text = svn_stringbuf_create_empty(pool);
+ const char *log_path;
+ int i;
+
+ if (path && path[0] != '\0')
+ log_path = svn_path_uri_encode(path, pool);
+ else
+ log_path = "/";
+
+ for (i = 0; i < patterns->nelts; ++i)
+ {
+ const char *pattern = APR_ARRAY_IDX(patterns, i, const char *);
+ svn_stringbuf_appendbyte(pattern_text, ' ');
+ svn_stringbuf_appendcstr(pattern_text, pattern);
+ }
+
+ return apr_psprintf(pool, "list %s r%ld%s%s", log_path, revision,
+ log_depth(depth, pool), pattern_text->data);
+}
Modified: subversion/trunk/subversion/svnserve/serve.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svnserve/serve.c?rev=1767197&r1=1767196&r2=1767197&view=diff
==============================================================================
--- subversion/trunk/subversion/svnserve/serve.c (original)
+++ subversion/trunk/subversion/svnserve/serve.c Sun Oct 30 23:43:06 2016
@@ -3538,6 +3538,123 @@ get_inherited_props(svn_ra_svn_conn_t *c
return SVN_NO_ERROR;
}
+/* Baton type to be used with list_receiver. */
+typedef struct list_receiver_baton_t
+{
+ /* Send the data through this connection. */
+ svn_ra_svn_conn_t *conn;
+
+ /* If set, send path and kind only. */
+ svn_boolean_t path_info_only;
+} list_receiver_baton_t;
+
+/* Implements svn_repos_dirent_receiver_t, sending DIRENT and PATH to the
+ * client. BATON must be a list_receiver_baton_t. */
+static svn_error_t *
+list_receiver(const char *path,
+ svn_dirent_t *dirent,
+ void *baton,
+ apr_pool_t *pool)
+{
+ list_receiver_baton_t *b = baton;
+
+ if (b->path_info_only)
+ SVN_ERR(svn_ra_svn__write_tuple(b->conn, pool, "cw", path,
+ svn_node_kind_to_word(dirent->kind)));
+ else
+ SVN_ERR(svn_ra_svn__write_tuple(b->conn, pool, "cw(nbr(?c)(?c))", path,
+ svn_node_kind_to_word(dirent->kind),
+ (apr_uint64_t) dirent->size,
+ dirent->has_props, dirent->created_rev,
+ svn_time_to_cstring(dirent->time, pool),
+ dirent->last_author));
+
+ return SVN_NO_ERROR;
+}
+
+static svn_error_t *
+list(svn_ra_svn_conn_t *conn,
+ apr_pool_t *pool,
+ svn_ra_svn__list_t *params,
+ void *baton)
+{
+ server_baton_t *b = baton;
+ const char *path, *full_path;
+ svn_revnum_t rev;
+ svn_depth_t depth;
+ apr_array_header_t *patterns;
+ svn_fs_root_t *root;
+ const char *depth_word;
+ apr_uint64_t dirent_fields;
+ svn_ra_svn__list_t *dirent_fields_list = NULL;
+ svn_ra_svn__list_t *patterns_list = NULL;
+ int i;
+ list_receiver_baton_t rb;
+ svn_error_t *err, *write_err;
+
+ authz_baton_t ab;
+ ab.server = b;
+ ab.conn = conn;
+
+ /* Read the command parameters. */
+ SVN_ERR(svn_ra_svn__parse_tuple(params, "c(?r)w?l?l", &path, &rev,
+ &depth_word, &dirent_fields_list,
+ &patterns_list));
+
+ depth = svn_depth_from_word(depth_word);
+ SVN_ERR(parse_dirent_fields(&dirent_fields, dirent_fields_list));
+
+ full_path = svn_fspath__join(b->repository->fs_path->data,
+ svn_relpath_canonicalize(path, pool), pool);
+
+ /* Read the patterns list. */
+ patterns = apr_array_make(pool, 0, sizeof(const char *));
+ for (i = 0; i < patterns_list->nelts; ++i)
+ {
+ svn_ra_svn__item_t *elt = &SVN_RA_SVN__LIST_ITEM(patterns_list, i);
+
+ if (elt->kind != SVN_RA_SVN_STRING)
+ return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
+ "Pattern field not a string");
+
+ APR_ARRAY_PUSH(patterns, const char *) = elt->u.string.data;
+ }
+
+ /* Check authorizations */
+ SVN_ERR(must_have_access(conn, pool, b, svn_authz_read,
+ full_path, FALSE));
+
+ if (!SVN_IS_VALID_REVNUM(rev))
+ SVN_CMD_ERR(svn_fs_youngest_rev(&rev, b->repository->fs, pool));
+
+ SVN_ERR(log_command(b, conn, pool, "%s",
+ svn_log__list(full_path, rev, patterns, depth,
+ dirent_fields, pool)));
+
+ /* Fetch the root of the appropriate revision. */
+ SVN_CMD_ERR(svn_fs_revision_root(&root, b->repository->fs, rev, pool));
+
+ /* Fetch the directory entries if requested and send them immediately. */
+ rb.conn = conn;
+ rb.path_info_only = (dirent_fields & ~SVN_DIRENT_KIND) == 0;
+
+ err = svn_repos_list(root, full_path, patterns, depth, rb.path_info_only,
+ authz_check_access_cb_func(b), &ab, list_receiver,
+ &rb, NULL, NULL, pool);
+
+
+ /* Finish response. */
+ write_err = svn_ra_svn__write_word(conn, pool, "done");
+ if (write_err)
+ {
+ svn_error_clear(err);
+ return write_err;
+ }
+ SVN_CMD_ERR(err);
+
+ return svn_error_trace(svn_ra_svn__write_cmd_response(conn, pool, ""));
+}
+
static const svn_ra_svn__cmd_entry_t main_commands[] = {
{ "reparent", reparent },
{ "get-latest-rev", get_latest_rev },
@@ -3570,6 +3687,7 @@ static const svn_ra_svn__cmd_entry_t mai
{ "replay-range", replay_range },
{ "get-deleted-rev", get_deleted_rev },
{ "get-iprops", get_inherited_props },
+ { "list", list },
{ NULL }
};
@@ -3968,7 +4086,7 @@ construct_server_baton(server_baton_t **
* send an empty mechlist. */
if (params->compression_level > 0)
SVN_ERR(svn_ra_svn__write_cmd_response(conn, scratch_pool,
- "nn()(wwwwwwwwwww)",
+ "nn()(wwwwwwwwwwww)",
(apr_uint64_t) 2, (apr_uint64_t) 2,
SVN_RA_SVN_CAP_EDIT_PIPELINE,
SVN_RA_SVN_CAP_SVNDIFF1,
@@ -3980,11 +4098,12 @@ construct_server_baton(server_baton_t **
SVN_RA_SVN_CAP_PARTIAL_REPLAY,
SVN_RA_SVN_CAP_INHERITED_PROPS,
SVN_RA_SVN_CAP_EPHEMERAL_TXNPROPS,
- SVN_RA_SVN_CAP_GET_FILE_REVS_REVERSE
+ SVN_RA_SVN_CAP_GET_FILE_REVS_REVERSE,
+ SVN_RA_SVN_CAP_LIST
));
else
SVN_ERR(svn_ra_svn__write_cmd_response(conn, scratch_pool,
- "nn()(wwwwwwwwww)",
+ "nn()(wwwwwwwwwww)",
(apr_uint64_t) 2, (apr_uint64_t) 2,
SVN_RA_SVN_CAP_EDIT_PIPELINE,
SVN_RA_SVN_CAP_ABSENT_ENTRIES,
@@ -3995,7 +4114,8 @@ construct_server_baton(server_baton_t **
SVN_RA_SVN_CAP_PARTIAL_REPLAY,
SVN_RA_SVN_CAP_INHERITED_PROPS,
SVN_RA_SVN_CAP_EPHEMERAL_TXNPROPS,
- SVN_RA_SVN_CAP_GET_FILE_REVS_REVERSE
+ SVN_RA_SVN_CAP_GET_FILE_REVS_REVERSE,
+ SVN_RA_SVN_CAP_LIST
));
/* Read client response, which we assume to be in version 2 format:
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
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 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>.
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.
> +
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>.
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.
> +