Michael Wood wrote:
>
> Your patch is wrapped.
Very interesting: you saw a wrapped copy, and the mailing list archive
shows it wrapped, but the copy I got back from the list server is not
wrapped for me. I think it's because your (and the archive's) viewers
don't support "format=flowed" in the "Content-Type" header field. To
alleviate this I could set Mozilla's "wrap at" to column 999, which is
the maximum allowed by the RFCs. Then only very long paragraphs would
get wrapped. It would make no difference to viewers that understand
"format=flowed", but I think many viewers don't and don't even have an
option to wrap long lines to fit the window (the user has to scroll
sideways), and so this would be awkward for those people.
Oh well, here they are as attachments. Message body repeated here as well.
_______________________________
In svnserve/serve.c the two functions "switch_cmd" and "diff" are
identical except that one passes TRUE and the other FALSE to the
"text_deltas" parameter of svn_repos_begin_report. The functions are 50
lines long - not terribly complex but definitely worth factoring out to
make the similarity obvious. Of course they might not remain this
similar in future, but at present they are. A suggested factoring is
given in the first diff below. Is this appropriate?
Closely related, and therefore in this same message:
I think the comment for the "text_deltas" flag has a typo that could be
fixed by removing two spurious words, as in the second diff below. But
maybe there is more to it. I don't quite understand how this single
flag makes the whole difference between "svn diff" and "svn switch".
Could someone who knows please verify that "false" just means "don't
generate text deltas"; if it means "do something else instead", then the
comment ought to say what it does instead.
- Julian
* subversion/svnserve/serve.c:
Factor out the common body of functions "switch_cmd" and "diff".
* subversion/include/svn_repos.h:
Fix typo in comment.
Index: subversion/svnserve/serve.c
===================================================================
--- subversion/svnserve/serve.c (revision 4704)
+++ subversion/svnserve/serve.c (working copy)
@@ -571,13 +571,16 @@
return handle_report(conn, pool, b->repos_url, report_baton);
}
-static svn_error_t *switch_cmd(svn_ra_svn_conn_t *conn, apr_pool_t *pool,
- apr_array_header_t *params, void *baton)
+/* Presently the implementations of "switch" and "diff" are so similar that
+ * they share this code. They need not continue to be so similar in future. */
+static svn_error_t *switch_or_diff(svn_ra_svn_conn_t *conn, apr_pool_t *pool,
+ apr_array_header_t *params, void *baton,
+ svn_boolean_t do_switch)
{
server_baton_t *b = baton;
svn_revnum_t rev;
const char *target;
- const char *switch_url, *switch_path;
+ const char *other_url, *other_path;
svn_boolean_t recurse;
const svn_delta_editor_t *editor;
void *edit_baton, *report_baton;
@@ -586,31 +589,31 @@
/* Parse the arguments. */
SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "[r]cbc", &rev, &target,
- &recurse, &switch_url));
+ &recurse, &other_url));
if (svn_path_is_empty(target))
target = NULL; /* ### Compatibility hack, shouldn't be needed */
if (!SVN_IS_VALID_REVNUM(rev))
SVN_CMD_ERR(svn_fs_youngest_rev(&rev, b->fs, pool));
- /* Verify that switch_url is in the same repository and get its fs path. */
- switch_url = svn_path_uri_decode(switch_url, pool);
+ /* Verify that other_url is in the same repository and get its fs path. */
+ other_url = svn_path_uri_decode(other_url, pool);
len = strlen(b->repos_url);
- if (strncmp(switch_url, b->repos_url, len) != 0)
+ if (strncmp(other_url, b->repos_url, len) != 0)
{
err = svn_error_createf (SVN_ERR_RA_ILLEGAL_URL, NULL,
"'%s'\n"
"is not the same repository as\n"
- "'%s'", switch_url, b->repos_url);
+ "'%s'", other_url, b->repos_url);
/* Wrap error so that it gets reported back to the client. */
return svn_error_create(SVN_ERR_RA_SVN_CMD_ERR, err, NULL);
}
- switch_path = switch_url + len;
+ other_path = other_url + len;
/* Make an svn_repos report baton. Tell it to drive the network editor
* when the report is complete. */
svn_ra_svn_get_editor(&editor, &edit_baton, conn, pool, NULL, NULL);
SVN_CMD_ERR(svn_repos_begin_report(&report_baton, rev, b->user, b->repos,
- b->fs_path, target, switch_path, TRUE,
+ b->fs_path, target, other_path, do_switch,
recurse, editor, edit_baton, pool));
/* Write an empty command-reponse, telling the client to start reporting. */
@@ -621,6 +624,18 @@
return handle_report(conn, pool, b->repos_url, report_baton);
}
+static svn_error_t *switch_cmd(svn_ra_svn_conn_t *conn, apr_pool_t *pool,
+ apr_array_header_t *params, void *baton)
+{
+ return switch_or_diff(conn, pool, params, baton, TRUE);
+}
+
+static svn_error_t *diff(svn_ra_svn_conn_t *conn, apr_pool_t *pool,
+ apr_array_header_t *params, void *baton)
+{
+ return switch_or_diff(conn, pool, params, baton, FALSE);
+}
+
static svn_error_t *status(svn_ra_svn_conn_t *conn, apr_pool_t *pool,
apr_array_header_t *params, void *baton)
{
@@ -652,56 +667,6 @@
return handle_report(conn, pool, b->repos_url, report_baton);
}
-static svn_error_t *diff(svn_ra_svn_conn_t *conn, apr_pool_t *pool,
- apr_array_header_t *params, void *baton)
-{
- server_baton_t *b = baton;
- svn_revnum_t rev;
- const char *target, *versus_url, *versus_path;
- svn_boolean_t recurse;
- const svn_delta_editor_t *editor;
- void *edit_baton, *report_baton;
-
- int len;
- svn_error_t *err;
-
- /* Parse the arguments. */
- SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "[r]cbc", &rev, &target,
- &recurse, &versus_url));
- if (svn_path_is_empty(target))
- target = NULL; /* ### Compatibility hack, shouldn't be needed */
- if (!SVN_IS_VALID_REVNUM(rev))
- SVN_CMD_ERR(svn_fs_youngest_rev(&rev, b->fs, pool));
-
- /* Verify that versus_url is in the same repository and get its fs path. */
- versus_url = svn_path_uri_decode(versus_url, pool);
- len = strlen(b->repos_url);
- if (strncmp(versus_url, b->repos_url, len) != 0)
- {
- err = svn_error_createf (SVN_ERR_RA_ILLEGAL_URL, NULL,
- "'%s'\n"
- "is not the same repository as\n"
- "'%s'", versus_url, b->repos_url);
- /* Wrap error so that it gets reported back to the client. */
- return svn_error_create(SVN_ERR_RA_SVN_CMD_ERR, err, NULL);
- }
- versus_path = versus_url + len;
-
- /* Make an svn_repos report baton. Tell it to drive the network editor
- * when the report is complete. */
- svn_ra_svn_get_editor(&editor, &edit_baton, conn, pool, NULL, NULL);
- SVN_CMD_ERR(svn_repos_begin_report(&report_baton, rev, b->user, b->repos,
- b->fs_path, target, versus_path, FALSE,
- recurse, editor, edit_baton, pool));
-
- /* Write an empty command-reponse, telling the client to start reporting. */
- SVN_ERR(svn_ra_svn_write_cmd_response(conn, pool, ""));
-
- /* Handle the client's report; when it's done, svn_repos will drive the
- * network editor with the diff. */
- return handle_report(conn, pool, b->repos_url, report_baton);
-}
-
/* Send a log entry to the client. */
static svn_error_t *log_receiver(void *baton, apr_hash_t *changed_paths,
svn_revnum_t rev, const char *author,
Index: subversion/include/svn_repos.h
===================================================================
--- subversion/include/svn_repos.h (revision 4704)
+++ subversion/include/svn_repos.h (working copy)
@@ -182,7 +182,7 @@
* transform the reported heirarchy to revision @a revnum, preserving the
* reported heirarchy.
*
- * @a text_deltas instructs the driver of the @a editor to enable to disable
+ * @a text_deltas instructs the driver of the @a editor to enable
* the generation of text deltas.
*
* @a recurse instructs the driver of the @a editor to send a recursive
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Feb 4 23:29:45 2003