You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2003/02/03 22:36:43 UTC
"serve.c" and "text_deltas" parameter of svn_repos_begin_report
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
Re: "serve.c" and "text_deltas" parameter of svn_repos_begin_report
Posted by Julian Foad <ju...@btopenworld.com>.
Greg Hudson wrote:
> On Mon, 2003-02-03 at 17:36, Julian Foad wrote:
>
>>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.
>
> I don't really believe in factoring out code which happens to be similar
> despite accomplishing rather different functional goals. "switch" and
> "diff" are very dissimilar operations (certainly more different than
> "switch" and "update"); they just happen to accept the same parameters.
> A change to "switch" is more likely than not to be inapplicable to
> "diff", so combining the implementations doesn't make maintenance any
> easier.
I see what you mean, but I think it's debatable. I'd say maintenance in
the medium term is more likely to involve identical changes to both.
And it took me quite a while of looking at one and then the other to be
certain that there really was no difference other than TRUE/FALSE, and
then that was very puzzling. But I accept your point of view, and am OK
with leaving it alone.
> Now, I would be willing to accept code to factor out, say, the 13 lines
> of code which check for a valid versus_url. That would apply to
> link_path as well as switch_cmd and diff.
That sounds like a good way to go. It should only take a few minutes,
but that will turn into an hour and I need my sleep now.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: "serve.c" and "text_deltas" parameter of svn_repos_begin_report
Posted by Greg Hudson <gh...@MIT.EDU>.
On Mon, 2003-02-03 at 17:36, Julian Foad wrote:
> 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.
I don't really believe in factoring out code which happens to be similar
despite accomplishing rather different functional goals. "switch" and
"diff" are very dissimilar operations (certainly more different than
"switch" and "update"); they just happen to accept the same parameters.
A change to "switch" is more likely than not to be inapplicable to
"diff", so combining the implementations doesn't make maintenance any
easier.
Now, I would be willing to accept code to factor out, say, the 13 lines
of code which check for a valid versus_url. That would apply to
link_path as well as switch_cmd and diff.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: "serve.c" and "text_deltas" parameter of svn_repos_begin_report
Posted by Nuutti Kotivuori <na...@iki.fi>.
Julian Foad wrote:
> 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.
No, actually the "problem" is at your end. You do have the
format=flowed specifier, and my mailer, for example, does honor it.
What happens is that your patch _is_ wrapped at 80 characters - but
it's wrapped with format=flowed (eg. with a space at the end of the
line), which means that:
- anyone who doesn't support format=flowed is just going to see a
broken patch wrapped at 80 characters.
- anyone who does support format=flowed is going to see the lines
wrapped at whatever they have specified their display- or
wrap-width. For me this is a hard cut at 80 characters, even though
my window is wider.
Both are incorrect behaviour, since we are not dealing with a
paragraph, but a single line which is really over 80 characters long -
and hence should be sent as such. Brane's hint about using a
preformatted paragraph is probably quite a good solution, I haven't
tested that myself - as is attaching the diffs.
-- Naked
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: "serve.c" and "text_deltas" parameter of svn_repos_begin_report
Posted by Branko Čibej <br...@xbc.nu>.
Julian Foad wrote:
> 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.
No, as a workaround you should paste the patches into a "preformatted"
paragraph in Mozilla, then it won't generate the spaces for flowed
format. That's what I always do, and my patches don't get wrapped.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: "serve.c" and "text_deltas" parameter of svn_repos_begin_report
Posted by Julian Foad <ju...@btopenworld.com>.
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.
Re: "serve.c" and "text_deltas" parameter of svn_repos_begin_report
Posted by Michael Wood <mw...@its.uct.ac.za>.
On Mon, Feb 03, 2003 at 10:36:43PM +0000, Julian Foad wrote:
[snip]
>
> 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,
[snip]
Your patch is wrapped.
--
Michael Wood <mw...@its.uct.ac.za>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org