r1390443: Switch ra_svn to templated commands

Hi Stefan.

In <svn.apache.org/r1390443>, you wrote:
[[[
Merge third batch of changes from the 10Gb branch.
Switch ra_svn to templated commands.

Revisions 1388276,1390209
]]]

I assume the main reason for this change is to remove the run-time overhead of parsing a format string, and in that respect it seems fine.

In terms of API style, however, it seems to me that you might as well make the API have a dedicated function for each RA command instead of using this single function with a template parameter.  Then we would have the benefit of static checking of the number and types of the arguments.  The implementation of this 'templated' API pretty much ends up calling a specific function per command anyway.  I'm sure an implementation with dedicated functions would be shorter in source code too (and a nanosecond quicker as well :-).

Does that sound good?

For reference, r1390443 looks like this in svn_ra_svn.h:
[[[
+/** List of all commands supported by the SVN:// protocol. */
+typedef enum svn_ra_svn_cmd_t
+{
+  svn_ra_svn_cmd_target_rev,
+  svn_ra_svn_cmd_open_root,
+  svn_ra_svn_cmd_delete_entry,
+  svn_ra_svn_cmd_add_dir,
+  svn_ra_svn_cmd_open_dir,
[...]
+  svn_ra_svn_cmd_replay_range,
+  svn_ra_svn_cmd_get_deleted_rev,
+
+  svn_ra_svn_cmd__last
+} svn_ra_svn_cmd_t;

/** Write a command over the network, using the same format string notation
  * as svn_ra_svn_write_tuple().
+ *
+ * @deprecated Provided for backward compatibility with the 1.9 API.
+ * Use svn_ra_svn_write_templated_cmd instead.
  */
+SVN_DEPRECATED
svn_error_t *
svn_ra_svn_write_cmd(svn_ra_svn_conn_t *conn,
                      apr_pool_t *pool,
                      const char *cmdname,
                      const char *fmt, ...);

+/** Write a command of type @a cmd over the network connection @a conn.
+ * The parameters to be provided are command-specific.  @a pool will be
+ * used for allocations. */
+svn_error_t *
+svn_ra_svn_write_templated_cmd(svn_ra_svn_conn_t *conn,
+                               apr_pool_t *pool,
+                               svn_ra_svn_cmd_t cmd, ...);
]]]

and the change to each call site looks like this:
[[[
-  SVN_ERR(svn_ra_svn_write_cmd(b->conn, pool, "set-path", "crb(?c)w",
+  SVN_ERR(svn_ra_svn_write_templated_cmd(b->conn, pool,
+                                         svn_ra_svn_cmd_set_path,
           path, rev, start_empty, lock_token, svn_depth_to_word(depth)));
]]]

- Julian

--
Join WANdisco's free daily demo sessions on Scaling Subversion for the Enterprise
<http://www.wandisco.com/training/webinars>