You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Karl Fogel <kf...@red-bean.com> on 2007/05/08 16:38:40 UTC
[PATCH] distinguish requested depth from reported depth
Vlad,
Mike Pilato and I wrote a patch last night to restore the distinction
between requested depth and reported depth. It passes 'make check',
but could you give it a review?
Note that we'll still need code to actually use the requested depth
when present (if it's absent, i.e., svn_depth_unknown, then this is
not the "upgrade" case and we just use the reported per-path depths as
our guide). But that's a separate change. This just lays the
groundwork for that.
-Karl
[[[
With cmpilato, restore the distinction between requested depth and
reported depth, as discussed in this mail and the thread around it:
http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=1267
From: Vlad Georgescu <vg...@gmail.com>
To: dev@subversion.tigris.org
Subject: Re: [PATCH] Make sparse-directories checkouts/updates \
work with old servers
Date: Fri, 04 May 2007 08:35:06 +0300
Message-ID: <46...@gmail.com>
This involved reverting portions of r24493 and r24468, and making some
additional changes:
* subversion/libsvn_ra_serf/update.c
(make_update_reporter): Send depth again.
* subversion/libsvn_ra_svn/client.c
(ra_svn_update, ra_svn_switch, ra_svn_status, ra_svn_diff): Same.
* subversion/libsvn_repos/reporter.c
(report_baton_t): Rename 'default_depth' to 'requested_depth', and
explain very clearly what it is, using small words, so you'll be
sure to understand.
(write_path_info): Just error if depth is svn_depth_unknown.
(svn_repos__begin_report): Rename 'default_depth' argument to 'depth',
assign it to b->requested_depth.
(svn_repos_set_path2, svn_repos_link_path2): Default to
svn_depth_infinity.
* subversion/libsvn_ra_svn/protocol
(update, switch, status, diff): Restore depth parameter.
* subversion/svnserve/serve.c
(set_path, link_path): Default depth to svn_depth_infinity again.
(update, switch_cmd, status, diff): Parse optional depth parameter again.
(accept_report): Change RECURSE parameter back to DEPTH.
* subversion/mod_dav_svn/reports/update.c
(dav_svn__update_report): Expect a "depth" element, possibly falling
back to "recurse".
* subversion/libsvn_ra_dav/fetch.c
(make_reporter): Always send requested depth.
]]]
Index: subversion/libsvn_ra_serf/update.c
===================================================================
--- subversion/libsvn_ra_serf/update.c (revision 24967)
+++ subversion/libsvn_ra_serf/update.c (working copy)
@@ -2402,8 +2402,7 @@
report->sess->bkt_alloc);
}
- /* Old servers still pay attention to "recursive" here (modern
- servers use the "depth" argument to link_path/set_path). */
+ /* Old servers know "recursive" but not "depth"; help them DTRT. */
if (depth == svn_depth_files || depth == svn_depth_empty)
{
svn_ra_serf__add_tag_buckets(report->buckets,
@@ -2411,6 +2410,11 @@
report->sess->bkt_alloc);
}
+ /* ### TODO(sd): Send depth as a word, not a number! */
+ svn_ra_serf__add_tag_buckets(report->buckets,
+ "S:depth", apr_itoa(pool, depth),
+ report->sess->bkt_alloc);
+
return SVN_NO_ERROR;
}
Index: subversion/mod_dav_svn/reports/update.c
===================================================================
--- subversion/mod_dav_svn/reports/update.c (revision 24967)
+++ subversion/mod_dav_svn/reports/update.c (working copy)
@@ -916,7 +916,8 @@
const dav_svn_repos *repos = resource->info->repos;
const char *target = "";
svn_boolean_t text_deltas = TRUE;
- svn_depth_t depth = svn_depth_infinity;
+ svn_depth_t depth = svn_depth_unknown;
+ svn_boolean_t saw_depth = FALSE;
svn_boolean_t resource_walk = FALSE;
svn_boolean_t ignore_ancestry = FALSE;
dav_svn__authz_read_baton arb;
@@ -1016,25 +1017,37 @@
return derr;
target = cdata;
}
- if (child->ns == ns && strcmp(child->name, "recursive") == 0)
+ if (child->ns == ns && strcmp(child->name, "depth") == 0)
{
cdata = dav_xml_get_cdata(child, resource->pool, 1);
if (! *cdata)
return malformed_element_error(child->name, resource->pool);
+ depth = svn_depth_from_word(cdata);
+ saw_depth = TRUE;
+ }
+ if ((child->ns == ns && strcmp(child->name, "recursive") == 0)
+ && (! saw_depth))
+ {
+ cdata = dav_xml_get_cdata(child, resource->pool, 1);
+ if (! *cdata)
+ return malformed_element_error(child->name, resource->pool);
if (strcmp(cdata, "no") == 0)
depth = svn_depth_files;
- /* "yes" is the default, so we do nothing special for it.
-
- Note that even modern, depth-aware clients still transmit
- "no" for "recursive" in the svn_depth_files case and the
+ else
+ depth = svn_depth_infinity;
+ /* Note that even modern, depth-aware clients still transmit
+ "no" for "recursive" (along with "files" for "depth") in
+ the svn_depth_files case, and transmit "no" in the
svn_depth_empty case. This is because they don't know if
they're talking to a depth-aware server or not, and they
- don't need to know -- all they have to do is transmit a
- "recursive" element here, and send depths in their
- set_path() calls, and the server will DTRT either way
- (although in the svn_depth_empty case, the client will
- still have some work to do in ignoring the files that
- come down). */
+ don't need to know -- all they have to do is transmit
+ both, and the server will DTRT either way (although in
+ the svn_depth_empty case, the client will still have some
+ work to do in ignoring the files that come down).
+
+ When both "depth" and "recursive" are sent, we don't
+ bother to check if they're mutually consistent, we just
+ let depth dominate. */
}
if (child->ns == ns && strcmp(child->name, "ignore-ancestry") == 0)
{
@@ -1188,10 +1201,6 @@
entry_counter++;
- /* Pre-1.5 clients specify a report-wide depth via a
- recurse flag when beginning the report. */
- depth = svn_depth_unknown;
-
while (this_attr)
{
if (! strcmp(this_attr->name, "rev"))
Index: subversion/libsvn_repos/reporter.c
===================================================================
--- subversion/libsvn_repos/reporter.c (revision 24967)
+++ subversion/libsvn_repos/reporter.c (working copy)
@@ -89,7 +89,13 @@
svn_revnum_t t_rev; /* Revnum which the edit will bring the wc to */
const char *t_path; /* FS path the edit will bring the wc to */
svn_boolean_t text_deltas; /* Whether to report text deltas */
- svn_depth_t default_depth; /* Default depth for set_path and link_path. */
+
+ /* If the client requested a specific depth, record it here; if the
+ client did not, then this is svn_depth_unknown, and the depth of
+ information transmitted from server to client will be governed
+ strictly by the path-associated depths recorded in the report. */
+ svn_depth_t requested_depth;
+
svn_boolean_t ignore_ancestry;
svn_boolean_t is_switch;
const svn_delta_editor_t *editor;
@@ -1044,7 +1050,7 @@
/* --- COLLECTING THE REPORT INFORMATION --- */
/* Record a report operation into the temporary file. Return an error
- if both DEPTH and B->default_depth is svn_depth_unknown. */
+ if DEPTH is svn_depth_unknown. */
static svn_error_t *
write_path_info(report_baton_t *b, const char *path, const char *lpath,
svn_revnum_t rev, svn_depth_t depth,
@@ -1054,16 +1060,10 @@
const char *lrep, *rrep, *drep, *ltrep, *rep;
if (depth == svn_depth_unknown)
- {
- depth = b->default_depth;
+ return svn_error_createf(SVN_ERR_REPOS_BAD_ARGS, NULL,
+ _("Unsupported report depth '%s'"),
+ svn_depth_to_word(depth));
- if (depth == svn_depth_unknown)
- return svn_error_createf(SVN_ERR_REPOS_BAD_ARGS, NULL,
- _("Unsupported report depth '%s' for "
- "path '%s'"),
- svn_depth_to_word(depth), path);
- }
-
/* Munge the path to be anchor-relative, so that we can use edit paths
as report paths. */
path = svn_path_join(b->s_operand, path, pool);
@@ -1104,8 +1104,7 @@
svn_boolean_t start_empty, const char *lock_token,
apr_pool_t *pool)
{
- return svn_repos_set_path3(baton, path, rev,
- ((report_baton_t *) baton)->default_depth,
+ return svn_repos_set_path3(baton, path, rev, svn_depth_infinity,
start_empty, lock_token, pool);
}
@@ -1131,8 +1130,7 @@
svn_revnum_t rev, svn_boolean_t start_empty,
const char *lock_token, apr_pool_t *pool)
{
- return svn_repos_link_path3(baton, path, link_path, rev,
- ((report_baton_t *) baton)->default_depth,
+ return svn_repos_link_path3(baton, path, link_path, rev, svn_depth_infinity,
start_empty, lock_token, pool);
}
@@ -1186,7 +1184,7 @@
const char *s_operand,
const char *switch_path,
svn_boolean_t text_deltas,
- svn_depth_t default_depth,
+ svn_depth_t depth,
svn_boolean_t ignore_ancestry,
const svn_delta_editor_t *editor,
void *edit_baton,
@@ -1207,7 +1205,7 @@
b->t_path = switch_path ? switch_path
: svn_path_join(fs_base, s_operand, pool);
b->text_deltas = text_deltas;
- b->default_depth = default_depth;
+ b->requested_depth = depth;
b->ignore_ancestry = ignore_ancestry;
b->is_switch = (switch_path != NULL);
b->editor = editor;
Index: subversion/libsvn_ra_svn/client.c
===================================================================
--- subversion/libsvn_ra_svn/client.c (revision 24967)
+++ subversion/libsvn_ra_svn/client.c (working copy)
@@ -1083,8 +1083,8 @@
svn_boolean_t recurse = SVN_DEPTH_TO_RECURSE(depth);
/* Tell the server we want to start an update. */
- SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "update", "(?r)cb", rev, target,
- recurse));
+ SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "update", "(?r)cbw", rev, target,
+ recurse, svn_depth_to_word(depth)));
SVN_ERR(handle_auth_request(sess_baton, pool));
/* Fetch a reporter for the caller to drive. The reporter will drive
@@ -1107,8 +1107,9 @@
svn_boolean_t recurse = SVN_DEPTH_TO_RECURSE(depth);
/* Tell the server we want to start a switch. */
- SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "switch", "(?r)cbc", rev,
- target, recurse, switch_url));
+ SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "switch", "(?r)cbcw", rev,
+ target, recurse, switch_url,
+ svn_depth_to_word(depth)));
SVN_ERR(handle_auth_request(sess_baton, pool));
/* Fetch a reporter for the caller to drive. The reporter will drive
@@ -1131,8 +1132,9 @@
svn_boolean_t recurse = SVN_DEPTH_TO_RECURSE(depth);
/* Tell the server we want to start a status operation. */
- SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "status", "cb(?r)",
- target, recurse, rev));
+ SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "status", "cb(?r)w",
+ target, recurse, rev,
+ svn_depth_to_word(depth)));
SVN_ERR(handle_auth_request(sess_baton, pool));
/* Fetch a reporter for the caller to drive. The reporter will drive
@@ -1158,9 +1160,10 @@
svn_boolean_t recurse = SVN_DEPTH_TO_RECURSE(depth);
/* Tell the server we want to start a diff. */
- SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "diff", "(?r)cbbcb", rev,
+ SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "diff", "(?r)cbbcbw", rev,
target, recurse, ignore_ancestry,
- versus_url, text_deltas));
+ versus_url, text_deltas,
+ svn_depth_to_word(depth)));
SVN_ERR(handle_auth_request(sess_baton, pool));
/* Fetch a reporter for the caller to drive. The reporter will drive
Index: subversion/libsvn_ra_svn/protocol
===================================================================
--- subversion/libsvn_ra_svn/protocol (revision 24967)
+++ subversion/libsvn_ra_svn/protocol (working copy)
@@ -302,7 +302,7 @@
returned. If rev is not specified, the youngest revision is used.
update
- params: ( [ rev:number ] target:string recurse:bool )
+ params: ( [ rev:number ] target:string recurse:bool ? depth:word)
Client switches to report command set.
Upon finish-report, server sends auth-request.
After auth exchange completes, server switches to editor command set.
@@ -310,7 +310,8 @@
response: ( )
switch
- params: ( [ rev:number ] target:string recurse:bool url:string )
+ params: ( [ rev:number ] target:string recurse:bool url:string
+ ? depth:word)
Client switches to report command set.
Upon finish-report, server sends auth-request.
After auth exchange completes, server switches to editor command set.
@@ -318,7 +319,7 @@
response: ( )
status
- params: ( target:string recurse:bool ? [ rev:number ] )
+ params: ( target:string recurse:bool ? [ rev:number ] ? depth:word )
Client switches to report command set.
Upon finish-report, server sends auth-request.
After auth exchange completes, server switches to editor command set.
@@ -327,7 +328,7 @@
diff
params: ( [ rev:number ] target:string recurse:bool ignore-ancestry:bool
- url:string ? text-deltas:bool )
+ url:string ? text-deltas:bool ? depth:word )
Client switches to report command set.
Upon finish-report, server sends auth-request.
After auth exchange completes, server switches to editor command set.
Index: subversion/libsvn_ra_dav/fetch.c
===================================================================
--- subversion/libsvn_ra_dav/fetch.c (revision 24967)
+++ subversion/libsvn_ra_dav/fetch.c (working copy)
@@ -3105,8 +3105,7 @@
SVN_ERR(svn_io_file_write_full(rb->tmpfile, s, strlen(s), NULL, pool));
}
- /* Old servers still pay attention to "recursive" here (modern
- servers use the "depth" argument to link_path/set_path). */
+ /* Old servers know "recursive" but not "depth"; help them DTRT. */
if (depth == svn_depth_files || depth == svn_depth_empty)
{
const char *data = "<S:recursive>no</S:recursive>" DEBUG_CR;
@@ -3114,6 +3113,13 @@
NULL, pool));
}
+ /* mod_dav_svn defaults to svn_depth_infinity, but we always send anyway. */
+ {
+ s = apr_psprintf(pool, "<S:depth>%s</S:depth>" DEBUG_CR,
+ svn_depth_to_word(depth));
+ SVN_ERR(svn_io_file_write_full(rb->tmpfile, s, strlen(s), NULL, pool));
+ }
+
/* mod_dav_svn will use ancestry in diffs unless it finds this element. */
if (ignore_ancestry)
{
Index: subversion/svnserve/serve.c
===================================================================
--- subversion/svnserve/serve.c (revision 24967)
+++ subversion/svnserve/serve.c (working copy)
@@ -45,7 +45,6 @@
#include "server.h"
-
typedef struct {
apr_pool_t *pool;
svn_revnum_t *new_rev;
@@ -517,9 +516,8 @@
report_driver_baton_t *b = baton;
const char *path, *lock_token, *depth_word;
svn_revnum_t rev;
- /* Pre-1.5 clients specify a report-wide depth via a recurse flag
- when beginning the report. */
- svn_depth_t depth = svn_depth_unknown;
+ /* Default to infinity, for old clients that don't send depth. */
+ svn_depth_t depth = svn_depth_infinity;
svn_boolean_t start_empty;
SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "crb?(?c)?w",
@@ -554,9 +552,8 @@
const char *path, *url, *lock_token, *fs_path, *depth_word;
svn_revnum_t rev;
svn_boolean_t start_empty;
- /* Pre-1.5 clients specify a report-wide depth via a recurse flag
- when beginning the report. */
- svn_depth_t depth = svn_depth_unknown;
+ /* Default to infinity, for old clients that don't send depth. */
+ svn_depth_t depth = svn_depth_infinity;
SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "ccrb?(?c)?w",
&path, &url, &rev, &start_empty,
@@ -613,7 +610,7 @@
server_baton_t *b, svn_revnum_t rev,
const char *target, const char *tgt_path,
svn_boolean_t text_deltas,
- svn_boolean_t recurse,
+ svn_depth_t depth,
svn_boolean_t ignore_ancestry)
{
const svn_delta_editor_t *editor;
@@ -626,9 +623,8 @@
svn_ra_svn_get_editor(&editor, &edit_baton, conn, pool, NULL, NULL);
SVN_CMD_ERR(svn_repos__begin_report(&report_baton, rev, b->repos,
b->fs_path->data, target, tgt_path,
- text_deltas,
- SVN_DEPTH_FROM_RECURSE(recurse),
- ignore_ancestry, editor, edit_baton,
+ text_deltas, depth, ignore_ancestry,
+ editor, edit_baton,
authz_check_access_cb_func(b),
b, pool));
@@ -1309,18 +1305,27 @@
{
server_baton_t *b = baton;
svn_revnum_t rev;
- const char *target;
+ const char *target, *depth_word;
svn_boolean_t recurse;
+ /* Default to unknown. Old clients won't send depth, but we'll
+ handle that by converting recurse if necessary. */
+ svn_depth_t depth = svn_depth_unknown;
/* Parse the arguments. */
- SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "(?r)cb", &rev, &target,
- &recurse));
+ SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "(?r)cb?w", &rev, &target,
+ &recurse, &depth_word));
target = svn_path_canonicalize(target, pool);
+
+ if (depth_word)
+ depth = svn_depth_from_word(depth_word);
+ else
+ depth = recurse ? svn_depth_infinity : svn_depth_empty;
+
SVN_ERR(trivial_auth_request(conn, pool, b));
if (!SVN_IS_VALID_REVNUM(rev))
SVN_CMD_ERR(svn_fs_youngest_rev(&rev, b->fs, pool));
- return accept_report(conn, pool, b, rev, target, NULL, TRUE, recurse, FALSE);
+ return accept_report(conn, pool, b, rev, target, NULL, TRUE, depth, FALSE);
}
static svn_error_t *switch_cmd(svn_ra_svn_conn_t *conn, apr_pool_t *pool,
@@ -1328,15 +1333,24 @@
{
server_baton_t *b = baton;
svn_revnum_t rev;
- const char *target;
+ const char *target, *depth_word;
const char *switch_url, *switch_path;
svn_boolean_t recurse;
+ /* Default to unknown. Old clients won't send depth, but we'll
+ handle that by converting recurse if necessary. */
+ svn_depth_t depth = svn_depth_unknown;
/* Parse the arguments. */
- SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "(?r)cbc", &rev, &target,
- &recurse, &switch_url));
+ SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "(?r)cbc?w", &rev, &target,
+ &recurse, &switch_url, &depth_word));
target = svn_path_canonicalize(target, pool);
switch_url = svn_path_canonicalize(switch_url, pool);
+
+ if (depth_word)
+ depth = svn_depth_from_word(depth_word);
+ else
+ depth = recurse ? svn_depth_infinity : svn_depth_empty;
+
SVN_ERR(trivial_auth_request(conn, pool, b));
if (!SVN_IS_VALID_REVNUM(rev))
SVN_CMD_ERR(svn_fs_youngest_rev(&rev, b->fs, pool));
@@ -1345,7 +1359,7 @@
&switch_path));
return accept_report(conn, pool, b, rev, target, switch_path, TRUE,
- recurse, TRUE);
+ depth, TRUE);
}
static svn_error_t *status(svn_ra_svn_conn_t *conn, apr_pool_t *pool,
@@ -1353,19 +1367,27 @@
{
server_baton_t *b = baton;
svn_revnum_t rev;
- const char *target;
+ const char *target, *depth_word;
svn_boolean_t recurse;
+ /* Default to unknown. Old clients won't send depth, but we'll
+ handle that by converting recurse if necessary. */
+ svn_depth_t depth = svn_depth_unknown;
/* Parse the arguments. */
- SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "cb?(?r)",
- &target, &recurse, &rev));
+ SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "cb?(?r)?w",
+ &target, &recurse, &rev, &depth_word));
target = svn_path_canonicalize(target, pool);
+
+ if (depth_word)
+ depth = svn_depth_from_word(depth_word);
+ else
+ depth = recurse ? svn_depth_infinity : svn_depth_empty;
+
SVN_ERR(trivial_auth_request(conn, pool, b));
if (!SVN_IS_VALID_REVNUM(rev))
SVN_CMD_ERR(svn_fs_youngest_rev(&rev, b->fs, pool));
- return accept_report(conn, pool, b, rev, target, NULL, FALSE, recurse,
- FALSE);
+ return accept_report(conn, pool, b, rev, target, NULL, FALSE, depth, FALSE);
}
static svn_error_t *diff(svn_ra_svn_conn_t *conn, apr_pool_t *pool,
@@ -1373,9 +1395,12 @@
{
server_baton_t *b = baton;
svn_revnum_t rev;
- const char *target, *versus_url, *versus_path;
+ const char *target, *versus_url, *versus_path, *depth_word;
svn_boolean_t recurse, ignore_ancestry;
svn_boolean_t text_deltas;
+ /* Default to unknown. Old clients won't send depth, but we'll
+ handle that by converting recurse if necessary. */
+ svn_depth_t depth = svn_depth_unknown;
/* Parse the arguments. */
if (params->nelts == 5)
@@ -1384,16 +1409,23 @@
SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "(?r)cbbc", &rev, &target,
&recurse, &ignore_ancestry, &versus_url));
text_deltas = TRUE;
+ depth_word = NULL;
}
else
{
- SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "(?r)cbbcb",
+ SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "(?r)cbbcb?w",
&rev, &target, &recurse,
&ignore_ancestry, &versus_url,
- &text_deltas));
+ &text_deltas, &depth_word));
}
target = svn_path_canonicalize(target, pool);
versus_url = svn_path_canonicalize(versus_url, pool);
+
+ if (depth_word)
+ depth = svn_depth_from_word(depth_word);
+ else
+ depth = recurse ? svn_depth_infinity : svn_depth_empty;
+
SVN_ERR(trivial_auth_request(conn, pool, b));
if (!SVN_IS_VALID_REVNUM(rev))
SVN_CMD_ERR(svn_fs_youngest_rev(&rev, b->fs, pool));
@@ -1402,7 +1434,7 @@
&versus_path));
return accept_report(conn, pool, b, rev, target, versus_path,
- text_deltas, recurse, ignore_ancestry);
+ text_deltas, depth, ignore_ancestry);
}
/* Regardless of whether a client's capabilities indicate an
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] distinguish requested depth from reported depth
Posted by Karl Fogel <kf...@red-bean.com>.
Vlad Georgescu <vg...@gmail.com> writes:
> So, just to make sure I understand, with this patch:
> - we always send depth
> - requested depth is never svn_depth_unknown
Oh, the requested depth *can* be svn_depth_unknown, and we need to
allow that to reach the server. If it is unknown, then the server
just uses the reported depths for the paths available (or defaults to
whatever, if an old client didn't report any depths, and didn't
request a recursion level either).
> And the plan is:
> - only send depth when wc depth != the depth requested by the user
The plan is to send requested depth whenever the user explicitly
requests one, and not otherwise (or, maybe, send svn_depth_unknown,
but either way it should mean the same thing). Does that sound sane?
> - on the server side, if the client didn't send a depth, set requested
> depth to svn_depth_unknown (but if we got recurse=false, set requested
> depth to svn_depth_files, to accommodate old clients)
Right.
> The patch itself looks good, just one minor comment below:
>
> [...]
>
> If we wanted to save a few lines of code, we could have accept_report
> take both a depth and a boolean recurse, and do the necessary
> conversions in one place.
I think that would be tricky, because right now the conditionals are
examining the 'depth_word' variable to determine whether or not to use
the value of 'recurse'. accept_report() doesn't have access to that
variable, and I think it would be a kind of layer violation if it did.
Anyway, resolving the recurse<->depth question very soon after the
arrival of the protocol elements seems wise to me -- the further down
the code stack the duality is perpetuated, the more likely we are to
get into trouble later :-).
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] distinguish requested depth from reported depth
Posted by Vlad Georgescu <vg...@gmail.com>.
Karl Fogel wrote:
> Vlad,
>
> Mike Pilato and I wrote a patch last night to restore the distinction
> between requested depth and reported depth. It passes 'make check',
> but could you give it a review?
>
> Note that we'll still need code to actually use the requested depth
> when present (if it's absent, i.e., svn_depth_unknown, then this is
> not the "upgrade" case and we just use the reported per-path depths as
> our guide). But that's a separate change. This just lays the
> groundwork for that.
>
So, just to make sure I understand, with this patch:
- we always send depth
- requested depth is never svn_depth_unknown
And the plan is:
- only send depth when wc depth != the depth requested by the user
- on the server side, if the client didn't send a depth, set requested
depth to svn_depth_unknown (but if we got recurse=false, set requested
depth to svn_depth_files, to accommodate old clients)
The patch itself looks good, just one minor comment below:
>
> [[[
> With cmpilato, restore the distinction between requested depth and
> reported depth, as discussed in this mail and the thread around it:
>
> http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=1267
> From: Vlad Georgescu <vg...@gmail.com>
> To: dev@subversion.tigris.org
> Subject: Re: [PATCH] Make sparse-directories checkouts/updates \
> work with old servers
> Date: Fri, 04 May 2007 08:35:06 +0300
> Message-ID: <46...@gmail.com>
>
> This involved reverting portions of r24493 and r24468, and making some
> additional changes:
>
> * subversion/libsvn_ra_serf/update.c
> (make_update_reporter): Send depth again.
>
> * subversion/libsvn_ra_svn/client.c
> (ra_svn_update, ra_svn_switch, ra_svn_status, ra_svn_diff): Same.
>
> * subversion/libsvn_repos/reporter.c
> (report_baton_t): Rename 'default_depth' to 'requested_depth', and
> explain very clearly what it is, using small words, so you'll be
> sure to understand.
> (write_path_info): Just error if depth is svn_depth_unknown.
> (svn_repos__begin_report): Rename 'default_depth' argument to 'depth',
> assign it to b->requested_depth.
> (svn_repos_set_path2, svn_repos_link_path2): Default to
> svn_depth_infinity.
>
> * subversion/libsvn_ra_svn/protocol
> (update, switch, status, diff): Restore depth parameter.
>
> * subversion/svnserve/serve.c
> (set_path, link_path): Default depth to svn_depth_infinity again.
> (update, switch_cmd, status, diff): Parse optional depth parameter again.
> (accept_report): Change RECURSE parameter back to DEPTH.
>
> * subversion/mod_dav_svn/reports/update.c
> (dav_svn__update_report): Expect a "depth" element, possibly falling
> back to "recurse".
>
> * subversion/libsvn_ra_dav/fetch.c
> (make_reporter): Always send requested depth.
> ]]]
[...]
> @@ -613,7 +610,7 @@
> server_baton_t *b, svn_revnum_t rev,
> const char *target, const char *tgt_path,
> svn_boolean_t text_deltas,
> - svn_boolean_t recurse,
> + svn_depth_t depth,
> svn_boolean_t ignore_ancestry)
> {
> const svn_delta_editor_t *editor;
> @@ -626,9 +623,8 @@
> svn_ra_svn_get_editor(&editor, &edit_baton, conn, pool, NULL, NULL);
> SVN_CMD_ERR(svn_repos__begin_report(&report_baton, rev, b->repos,
> b->fs_path->data, target, tgt_path,
> - text_deltas,
> - SVN_DEPTH_FROM_RECURSE(recurse),
> - ignore_ancestry, editor, edit_baton,
> + text_deltas, depth, ignore_ancestry,
> + editor, edit_baton,
> authz_check_access_cb_func(b),
> b, pool));
>
> @@ -1309,18 +1305,27 @@
> {
> server_baton_t *b = baton;
> svn_revnum_t rev;
> - const char *target;
> + const char *target, *depth_word;
> svn_boolean_t recurse;
> + /* Default to unknown. Old clients won't send depth, but we'll
> + handle that by converting recurse if necessary. */
> + svn_depth_t depth = svn_depth_unknown;
>
> /* Parse the arguments. */
> - SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "(?r)cb", &rev, &target,
> - &recurse));
> + SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "(?r)cb?w", &rev, &target,
> + &recurse, &depth_word));
> target = svn_path_canonicalize(target, pool);
> +
> + if (depth_word)
> + depth = svn_depth_from_word(depth_word);
> + else
> + depth = recurse ? svn_depth_infinity : svn_depth_empty;
> +
> SVN_ERR(trivial_auth_request(conn, pool, b));
> if (!SVN_IS_VALID_REVNUM(rev))
> SVN_CMD_ERR(svn_fs_youngest_rev(&rev, b->fs, pool));
>
> - return accept_report(conn, pool, b, rev, target, NULL, TRUE, recurse, FALSE);
> + return accept_report(conn, pool, b, rev, target, NULL, TRUE, depth, FALSE);
> }
>
> static svn_error_t *switch_cmd(svn_ra_svn_conn_t *conn, apr_pool_t *pool,
> @@ -1328,15 +1333,24 @@
> {
> server_baton_t *b = baton;
> svn_revnum_t rev;
> - const char *target;
> + const char *target, *depth_word;
> const char *switch_url, *switch_path;
> svn_boolean_t recurse;
> + /* Default to unknown. Old clients won't send depth, but we'll
> + handle that by converting recurse if necessary. */
> + svn_depth_t depth = svn_depth_unknown;
>
> /* Parse the arguments. */
> - SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "(?r)cbc", &rev, &target,
> - &recurse, &switch_url));
> + SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "(?r)cbc?w", &rev, &target,
> + &recurse, &switch_url, &depth_word));
> target = svn_path_canonicalize(target, pool);
> switch_url = svn_path_canonicalize(switch_url, pool);
> +
> + if (depth_word)
> + depth = svn_depth_from_word(depth_word);
> + else
> + depth = recurse ? svn_depth_infinity : svn_depth_empty;
> +
> SVN_ERR(trivial_auth_request(conn, pool, b));
> if (!SVN_IS_VALID_REVNUM(rev))
> SVN_CMD_ERR(svn_fs_youngest_rev(&rev, b->fs, pool));
> @@ -1345,7 +1359,7 @@
> &switch_path));
>
> return accept_report(conn, pool, b, rev, target, switch_path, TRUE,
> - recurse, TRUE);
> + depth, TRUE);
> }
[...]
If we wanted to save a few lines of code, we could have accept_report
take both a depth and a boolean recurse, and do the necessary
conversions in one place.
--
Vlad
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org