You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Vlad Georgescu <vg...@gmail.com> on 2007/03/24 13:56:37 UTC
[PATCH] Make sparse-directories checkouts/updates work with old servers
Hi,
This patch makes checkouts and updates at svn_depth_empty and
svn_depth_immediates work with servers that do not understand the new
reporter changes, by making the update editor ignore edits on paths below
the edit depth.
With this patch, I can successfully run the depth tests against a 1.4
svnserve. As this is my first attempt at sparse-directories related work,
I'm posting the patch here for review.
[[[
Make updates and checkouts at 'empty' or 'immediates' depth work with old
servers.
* subversion/include/svn_types.h
(svn_depth_t): Remove comment about svn_depth_exclude being potentially
unnecessary.
* subversion/libsvn_ra_svn/client.c
(ra_svn_update): Don't use the SVN_DEPTH_TO_RECURSE macro, because we
don't want svn_depth_immediates mapped to FALSE in this case. Use a
custom check instead.
* subversion/libsvn_wc/update_editor.c
(dir_baton): New member 'depth'.
(file_baton.skipped): Update comment to reflect new usage.
(prep_directory): Remove 'depth' parameter, it is stored in the dir_baton
now. Use db->depth instead of depth when creating the administrative area.
(open_root): Initialize the returned dir_baton's depth to the default
depth of the edit.
(delete_entry): For svn_depth_empty and svn_depth_files, ignore deletions
of children that haven't been explicitly brought in.
(add_directory): If the directory is the target of the edit, set its
depth to that of the edit. Otherwise, initialize the returned dir_baton's
depth based on the depth of the parent. Remove the old depth calculation
code, and update the call to prep_directory.
(open_directory): If the directory is the target of the edit, set its
depth to that of the edit. Otherwise, initialize the returned dir_baton's
depth based on the depth of the parent. If the entry exists on disk, allow
its depth to override svn_depth_infinity.
(change_dir_prop, close_directory): Don't do anything if the directory is
at svn_depth_exclude.
(absent_file_or_dir): Don't do anything if the parent is empty or excluded,
or if it is at svn_depth_files and the absent entry is a directory.
(add_or_open_file): If parent is at svn_depth_empty or svn_depth_exclude,
skip this file.
]]]
--
Vlad
Re: [PATCH] Make sparse-directories checkouts/updates work with old servers
Posted by Karl Fogel <kf...@red-bean.com>.
Vlad Georgescu <vg...@gmail.com> writes:
> This patch makes checkouts and updates at svn_depth_empty and
> svn_depth_immediates work with servers that do not understand the new
> reporter changes, by making the update editor ignore edits on paths below
> the edit depth.
>
> With this patch, I can successfully run the depth tests against a 1.4
> svnserve. As this is my first attempt at sparse-directories related work,
> I'm posting the patch here for review.
Thanks! I'll take a look as soon as I can (I've been busy with
http://pastebin.ca/408580, that's all.) But hopefully others can
review this patch too; a lot of people here understand the
sparse-directories changes now, which is good.
-Karl
> [[[
> Make updates and checkouts at 'empty' or 'immediates' depth work with old
> servers.
>
> * subversion/include/svn_types.h
> (svn_depth_t): Remove comment about svn_depth_exclude being potentially
> unnecessary.
>
> * subversion/libsvn_ra_svn/client.c
> (ra_svn_update): Don't use the SVN_DEPTH_TO_RECURSE macro, because we
> don't want svn_depth_immediates mapped to FALSE in this case. Use a
> custom check instead.
>
> * subversion/libsvn_wc/update_editor.c
> (dir_baton): New member 'depth'.
> (file_baton.skipped): Update comment to reflect new usage.
> (prep_directory): Remove 'depth' parameter, it is stored in the dir_baton
> now. Use db->depth instead of depth when creating the administrative area.
> (open_root): Initialize the returned dir_baton's depth to the default
> depth of the edit.
> (delete_entry): For svn_depth_empty and svn_depth_files, ignore deletions
> of children that haven't been explicitly brought in.
> (add_directory): If the directory is the target of the edit, set its
> depth to that of the edit. Otherwise, initialize the returned dir_baton's
> depth based on the depth of the parent. Remove the old depth calculation
> code, and update the call to prep_directory.
> (open_directory): If the directory is the target of the edit, set its
> depth to that of the edit. Otherwise, initialize the returned dir_baton's
> depth based on the depth of the parent. If the entry exists on disk, allow
> its depth to override svn_depth_infinity.
> (change_dir_prop, close_directory): Don't do anything if the directory is
> at svn_depth_exclude.
> (absent_file_or_dir): Don't do anything if the parent is empty or excluded,
> or if it is at svn_depth_files and the absent entry is a directory.
> (add_or_open_file): If parent is at svn_depth_empty or svn_depth_exclude,
> skip this file.
> ]]]
>
> --
> Vlad
>
>
>
> Index: subversion/include/svn_types.h
> ===================================================================
> --- subversion/include/svn_types.h (revision 24077)
> +++ subversion/include/svn_types.h (working copy)
> @@ -212,8 +212,7 @@
> ### TODO(sd): This depth may turn out to be unnecessary. ### */
> svn_depth_unknown = -2,
>
> - /* Exclude (remove, whatever) directory D.
> - ### TODO(sd): This depth may turn out to be unnecessary. ### */
> + /* Exclude (remove, whatever) directory D. */
> svn_depth_exclude = -1,
>
> /* Just the named directory D, no entries. Updates will not pull in
> Index: subversion/libsvn_ra_svn/client.c
> ===================================================================
> --- subversion/libsvn_ra_svn/client.c (revision 24077)
> +++ subversion/libsvn_ra_svn/client.c (working copy)
> @@ -1079,8 +1079,14 @@
> {
> svn_ra_svn__session_baton_t *sess_baton = session->priv;
> svn_ra_svn_conn_t *conn = sess_baton->conn;
> - svn_boolean_t recurse = SVN_DEPTH_TO_RECURSE(depth);
> + svn_boolean_t recurse;
>
> + /* Old servers know "recursive" but not "depth"; help them DTRT. */
> + if (depth == svn_depth_empty || depth == svn_depth_files)
> + recurse = FALSE;
> + else
> + recurse = TRUE;
> +
> /* Tell the server we want to start an update. */
> SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "update", "(?r)cbw", rev, target,
> recurse, svn_depth_to_word(depth)));
> Index: subversion/libsvn_wc/update_editor.c
> ===================================================================
> --- subversion/libsvn_wc/update_editor.c (revision 24077)
> +++ subversion/libsvn_wc/update_editor.c (working copy)
> @@ -138,6 +138,9 @@
> /* The global edit baton. */
> struct edit_baton *edit_baton;
>
> + /* The depth of the directory in this edit. */
> + svn_depth_t depth;
> +
> /* Baton for this directory's parent, or NULL if this is the root
> directory. */
> struct dir_baton *parent_baton;
> @@ -567,7 +570,9 @@
> /* Set if this file is new. */
> svn_boolean_t added;
>
> - /* Set if this file is skipped because it was in conflict. */
> + /* Set if this file is skipped because it was in conflict, or because
> + it was a child of a directory at svn_depth_empty or
> + svn_depth_exclude. */
> svn_boolean_t skipped;
>
> /* Set if an unversioned file of the same name already existed in
> @@ -701,7 +706,6 @@
>
>
> /* Prepare directory for dir_baton DB for updating or checking out.
> - * Give it depth DEPTH.
> *
> * If the path already exists, but is not a working copy for
> * ANCESTOR_URL and ANCESTOR_REVISION, then an error will be returned.
> @@ -710,7 +714,6 @@
> prep_directory(struct dir_baton *db,
> const char *ancestor_url,
> svn_revnum_t ancestor_revision,
> - svn_depth_t depth,
> apr_pool_t *pool)
> {
> const char *repos;
> @@ -730,7 +733,7 @@
> or by checking that it is so already. */
> SVN_ERR(svn_wc_ensure_adm3(db->path, NULL,
> ancestor_url, repos,
> - ancestor_revision, depth, pool));
> + ancestor_revision, db->depth, pool));
>
> if (! db->edit_baton->adm_access
> || strcmp(svn_wc_adm_access_path(db->edit_baton->adm_access),
> @@ -898,6 +901,7 @@
> eb->root_opened = TRUE;
>
> *dir_baton = d = make_dir_baton(NULL, eb, NULL, FALSE, pool);
> + d->depth = eb->depth;
> if (! *eb->target)
> {
> /* For an update with a NULL target, this is equivalent to open_dir(): */
> @@ -1108,6 +1112,24 @@
> apr_pool_t *pool)
> {
> struct dir_baton *pb = parent_baton;
> +
> + if (pb->depth == svn_depth_exclude)
> + return SVN_NO_ERROR;
> +
> + /* For svn_depth_empty and svn_depth_files, ignore deletions of entries
> + that haven't been explicitly brought in. */
> + else if (pb->depth == svn_depth_empty || pb->depth == svn_depth_files)
> + {
> + svn_wc_adm_access_t *adm_access;
> + svn_wc_entry_t *entry;
> + const char *full_path = svn_path_join(pb->edit_baton->anchor, path, pool);
> +
> + SVN_ERR(svn_wc_entry(&entry, full_path,
> + pb->edit_baton->adm_access, FALSE, pool));
> + if (! entry)
> + return SVN_NO_ERROR;
> + }
> +
> return do_entry_deletion(pb->edit_baton, pb->path, path, &pb->log_number,
> pool);
> }
> @@ -1135,6 +1157,26 @@
> || ((! copyfrom_path) && (SVN_IS_VALID_REVNUM(copyfrom_revision))))
> abort();
>
> + *child_baton = db;
> +
> + /* If we have a non-empty target and the parent of the newly added
> + directory is the root of the edit, then the new directory *is* the
> + target and we should set its depth accordingly. */
> + if (*eb->target && ! pb->parent_baton)
> + db->depth = eb->depth;
> + else if (pb->depth == svn_depth_empty
> + || pb->depth == svn_depth_exclude
> + || pb->depth == svn_depth_files)
> + {
> + db->depth = svn_depth_exclude;
> + db->bump_info->skipped = TRUE;
> + return SVN_NO_ERROR;
> + }
> + else if (pb->depth == svn_depth_immediates)
> + db->depth = svn_depth_empty;
> + else
> + db->depth = svn_depth_infinity;
> +
> SVN_ERR(svn_io_check_path(db->path, &kind, db->pool));
>
> /* The path can exist, but it must be a directory... */
> @@ -1287,23 +1329,10 @@
> }
> }
>
> - {
> - svn_depth_t depth = svn_depth_infinity;
> + SVN_ERR(prep_directory(db, db->new_URL,
> + *(eb->target_revision),
> + db->pool));
>
> - if (eb->depth == svn_depth_empty
> - || eb->depth == svn_depth_files
> - || eb->depth == svn_depth_immediates)
> - depth = svn_depth_empty;
> -
> - SVN_ERR(prep_directory(db,
> - db->new_URL,
> - *(eb->target_revision),
> - depth,
> - db->pool));
> - }
> -
> - *child_baton = db;
> -
> /* If this add was obstructed by dir scheduled for addition without
> history let close_file() handle the notification because there
> might be properties to deal with. */
> @@ -1350,6 +1379,26 @@
>
> /* Skip this directory if it has property conflicts. */
> SVN_ERR(svn_wc_entry(&entry, db->path, eb->adm_access, FALSE, pool));
> +
> + /* If we have a non-empty target and the parent of the newly added
> + directory is the root of the edit, then the new directory *is* the
> + target and we should set its depth accordingly. */
> + if (*eb->target && ! pb->parent_baton)
> + db->depth = eb->depth;
> + else if (pb->depth == svn_depth_empty
> + || pb->depth == svn_depth_files
> + || pb->depth == svn_depth_exclude)
> + {
> + db->depth = svn_depth_exclude;
> + db->bump_info->skipped = TRUE;
> + return SVN_NO_ERROR;
> + }
> + else if (pb->depth == svn_depth_immediates)
> + db->depth = svn_depth_empty;
> + else if (entry)
> + /* Allow the entry's actual depth to override svn_depth_infinity. */
> + db->depth = entry->depth;
> +
> if (entry)
> {
> /* Text conflicts can't happen for a directory, but we need to supply
> @@ -1411,7 +1460,7 @@
> svn_prop_t *propchange;
> struct dir_baton *db = dir_baton;
>
> - if (db->bump_info->skipped)
> + if (db->bump_info->skipped || db->depth == svn_depth_exclude)
> return SVN_NO_ERROR;
>
> propchange = apr_array_push(db->propchanges);
> @@ -1451,6 +1500,12 @@
> apr_array_header_t *entry_props, *wc_props, *regular_props;
> svn_wc_adm_access_t *adm_access;
>
> + if (db->depth == svn_depth_exclude)
> + {
> + SVN_ERR(maybe_bump_dir_info(db->edit_baton, db->bump_info, db->pool));
> + return SVN_NO_ERROR;
> + }
> +
> SVN_ERR(svn_categorize_props(db->propchanges, &entry_props, &wc_props,
> ®ular_props, pool));
>
> @@ -1578,6 +1633,11 @@
> const svn_wc_entry_t *ent;
> svn_wc_entry_t tmp_entry;
>
> + if (pb->depth == svn_depth_empty
> + || pb->depth == svn_depth_exclude
> + || (kind == svn_node_dir && pb->depth == svn_depth_files))
> + return SVN_NO_ERROR;
> +
> /* Extra check: an item by this name may not exist, but there may
> still be one scheduled for addition. That's a genuine
> tree-conflict. */
> @@ -1663,8 +1723,14 @@
> conflict in the entry and proceed. Similarly if it has changed
> kind. see issuezilla task #398. */
>
> - fb = make_file_baton(pb, path, adding, pool);
> + *file_baton = fb = make_file_baton(pb, path, adding, pool);
>
> + if (pb->depth == svn_depth_empty || pb->depth == svn_depth_exclude)
> + {
> + fb->skipped = TRUE;
> + return SVN_NO_ERROR;
> + }
> +
> /* It is interesting to note: everything below is just validation. We
> aren't actually doing any "work" or fetching any persistent data. */
>
> @@ -1773,7 +1839,6 @@
>
> svn_pool_destroy(subpool);
>
> - *file_baton = fb;
> return SVN_NO_ERROR;
> }
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Make sparse-directories checkouts/updates work with old servers
Posted by Karl Fogel <kf...@red-bean.com>.
Vlad Georgescu <vg...@gmail.com> writes:
> Hmm, I'm not sure it's something as simple as a bugfix. Suppose we
> checkout a greek tree at depth=empty, and then do a depth=infinity
> update. How is the (1.5+) server supposed to know it should send A/ and
> iota? All it sees is the top-level set-path with a depth of infinity,
> the same as a fully complete working copy might send.
Why would the top-level set_path() send depth=infinity? Wouldn't it
send depth=empty? I thought the whole point of the depth parameter to
set_path() was to accurately describe the working copy state.
The *desired* depth, infinity, should be sent by other means. If
we've conflated those two things, then we've made a mistake.
...and, I think maybe we have made that mistake, haven't we? :-)
> With old servers it gets more complicated. You'd probably have to call
> delete-path on each file or directory that's missing, except that a
> working copy doesn't actually have that information.
>
> And, of course, whatever solution we come up with must simultaneously
> work for both old and new servers, because we don't currently
> distinguish between them.
Ugh. This is a deep problem. I'm pondering.
---------------------------------------------------------------------
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
[PATCH] distinguish requested depth from reported depth
Posted by Karl Fogel <kf...@red-bean.com>.
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] Make sparse-directories checkouts/updates work with old
servers
Posted by Vlad Georgescu <vg...@gmail.com>.
Karl Fogel wrote:
> What if we use the 'start_empty' flag to set_path() and link_path()?
> Is that the inverse we want, that is, like saying to the server
> "assume all entries have been deleted, except for the ones I tell you
> I have"? Specifically, I mean:
>
> When the working directory has a depth other than svn_depth_infinity,
> *and* we're upgrading it to a deeper depth, then pass start_empty=true
> to reporter->set_path() / reporter->link_path(), and explicitly send
> any entries that are present in the working copy.
Yeah, that should work.
> If there's some way we can know that the server is 1.5 or higher, then
> we can just send working depth, and not bother with start_empty=true
> and with sending all the entries. But I don't know if enough of a
> feature exchange will have taken place by the time we're talking to
> the reporter.
I think trying to do different things depending on the server version
might be painful. Even if we knew whether the server has the required
version/capability when starting a report, I don't see a good way to get
that information from svn_ra_do_update and into svn_wc_crawl_revisions.
--
Vlad
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Make sparse-directories checkouts/updates work with old servers
Posted by Karl Fogel <kf...@red-bean.com>.
Vlad Georgescu <vg...@gmail.com> writes:
>> I think this means that r24493, r24468, and r24453 were mistaken. We
>> should be transmitting a depth from client to server, and remembering
>> it on the server side as the *requested* depth, not confusing it with
>> the *described* (default) depth.
>>
>> Does this analysis sound right to you?
>
> Yes, that's pretty much what I was thinking.
I'm relieved to hear that :-).
> How does this support the "upgrade" scenario? If you want to upgrade a
> working copy from, e.g, svn_depth_files to svn_depth_infinity, you have
> to get the (old) server to send you the complete subdirectories, not
> just updates. That's why I suggested the need to call delete-path on
> the missing stuff (to mark the subdirectories as missing in the working
> copy, telling the server to send fulltexts, not deltas).
>
> The problem here is that the depth-files working copy doesn't know the
> names of the subdirectories that it's missing, so you wouldn't be able
> to call delete-path on them.
>
> Hope I'm making sense here, the caffeine hasn't kicked in yet :).
Yes, you're making sense.
What if we use the 'start_empty' flag to set_path() and link_path()?
Is that the inverse we want, that is, like saying to the server
"assume all entries have been deleted, except for the ones I tell you
I have"? Specifically, I mean:
When the working directory has a depth other than svn_depth_infinity,
*and* we're upgrading it to a deeper depth, then pass start_empty=true
to reporter->set_path() / reporter->link_path(), and explicitly send
any entries that are present in the working copy.
If there's some way we can know that the server is 1.5 or higher, then
we can just send working depth, and not bother with start_empty=true
and with sending all the entries. But I don't know if enough of a
feature exchange will have taken place by the time we're talking to
the reporter.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Make sparse-directories checkouts/updates work with old
servers
Posted by Vlad Georgescu <vg...@gmail.com>.
Karl Fogel wrote:
[...]
> It seems we have a problem here. The client needs to do two separate
> things:
>
> 1) Describe the working copy (including depth) to the server.
> 2) Tell the server what depth the client *wants* to have.
>
> Have we been carelessly confusing these two things?
>
> The reporter set_path() calls are about (1), and the 'depth' parameter
> to (for example) svn_ra_do_update2() is about (2).
>
> (I know you know all this, I'm just stating everything very carefully
> here to make sure my own understanding is correct!)
>
> Now, (2) is a single depth, sent exactly once during the update. It
> doesn't really matter if it's sent early or late in the request -- I
> lean toward early, because in some future full-duplex universe, that
> might allow the server to start sending back answers sooner.
>
> I think this means that r24493, r24468, and r24453 were mistaken. We
> should be transmitting a depth from client to server, and remembering
> it on the server side as the *requested* depth, not confusing it with
> the *described* (default) depth.
>
> Does this analysis sound right to you?
>
Yes, that's pretty much what I was thinking.
>> With old servers it gets more complicated. You'd probably have to call
>> delete-path on each file or directory that's missing, except that a
>> working copy doesn't actually have that information.
>
> I don't think that's necessary, we're just going to have the client
> ignore unwanted portions of the update (cf. Mike Pilato's recent
> patch to the RA layers).
>
How does this support the "upgrade" scenario? If you want to upgrade a
working copy from, e.g, svn_depth_files to svn_depth_infinity, you have
to get the (old) server to send you the complete subdirectories, not
just updates. That's why I suggested the need to call delete-path on
the missing stuff (to mark the subdirectories as missing in the working
copy, telling the server to send fulltexts, not deltas).
The problem here is that the depth-files working copy doesn't know the
names of the subdirectories that it's missing, so you wouldn't be able
to call delete-path on them.
Hope I'm making sense here, the caffeine hasn't kicked in yet :).
>> And, of course, whatever solution we come up with must simultaneously
>> work for both old and new servers, because we don't currently
>> distinguish between them.
>
> Oh, I think we're just going to have different solutions for the
> different situations.
>
> For old servers, the client sends a requested depth (which the server
> ignores) and per-path depth information (which the server also
> ignores), and then the client ignores extra information the server
> sends back.
>
> For new servers, the client doesn't need to ignore anything, because
> the server doesn't send any extra information anyway. But if it did,
> the client would just ignore it :-).
Sure, this works fine for "normal" updates, it's upgrades that I was
worried about...
--
Vlad
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Make sparse-directories checkouts/updates work with old servers
Posted by Karl Fogel <kf...@red-bean.com>.
Vlad Georgescu <vg...@gmail.com> writes:
> Hmm, I'm not sure it's something as simple as a bugfix. Suppose we
> checkout a greek tree at depth=empty, and then do a depth=infinity
> update. How is the (1.5+) server supposed to know it should send A/ and
> iota? All it sees is the top-level set-path with a depth of infinity,
> the same as a fully complete working copy might send.
Sorry it's taken me so long to come back to this thread; I drove to a
conference a thousand miles away, talked, and drove back :-).
It seems we have a problem here. The client needs to do two separate
things:
1) Describe the working copy (including depth) to the server.
2) Tell the server what depth the client *wants* to have.
Have we been carelessly confusing these two things?
The reporter set_path() calls are about (1), and the 'depth' parameter
to (for example) svn_ra_do_update2() is about (2).
(I know you know all this, I'm just stating everything very carefully
here to make sure my own understanding is correct!)
Now, (2) is a single depth, sent exactly once during the update. It
doesn't really matter if it's sent early or late in the request -- I
lean toward early, because in some future full-duplex universe, that
might allow the server to start sending back answers sooner.
I think this means that r24493, r24468, and r24453 were mistaken. We
should be transmitting a depth from client to server, and remembering
it on the server side as the *requested* depth, not confusing it with
the *described* (default) depth.
Does this analysis sound right to you?
> With old servers it gets more complicated. You'd probably have to call
> delete-path on each file or directory that's missing, except that a
> working copy doesn't actually have that information.
I don't think that's necessary, we're just going to have the client
ignore unwanted portions of the update (cf. Mike Pilato's recent
patch to the RA layers).
> And, of course, whatever solution we come up with must simultaneously
> work for both old and new servers, because we don't currently
> distinguish between them.
Oh, I think we're just going to have different solutions for the
different situations.
For old servers, the client sends a requested depth (which the server
ignores) and per-path depth information (which the server also
ignores), and then the client ignores extra information the server
sends back.
For new servers, the client doesn't need to ignore anything, because
the server doesn't send any extra information anyway. But if it did,
the client would just ignore it :-).
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Make sparse-directories checkouts/updates work with old
servers
Posted by Vlad Georgescu <vg...@gmail.com>.
Karl Fogel wrote:
> Vlad, I think this patch is still outstanding -- were you planning to
> post another iteration? (It sounded like you were, if not let me know.)
>
I was, but there's some stuff I need to figure out first (see below).
> To answer your question:
>
>> For example, what happens (or should happen) if we do a depth=infinity
>> update on a depth=empty wc? Do we:
>>
>> a) do a depth=empty update
>> b) bring in everything that's missing into the wc, effectively making it
>> depth=infinity
>> c) update only those files and directories in the wc that have been
>> brought in explicitly
>>
>> notes/sparse-directories.txt says b), but I don't think that's what the
>> code in trunk/ currently does.
>
> I'm pretty sure (b) is correct; if trunk needs a bugfix, then trunk
> needs a bugfix, but let's not let that stand in the way of your patch.
>
> -K
Hmm, I'm not sure it's something as simple as a bugfix. Suppose we
checkout a greek tree at depth=empty, and then do a depth=infinity
update. How is the (1.5+) server supposed to know it should send A/ and
iota? All it sees is the top-level set-path with a depth of infinity,
the same as a fully complete working copy might send.
With old servers it gets more complicated. You'd probably have to call
delete-path on each file or directory that's missing, except that a
working copy doesn't actually have that information.
And, of course, whatever solution we come up with must simultaneously
work for both old and new servers, because we don't currently
distinguish between them.
--
Vlad
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Make sparse-directories checkouts/updates work with old servers
Posted by Karl Fogel <kf...@red-bean.com>.
Vlad, I think this patch is still outstanding -- were you planning to
post another iteration? (It sounded like you were, if not let me know.)
To answer your question:
> For example, what happens (or should happen) if we do a depth=infinity
> update on a depth=empty wc? Do we:
>
> a) do a depth=empty update
> b) bring in everything that's missing into the wc, effectively making it
> depth=infinity
> c) update only those files and directories in the wc that have been
> brought in explicitly
>
> notes/sparse-directories.txt says b), but I don't think that's what the
> code in trunk/ currently does.
I'm pretty sure (b) is correct; if trunk needs a bugfix, then trunk
needs a bugfix, but let's not let that stand in the way of your patch.
-K
Vlad Georgescu <vg...@gmail.com> writes:
> Karl Fogel wrote:
> ...
>>> Index: subversion/libsvn_ra_svn/client.c
>>> ===================================================================
>>> --- subversion/libsvn_ra_svn/client.c (revision 24077)
>>> +++ subversion/libsvn_ra_svn/client.c (working copy)
>>> @@ -1079,8 +1079,14 @@
>>> {
>>> svn_ra_svn__session_baton_t *sess_baton = session->priv;
>>> svn_ra_svn_conn_t *conn = sess_baton->conn;
>>> - svn_boolean_t recurse = SVN_DEPTH_TO_RECURSE(depth);
>>> + svn_boolean_t recurse;
>>>
>>> + /* Old servers know "recursive" but not "depth"; help them DTRT. */
>>> + if (depth == svn_depth_empty || depth == svn_depth_files)
>>> + recurse = FALSE;
>>> + else
>>> + recurse = TRUE;
>>> +
>>> /* Tell the server we want to start an update. */
>>> SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "update", "(?r)cbw", rev, target,
>>> recurse, svn_depth_to_word(depth)));
>>
>> This raises an interesting question: should SVN_DEPTH_TO_RECURSE()
>> itself change to the behavior you have above? I surveyed its callers:
>> it looks like approximately half could use (or could easily be changed
>> to use) the behavior you wrote above, and about half need the current
>> behavior of SVN_DEPTH_TO_RECURSE().
>>
>> I'm not sure what to recommend here, I just wanted to raise this
>> issue. If you have time to take a look at that as a separate change,
>> even better.
>
> We could:
> a) introduce a new macro, or
> b) update all APIs which currently take a boolean recurse parameter, and
> which depend on the current behavior of SVN_DEPTH_TO_RECURSE, by making
> them take a svn_depth_t parameter. After that, we could change
> SVN_DEPTH_TO_RECURSE to the new behavior.
>
>>> Index: subversion/libsvn_wc/update_editor.c
>>> ===================================================================
>>> --- subversion/libsvn_wc/update_editor.c (revision 24077)
>>> +++ subversion/libsvn_wc/update_editor.c (working copy)
>>> @@ -1135,6 +1157,26 @@
>>> || ((! copyfrom_path) && (SVN_IS_VALID_REVNUM(copyfrom_revision))))
>>> abort();
>>>
>>> + *child_baton = db;
>>> +
>>> + /* If we have a non-empty target and the parent of the newly added
>>> + directory is the root of the edit, then the new directory *is* the
>>> + target and we should set its depth accordingly. */
>>> + if (*eb->target && ! pb->parent_baton)
>>> + db->depth = eb->depth;
>>> + else if (pb->depth == svn_depth_empty
>>> + || pb->depth == svn_depth_exclude
>>> + || pb->depth == svn_depth_files)
>>> + {
>>> + db->depth = svn_depth_exclude;
>>> + db->bump_info->skipped = TRUE;
>>> + return SVN_NO_ERROR;
>>> + }
>>> + else if (pb->depth == svn_depth_immediates)
>>> + db->depth = svn_depth_empty;
>>> + else
>>> + db->depth = svn_depth_infinity;
>>> +
>>> SVN_ERR(svn_io_check_path(db->path, &kind, db->pool));
>>
>> I feel silly saying this, but if one clause has braces, we usually
>> make sure they all do. I personally don't mind either way, but it
>> helps make the code more readable for a lot of people.
>
> Sure, I'll make these changes in the next iteration of the patch, or
> before I commit.
>
>> The logic here looks good to me: it's exactly the depth-shortening
>> effect I'd expected to see as we walk down the directory hierarchy.
>>
>>> @@ -1350,6 +1379,26 @@
>>>
>>> /* Skip this directory if it has property conflicts. */
>>> SVN_ERR(svn_wc_entry(&entry, db->path, eb->adm_access, FALSE, pool));
>>> +
>>> + /* If we have a non-empty target and the parent of the newly added
>>> + directory is the root of the edit, then the new directory *is* the
>>> + target and we should set its depth accordingly. */
>>> + if (*eb->target && ! pb->parent_baton)
>>> + db->depth = eb->depth;
>>> + else if (pb->depth == svn_depth_empty
>>> + || pb->depth == svn_depth_files
>>> + || pb->depth == svn_depth_exclude)
>>> + {
>>> + db->depth = svn_depth_exclude;
>>> + db->bump_info->skipped = TRUE;
>>> + return SVN_NO_ERROR;
>>> + }
>>> + else if (pb->depth == svn_depth_immediates)
>>> + db->depth = svn_depth_empty;
>>> + else if (entry)
>>> + /* Allow the entry's actual depth to override svn_depth_infinity. */
>>> + db->depth = entry->depth;
>>> +
>>
>> Ah, okay, I think I understand: if the operation's requested depth is
>> not svn_depth_infinity, then an entry->depth can't override it --
>> after all, if this were a new server, it wouldn't give us the
>> information we'd need to descend anyway. It's only because of the
>> old-server case that we have to ignore content coming down the pipe.
>> But if svn_depth_infinity, then we're getting exactly what we want
>> from the pipe, and the question is: what depth is the wc using?
>>
>> Is that a fair summary of the reasoning here?
>>
>> (Just trying to make sure I understand the code.)
>
> I wanted to get the same behavior from old servers that we get from new
> servers. From my (probably inaccurate) understanding of the code, I
> thought that if we have a svn_depth_infinity update and we hit a
> directory that is at a non-infinity depth, we don't recurse any further.
> Is this what currently happens?
>
> For example, what happens (or should happen) if we do a depth=infinity
> update on a depth=empty wc? Do we:
>
> a) do a depth=empty update
> b) bring in everything that's missing into the wc, effectively making it
> depth=infinity
> c) update only those files and directories in the wc that have been
> brought in explicitly
>
> notes/sparse-directories.txt says b), but I don't think that's what the
> code in trunk/ currently does.
>
>>
>>> @@ -1451,6 +1500,12 @@
>>> apr_array_header_t *entry_props, *wc_props, *regular_props;
>>> svn_wc_adm_access_t *adm_access;
>>>
>>> + if (db->depth == svn_depth_exclude)
>>> + {
>>> + SVN_ERR(maybe_bump_dir_info(db->edit_baton, db->bump_info, db->pool));
>>> + return SVN_NO_ERROR;
>>> + }
>>> +
>>> SVN_ERR(svn_categorize_props(db->propchanges, &entry_props, &wc_props,
>>> ®ular_props, pool));
>>>
>>
>> Hmmm. This is in close_directory(). Are you sure you didn't mean
>> svn_depth_empty intead of svn_depth_exclude?
>
> My intention was to make close_directory() a no-op for excluded (i.e
> ignored) directories (the call to maybe_bump_dir_info is still necessary
> to update a reference count). Empty directories still need the full
> treatment.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Make sparse-directories checkouts/updates work with old
servers
Posted by Vlad Georgescu <vg...@gmail.com>.
Karl Fogel wrote:
...
>> Index: subversion/libsvn_ra_svn/client.c
>> ===================================================================
>> --- subversion/libsvn_ra_svn/client.c (revision 24077)
>> +++ subversion/libsvn_ra_svn/client.c (working copy)
>> @@ -1079,8 +1079,14 @@
>> {
>> svn_ra_svn__session_baton_t *sess_baton = session->priv;
>> svn_ra_svn_conn_t *conn = sess_baton->conn;
>> - svn_boolean_t recurse = SVN_DEPTH_TO_RECURSE(depth);
>> + svn_boolean_t recurse;
>>
>> + /* Old servers know "recursive" but not "depth"; help them DTRT. */
>> + if (depth == svn_depth_empty || depth == svn_depth_files)
>> + recurse = FALSE;
>> + else
>> + recurse = TRUE;
>> +
>> /* Tell the server we want to start an update. */
>> SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "update", "(?r)cbw", rev, target,
>> recurse, svn_depth_to_word(depth)));
>
> This raises an interesting question: should SVN_DEPTH_TO_RECURSE()
> itself change to the behavior you have above? I surveyed its callers:
> it looks like approximately half could use (or could easily be changed
> to use) the behavior you wrote above, and about half need the current
> behavior of SVN_DEPTH_TO_RECURSE().
>
> I'm not sure what to recommend here, I just wanted to raise this
> issue. If you have time to take a look at that as a separate change,
> even better.
We could:
a) introduce a new macro, or
b) update all APIs which currently take a boolean recurse parameter, and
which depend on the current behavior of SVN_DEPTH_TO_RECURSE, by making
them take a svn_depth_t parameter. After that, we could change
SVN_DEPTH_TO_RECURSE to the new behavior.
>> Index: subversion/libsvn_wc/update_editor.c
>> ===================================================================
>> --- subversion/libsvn_wc/update_editor.c (revision 24077)
>> +++ subversion/libsvn_wc/update_editor.c (working copy)
>> @@ -1135,6 +1157,26 @@
>> || ((! copyfrom_path) && (SVN_IS_VALID_REVNUM(copyfrom_revision))))
>> abort();
>>
>> + *child_baton = db;
>> +
>> + /* If we have a non-empty target and the parent of the newly added
>> + directory is the root of the edit, then the new directory *is* the
>> + target and we should set its depth accordingly. */
>> + if (*eb->target && ! pb->parent_baton)
>> + db->depth = eb->depth;
>> + else if (pb->depth == svn_depth_empty
>> + || pb->depth == svn_depth_exclude
>> + || pb->depth == svn_depth_files)
>> + {
>> + db->depth = svn_depth_exclude;
>> + db->bump_info->skipped = TRUE;
>> + return SVN_NO_ERROR;
>> + }
>> + else if (pb->depth == svn_depth_immediates)
>> + db->depth = svn_depth_empty;
>> + else
>> + db->depth = svn_depth_infinity;
>> +
>> SVN_ERR(svn_io_check_path(db->path, &kind, db->pool));
>
> I feel silly saying this, but if one clause has braces, we usually
> make sure they all do. I personally don't mind either way, but it
> helps make the code more readable for a lot of people.
Sure, I'll make these changes in the next iteration of the patch, or
before I commit.
> The logic here looks good to me: it's exactly the depth-shortening
> effect I'd expected to see as we walk down the directory hierarchy.
>
>> @@ -1350,6 +1379,26 @@
>>
>> /* Skip this directory if it has property conflicts. */
>> SVN_ERR(svn_wc_entry(&entry, db->path, eb->adm_access, FALSE, pool));
>> +
>> + /* If we have a non-empty target and the parent of the newly added
>> + directory is the root of the edit, then the new directory *is* the
>> + target and we should set its depth accordingly. */
>> + if (*eb->target && ! pb->parent_baton)
>> + db->depth = eb->depth;
>> + else if (pb->depth == svn_depth_empty
>> + || pb->depth == svn_depth_files
>> + || pb->depth == svn_depth_exclude)
>> + {
>> + db->depth = svn_depth_exclude;
>> + db->bump_info->skipped = TRUE;
>> + return SVN_NO_ERROR;
>> + }
>> + else if (pb->depth == svn_depth_immediates)
>> + db->depth = svn_depth_empty;
>> + else if (entry)
>> + /* Allow the entry's actual depth to override svn_depth_infinity. */
>> + db->depth = entry->depth;
>> +
>
> Ah, okay, I think I understand: if the operation's requested depth is
> not svn_depth_infinity, then an entry->depth can't override it --
> after all, if this were a new server, it wouldn't give us the
> information we'd need to descend anyway. It's only because of the
> old-server case that we have to ignore content coming down the pipe.
> But if svn_depth_infinity, then we're getting exactly what we want
> from the pipe, and the question is: what depth is the wc using?
>
> Is that a fair summary of the reasoning here?
>
> (Just trying to make sure I understand the code.)
I wanted to get the same behavior from old servers that we get from new
servers. From my (probably inaccurate) understanding of the code, I
thought that if we have a svn_depth_infinity update and we hit a
directory that is at a non-infinity depth, we don't recurse any further.
Is this what currently happens?
For example, what happens (or should happen) if we do a depth=infinity
update on a depth=empty wc? Do we:
a) do a depth=empty update
b) bring in everything that's missing into the wc, effectively making it
depth=infinity
c) update only those files and directories in the wc that have been
brought in explicitly
notes/sparse-directories.txt says b), but I don't think that's what the
code in trunk/ currently does.
>
>> @@ -1451,6 +1500,12 @@
>> apr_array_header_t *entry_props, *wc_props, *regular_props;
>> svn_wc_adm_access_t *adm_access;
>>
>> + if (db->depth == svn_depth_exclude)
>> + {
>> + SVN_ERR(maybe_bump_dir_info(db->edit_baton, db->bump_info, db->pool));
>> + return SVN_NO_ERROR;
>> + }
>> +
>> SVN_ERR(svn_categorize_props(db->propchanges, &entry_props, &wc_props,
>> ®ular_props, pool));
>>
>
> Hmmm. This is in close_directory(). Are you sure you didn't mean
> svn_depth_empty intead of svn_depth_exclude?
My intention was to make close_directory() a no-op for excluded (i.e
ignored) directories (the call to maybe_bump_dir_info is still necessary
to update a reference count). Empty directories still need the full
treatment.
--
Vlad
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Make sparse-directories checkouts/updates work with old servers
Posted by Karl Fogel <kf...@red-bean.com>.
plundblad@google.com (Peter Lundblad) writes:
> Ah, I see, but still wouldn't test this patch because we expect new
> servers to DTRT, right?
Right -- in a normal testing setup, it wouldn't test Vlad's patch.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Make sparse-directories checkouts/updates work with old servers
Posted by Peter Lundblad <pl...@google.com>.
Karl Fogel writes:
> plundblad@google.com (Peter Lundblad) writes:
> >> Great! (Sanity check: is there definitely a test in there that tests
> >> the behavior we want?)
> >
> > How could there be? We normally test current client against current servers.
> > Sometimes, when people do compatibility work, they run tests with
> > mixed server/client, but we don't have anything automatic for that.
> > OTOH, if you test a new client against an old server, then the regular
> > depth tests should fail if this code doesn't do what's intended.
>
> I just meant a test that does (say) a depth-immediates checkout and
> verifies that it got exactly the right content.
Ah, I see, but still wouldn't test this patch because we expect new
servers to DTRT, right?
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Make sparse-directories checkouts/updates work with old servers
Posted by Karl Fogel <kf...@red-bean.com>.
plundblad@google.com (Peter Lundblad) writes:
>> Great! (Sanity check: is there definitely a test in there that tests
>> the behavior we want?)
>
> How could there be? We normally test current client against current servers.
> Sometimes, when people do compatibility work, they run tests with
> mixed server/client, but we don't have anything automatic for that.
> OTOH, if you test a new client against an old server, then the regular
> depth tests should fail if this code doesn't do what's intended.
I just meant a test that does (say) a depth-immediates checkout and
verifies that it got exactly the right content.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re:
Re: [PATCH] Make sparse-directories checkouts/updates work with old servers
Posted by Peter Lundblad <pl...@google.com>.
Karl Fogel writes:
> Vlad Georgescu <vg...@gmail.com> writes:
> > This patch makes checkouts and updates at svn_depth_empty and
> > svn_depth_immediates work with servers that do not understand the new
> > reporter changes, by making the update editor ignore edits on paths below
> > the edit depth.
> >
> > With this patch, I can successfully run the depth tests against a 1.4
> > svnserve. As this is my first attempt at sparse-directories related work,
> > I'm posting the patch here for review.
>
> Great! (Sanity check: is there definitely a test in there that tests
> the behavior we want?)
>
How could there be? We normally test current client against current servers.
Sometimes, when people do compatibility work, they run tests with
mixed server/client, but we don't have anything automatic for that.
OTOH, if you test a new client against an old server, then the regular
depth tests should fail if this code doesn't do what's intended.
I think it is a problem that we don't have automatic tests for our
client-server compatibility guarantees:( In this case, we could do with
some unittests for the update editor in libsvn_wc if someone has some
spare time...
thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Make sparse-directories checkouts/updates work with old servers
Posted by Karl Fogel <kf...@red-bean.com>.
Vlad Georgescu <vg...@gmail.com> writes:
> This patch makes checkouts and updates at svn_depth_empty and
> svn_depth_immediates work with servers that do not understand the new
> reporter changes, by making the update editor ignore edits on paths below
> the edit depth.
>
> With this patch, I can successfully run the depth tests against a 1.4
> svnserve. As this is my first attempt at sparse-directories related work,
> I'm posting the patch here for review.
Great! (Sanity check: is there definitely a test in there that tests
the behavior we want?)
> [[[
> Make updates and checkouts at 'empty' or 'immediates' depth work with old
> servers.
>
> * subversion/include/svn_types.h
> (svn_depth_t): Remove comment about svn_depth_exclude being potentially
> unnecessary.
>
> * subversion/libsvn_ra_svn/client.c
> (ra_svn_update): Don't use the SVN_DEPTH_TO_RECURSE macro, because we
> don't want svn_depth_immediates mapped to FALSE in this case. Use a
> custom check instead.
>
> * subversion/libsvn_wc/update_editor.c
> (dir_baton): New member 'depth'.
> (file_baton.skipped): Update comment to reflect new usage.
> (prep_directory): Remove 'depth' parameter, it is stored in the dir_baton
> now. Use db->depth instead of depth when creating the administrative area.
> (open_root): Initialize the returned dir_baton's depth to the default
> depth of the edit.
> (delete_entry): For svn_depth_empty and svn_depth_files, ignore deletions
> of children that haven't been explicitly brought in.
> (add_directory): If the directory is the target of the edit, set its
> depth to that of the edit. Otherwise, initialize the returned dir_baton's
> depth based on the depth of the parent. Remove the old depth calculation
> code, and update the call to prep_directory.
> (open_directory): If the directory is the target of the edit, set its
> depth to that of the edit. Otherwise, initialize the returned dir_baton's
> depth based on the depth of the parent. If the entry exists on disk, allow
> its depth to override svn_depth_infinity.
> (change_dir_prop, close_directory): Don't do anything if the directory is
> at svn_depth_exclude.
> (absent_file_or_dir): Don't do anything if the parent is empty or excluded,
> or if it is at svn_depth_files and the absent entry is a directory.
> (add_or_open_file): If parent is at svn_depth_empty or svn_depth_exclude,
> skip this file.
> ]]]
*nod* This all looks like the right approach to me.
> Index: subversion/libsvn_ra_svn/client.c
> ===================================================================
> --- subversion/libsvn_ra_svn/client.c (revision 24077)
> +++ subversion/libsvn_ra_svn/client.c (working copy)
> @@ -1079,8 +1079,14 @@
> {
> svn_ra_svn__session_baton_t *sess_baton = session->priv;
> svn_ra_svn_conn_t *conn = sess_baton->conn;
> - svn_boolean_t recurse = SVN_DEPTH_TO_RECURSE(depth);
> + svn_boolean_t recurse;
>
> + /* Old servers know "recursive" but not "depth"; help them DTRT. */
> + if (depth == svn_depth_empty || depth == svn_depth_files)
> + recurse = FALSE;
> + else
> + recurse = TRUE;
> +
> /* Tell the server we want to start an update. */
> SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "update", "(?r)cbw", rev, target,
> recurse, svn_depth_to_word(depth)));
This raises an interesting question: should SVN_DEPTH_TO_RECURSE()
itself change to the behavior you have above? I surveyed its callers:
it looks like approximately half could use (or could easily be changed
to use) the behavior you wrote above, and about half need the current
behavior of SVN_DEPTH_TO_RECURSE().
I'm not sure what to recommend here, I just wanted to raise this
issue. If you have time to take a look at that as a separate change,
even better.
> Index: subversion/libsvn_wc/update_editor.c
> ===================================================================
> --- subversion/libsvn_wc/update_editor.c (revision 24077)
> +++ subversion/libsvn_wc/update_editor.c (working copy)
> @@ -1135,6 +1157,26 @@
> || ((! copyfrom_path) && (SVN_IS_VALID_REVNUM(copyfrom_revision))))
> abort();
>
> + *child_baton = db;
> +
> + /* If we have a non-empty target and the parent of the newly added
> + directory is the root of the edit, then the new directory *is* the
> + target and we should set its depth accordingly. */
> + if (*eb->target && ! pb->parent_baton)
> + db->depth = eb->depth;
> + else if (pb->depth == svn_depth_empty
> + || pb->depth == svn_depth_exclude
> + || pb->depth == svn_depth_files)
> + {
> + db->depth = svn_depth_exclude;
> + db->bump_info->skipped = TRUE;
> + return SVN_NO_ERROR;
> + }
> + else if (pb->depth == svn_depth_immediates)
> + db->depth = svn_depth_empty;
> + else
> + db->depth = svn_depth_infinity;
> +
> SVN_ERR(svn_io_check_path(db->path, &kind, db->pool));
I feel silly saying this, but if one clause has braces, we usually
make sure they all do. I personally don't mind either way, but it
helps make the code more readable for a lot of people.
The logic here looks good to me: it's exactly the depth-shortening
effect I'd expected to see as we walk down the directory hierarchy.
> @@ -1350,6 +1379,26 @@
>
> /* Skip this directory if it has property conflicts. */
> SVN_ERR(svn_wc_entry(&entry, db->path, eb->adm_access, FALSE, pool));
> +
> + /* If we have a non-empty target and the parent of the newly added
> + directory is the root of the edit, then the new directory *is* the
> + target and we should set its depth accordingly. */
> + if (*eb->target && ! pb->parent_baton)
> + db->depth = eb->depth;
> + else if (pb->depth == svn_depth_empty
> + || pb->depth == svn_depth_files
> + || pb->depth == svn_depth_exclude)
> + {
> + db->depth = svn_depth_exclude;
> + db->bump_info->skipped = TRUE;
> + return SVN_NO_ERROR;
> + }
> + else if (pb->depth == svn_depth_immediates)
> + db->depth = svn_depth_empty;
> + else if (entry)
> + /* Allow the entry's actual depth to override svn_depth_infinity. */
> + db->depth = entry->depth;
> +
Ah, okay, I think I understand: if the operation's requested depth is
not svn_depth_infinity, then an entry->depth can't override it --
after all, if this were a new server, it wouldn't give us the
information we'd need to descend anyway. It's only because of the
old-server case that we have to ignore content coming down the pipe.
But if svn_depth_infinity, then we're getting exactly what we want
from the pipe, and the question is: what depth is the wc using?
Is that a fair summary of the reasoning here?
(Just trying to make sure I understand the code.)
> @@ -1451,6 +1500,12 @@
> apr_array_header_t *entry_props, *wc_props, *regular_props;
> svn_wc_adm_access_t *adm_access;
>
> + if (db->depth == svn_depth_exclude)
> + {
> + SVN_ERR(maybe_bump_dir_info(db->edit_baton, db->bump_info, db->pool));
> + return SVN_NO_ERROR;
> + }
> +
> SVN_ERR(svn_categorize_props(db->propchanges, &entry_props, &wc_props,
> ®ular_props, pool));
>
Hmmm. This is in close_directory(). Are you sure you didn't mean
svn_depth_empty intead of svn_depth_exclude?
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org