Re: [PATCH] distinguish requested depth from reported depth

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 <vgeorgescu@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: <463AC60A.3040103@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