Vlad Georgescu <vgeorgescu@gmail.com> writes:
> I finished implementing depth upgrades (with 1.5 servers) in the
> attached patch. I can get the depth upgrade tests to work without
> breaking anything else (tested over ra_svn and ra_local).
>
> I'm posting it here for review.
Once again, you rock. Review below.
> [[[
> Make depth upgrades work.
>
> Patch by: me
> kfogel
>
> * subversion/libsvn_client/update.c
> (svn_client__update_internal): Don't read the on-disk entry to
> determine the depth for the update. Handle externals if depth is
> svn_depth_unknown.
>
> * subversion/libsvn_client/checkout.c
> (svn_client__checkout_internal): Pass the requested depth to
> svn_client__update_internal.
>
> * subversion/libsvn_wc/adm_crawler.c
> (svn_wc_crawl_revisions3): If depth is svn_depth_unknown, treat it
> like svn_depth_infinity. Use the entry depth instead of the
> requested depth for the top-level set-path. Don't call
> report_revisions_and_depths if the requested depth is empty.
> (report_revisions_and_depths): Report files when they are present in a
> depth-empty wc, and directories when they are present in a
> depth-empty or depth-files wc. Report subdirectories in a
> depth-immediates dir.
>
> * subversion/libsvn_wc/adm_ops.c
> (tweak_entries): Convert svn_depth_unknown to svn_depth_infinity, and
> handle depth correctly.
>
> * subversion/libsvn_repos/reporter.c
> (is_depth_upgrade): New.
"New function." :-)
> (update_entry): Take both a wc_depth and a requested_depth instead of
> a single depth, use them when calling delta_dirs().
> (delta_dirs): Take both a wc_depth and a requested_depth instead of a
> single depth, use them when iterating over the directory entries to
> determine whether to send the whole entry or just deltas.
> (drive): Update calls to update_entry and delta_dirs.
Ah, this may be the important thing I missed before: just give
delta_dirs() and update_entry() two depth args instead of trying to
make it work with one. Good idea!
> --- ../svn-trunk/subversion/libsvn_repos/reporter.c (revision 25262)
> +++ ../svn-trunk/subversion/libsvn_repos/reporter.c (working copy)
> @@ -580,6 +582,31 @@
Hmm, funny patch path ("../svn-trunk/...") made it a little hard to
apply the patch, but no big deal.
> }
>
>
> +/* Given REQUESTED_DEPTH, WC_DEPTH and the current entry's KIND,
> + determine whether we need to send the whole entry, not just deltas. */
> +static svn_boolean_t
> +is_depth_upgrade(svn_depth_t wc_depth,
> + svn_depth_t requested_depth,
> + svn_node_kind_t kind)
> +{
> + if (requested_depth == svn_depth_unknown
> + || requested_depth <= wc_depth
> + || wc_depth == svn_depth_immediates)
> + return FALSE;
> +
> + if (kind == svn_node_file
> + && wc_depth == svn_depth_files)
> + return FALSE;
> +
> + if (kind == svn_node_dir
> + && wc_depth == svn_depth_empty
> + && requested_depth == svn_depth_files)
> + return FALSE;
> +
> + return TRUE;
> +}
I had to stare at that for a long time :-). Then later I saw the doc
string for delta_dirs() and was much better able to reason about this.
Maybe it would be good to reference the new delta_dirs() doc string
from here?
> @@ -750,20 +754,65 @@
> }
> }
>
> +/* A helper macro for when we have to recurse into subdirectories. */
> +#define DEPTH_BELOW_HERE(depth) ((depth) == svn_depth_immediates) ? \
> + svn_depth_empty : (depth)
>
> /* Emit edits within directory DIR_BATON (with corresponding path
> E_PATH) with the changes from the directory S_REV/S_PATH to the
> directory B->t_rev/T_PATH. S_PATH may be NULL if the entry does
> not exist in the source.
>
> - DEPTH is the depth from the point in the descent represented by
> - this call, active until shadowed by some depth specified lower down
> - (e.g., by a (path_info_t *)->depth).
> + WC_DEPTH is this path's depth as reported by set_path/link_path.
> + REQUESTED_DEPTH is derived from the depth set by
> + svn_repos_begin_report().
> +
> + When iterating over this direcory's entries, the following tables
> + describe what happens for all possible combinations
> + of WC_DEPTH/REQUESTED_DEPTH (rows represent WC_DEPTH, columns
> + represent REQUESTED_DEPTH):
> +
> + Legend:
> + X: ignore this entry
> + o: update this entry
> + U: client is upgarding to a deeper wc, send the whole entry
Typo: "upgrading"
Also, the distinction between "update this entry" and "send the whole
entry" probably won't be quite clear to most readers (it's not even
quite clear to me, although I understand the desired end result). I
think it would be okay to give more verbose explanations here, since
this is the core of the logic.
> + For files:
> + ______________________________________________________________
> + | req. depth| unknown | empty | files | immediates | infinity |
> + |wc. depth | | | | | |
> + |___________|_________|_______|_______|____________|__________|
> + |empty | X | X | U | U | U |
> + |___________|_________|_______|_______|____________|__________|
> + |files | o | X | o | o | o |
> + |___________|_________|_______|_______|____________|__________|
> + |immediates | o | X | o | o | o |
> + |___________|_________|_______|_______|____________|__________|
> + |infinity | o | X | o | o | o |
> + |___________|_________|_______|_______|____________|__________|
> +
> + For directories:
> + ______________________________________________________________
> + | req. depth| unknown | empty | files | immediates | infinity |
> + |wc. depth | | | | | |
> + |___________|_________|_______|_______|____________|__________|
> + |empty | X | X | X | U | U |
> + |___________|_________|_______|_______|____________|__________|
> + |files | X | X | X | U | U |
> + |___________|_________|_______|_______|____________|__________|
> + |immediates | o | X | X | o | o |
> + |___________|_________|_______|_______|____________|__________|
> + |infinity | o | X | X | o | o |
> + |___________|_________|_______|_______|____________|__________|
> +
> + These rules are enforced by the is_depth_upgrade() function and by
> + various other checks below.
> */
That's a terrific doc string -- those tables are exactly what was
needed, thank you!
> static svn_error_t *
> delta_dirs(report_baton_t *b, svn_revnum_t s_rev, const char *s_path,
> const char *t_path, void *dir_baton, const char *e_path,
> - svn_boolean_t start_empty, svn_depth_t depth, apr_pool_t *pool)
> + svn_boolean_t start_empty, svn_depth_t wc_depth,
> + svn_depth_t requested_depth, apr_pool_t *pool)
> {
> svn_fs_root_t *s_root;
> apr_hash_t *s_entries = NULL, *t_entries;
> @@ -781,149 +830,162 @@
> SVN_ERR(delta_proplists(b, s_rev, start_empty ? NULL : s_path, t_path,
> NULL, change_dir_prop, dir_baton, pool));
>
> - /* Get the list of entries in each of source and target. */
> - if (s_path && !start_empty)
> + if (requested_depth > svn_depth_empty
> + || requested_depth == svn_depth_unknown)
> {
> - SVN_ERR(get_source_root(b, &s_root, s_rev));
> - SVN_ERR(svn_fs_dir_entries(&s_entries, s_root, s_path, pool));
> - }
> - SVN_ERR(svn_fs_dir_entries(&t_entries, b->t_root, t_path, pool));
> + /* Get the list of entries in each of source and target. */
> + if (s_path && !start_empty)
> + {
> + SVN_ERR(get_source_root(b, &s_root, s_rev));
> + SVN_ERR(svn_fs_dir_entries(&s_entries, s_root, s_path, pool));
> + }
> + SVN_ERR(svn_fs_dir_entries(&t_entries, b->t_root, t_path, pool));
>
> - /* Iterate over the report information for this directory. */
> - subpool = svn_pool_create(pool);
> + /* Iterate over the report information for this directory. */
> + subpool = svn_pool_create(pool);
>
> - while (1)
> - {
> - svn_pool_clear(subpool);
> - SVN_ERR(fetch_path_info(b, &name, &info, e_path, subpool));
> - if (!name)
> - break;
> + while (1)
> + {
> + svn_pool_clear(subpool);
> + SVN_ERR(fetch_path_info(b, &name, &info, e_path, subpool));
> + if (!name)
> + break;
>
> - if (info && !SVN_IS_VALID_REVNUM(info->rev))
> - {
> - /* We want to perform deletes before non-replacement adds,
> - for graceful handling of case-only renames on
> - case-insensitive client filesystems. So, if the report
> - item is a delete, remove the entry from the source hash,
> - but don't update the entry yet. */
> + if (info && !SVN_IS_VALID_REVNUM(info->rev))
> + {
> + /* We want to perform deletes before non-replacement adds,
> + for graceful handling of case-only renames on
> + case-insensitive client filesystems. So, if the report
> + item is a delete, remove the entry from the source hash,
> + but don't update the entry yet. */
> + if (s_entries)
> + apr_hash_set(s_entries, name, APR_HASH_KEY_STRING, NULL);
> + continue;
> + }
> +
> + e_fullpath = svn_path_join(e_path, name, subpool);
> + t_fullpath = svn_path_join(t_path, name, subpool);
> + t_entry = apr_hash_get(t_entries, name, APR_HASH_KEY_STRING);
> + s_fullpath = s_path ? svn_path_join(s_path, name, subpool) : NULL;
> + s_entry = s_entries ?
> + apr_hash_get(s_entries, name, APR_HASH_KEY_STRING) : NULL;
> +
> + /* The only special case here is when requested_depth is files
> + but the reported path is a directory. This is technically
> + a client error, but we handle it anyway, by skipping the
> + entry. */
Nice catch!
> + if (requested_depth != svn_depth_files
> + || ((! t_entry || t_entry->kind != svn_node_dir)
> + && (! s_entry || s_entry->kind != svn_node_dir)))
> + SVN_ERR(update_entry(b, s_rev, s_fullpath, s_entry, t_fullpath,
> + t_entry, dir_baton, e_fullpath, info,
> + info ? info->depth
> + : DEPTH_BELOW_HERE(wc_depth),
> + DEPTH_BELOW_HERE(requested_depth), subpool));
I expected that parameter to be...
info ? DEPTH_BELOW_HERE(info->depth)
: DEPTH_BELOW_HERE(wc_depth),
...but maybe I'm missing something?
> --- ../svn-trunk/subversion/libsvn_wc/adm_crawler.c (revision 25262)
> +++ ../svn-trunk/subversion/libsvn_wc/adm_crawler.c (working copy)
> @@ -466,6 +479,11 @@
> compared to. */
> SVN_ERR(svn_wc_entry(&entry, path, adm_access, FALSE, pool));
>
> + /* svn_depth_unknown is just like svn_depth_infinity, as far as
> + reporting is concerned. */
> + if (depth == svn_depth_unknown)
> + depth = svn_depth_infinity;
> +
> if ((! entry) || ((entry->schedule == svn_wc_schedule_add)
> && (entry->kind == svn_node_dir)))
> {
Okay, this is the one thing that still bothers me.
The depth passed in to svn_wc_crawl_revisions3() is the *requested*
depth, right? (We haven't yet documented that function's 'depth'
parameter yet, unfortunately, but I'm pretty sure it's requested
depth.) About thirteen lines after the above, we pass that depth to
set_path() as what seems to be a wc depth:
SVN_ERR(reporter->set_path(report_baton, "", base_rev, depth,
entry ? entry->incomplete : TRUE,
NULL, pool));
Now in this particular call to set_path(), it's not a problem, because
there's no versioned path to crawl anyway. Here's the full context:
if ((! entry) || ((entry->schedule == svn_wc_schedule_add)
&& (entry->kind == svn_node_dir)))
{
/* There aren't any versioned paths to crawl which are known to
the repository. */
SVN_ERR(svn_wc__entry_versioned(&parent_entry,
svn_path_dirname(path, pool),
adm_access, FALSE, pool));
base_rev = parent_entry->revision;
SVN_ERR(reporter->set_path(report_baton, "", base_rev, depth,
entry ? entry->incomplete : TRUE,
NULL, pool));
SVN_ERR(reporter->delete_path(report_baton, "", pool));
So even though it's a little confusing to someone reading the code,
maybe it's okay that we passed a requested depth (instead of a wc
depth) to set_path(), since there's no wc depth available. But let's
look at the other place 'depth' is used in svn_wc_crawl_revisions3():
if (entry->kind == svn_node_dir)
{
if (missing)
{
/* Always report directories as missing; we can't recreate
them locally. */
err = reporter->delete_path(report_baton, "", pool);
if (err)
goto abort_report;
}
else if (depth != svn_depth_empty)
{
/* Recursively crawl ROOT_DIRECTORY and report differing
revisions. */
err = report_revisions_and_depths(adm_access,
"",
base_rev,
reporter, report_baton,
notify_func, notify_baton,
restore_files, depth,
entry->incomplete,
use_commit_times,
traversal_info,
pool);
if (err)
goto abort_report;
}
}
That seems wrong. It's used for the conditional and as a parameter to
report_revisions_and_depths(). But if 'svn_depth_unknown' was passed
in as the requested depth parameter to svn_wc_crawl_revisions3(),
shouldn't it just be passed along to report_revisions_and_depths()?
Everything else about this patch seems exactly right to me, except
this one thing, but it's an important thing :-). If svn_depth_unknown
was requested, then that's what we should report to the repository.
Maybe that means we'll have to change some of the crawler code to
*treat* svn_depth_unknown like svn_depth_infinity, when recursing into
subdirs, but that's okay. The important thing is not to lie to the
server about what was requested.
(By the way, report_revisions_and_depths()'s doc string says that
'depth' is the *requested* depth, but doesn't mention how to handle
svn_depth_unknown. It seems to me it should talk about that.)
> @@ -476,8 +494,7 @@
> adm_access, FALSE, pool));
>
> base_rev = parent_entry->revision;
> - if (depth == svn_depth_unknown)
> - depth = parent_entry->depth;
> +
> SVN_ERR(reporter->set_path(report_baton, "", base_rev, depth,
> entry ? entry->incomplete : TRUE,
> NULL, pool));
Okay, this is that first, special-case set_path() call, where we have
no wc depth to report, so we fall back on the requested depth.
> @@ -499,13 +516,10 @@
> base_rev = parent_entry->revision;
> }
>
> - if (depth == svn_depth_unknown)
> - depth = entry->depth;
> -
> /* The first call to the reporter merely informs it that the
> top-level directory being updated is at BASE_REV. Its PATH
> argument is ignored. */
> - SVN_ERR(reporter->set_path(report_baton, "", base_rev, depth,
> + SVN_ERR(reporter->set_path(report_baton, "", base_rev, entry->depth,
> entry->incomplete , /* start_empty ? */
> NULL, pool));
Yep, entry->depth seems right.
> @@ -532,16 +546,8 @@
> if (err)
> goto abort_report;
> }
> - else
> + else if (depth != svn_depth_empty)
> {
> - /* ### TODO(sd): Just passing depth here is not enough. There
> - ### can be circumstances where the root is depth 0 or 1,
> - ### but some child directories are present at depth
> - ### infinity. We need to detect them and recurse into
> - ### them *unless* there is a passed-in depth that is not
> - ### infinity. I think.
> - */
> -
And this is the conditional that leads into the problems I discussed
above.
> --- ../svn-trunk/subversion/libsvn_wc/adm_ops.c (revision 25262)
> +++ ../svn-trunk/subversion/libsvn_wc/adm_ops.c (working copy)
> @@ -89,9 +89,10 @@
> &write_required,
> svn_wc_adm_access_pool(dirpath)));
>
> - if (depth == svn_depth_files
> - || depth == svn_depth_immediates
> - || depth == svn_depth_infinity)
> + if (depth == svn_depth_unknown)
> + depth = svn_depth_infinity;
> +
> + if (depth > svn_depth_empty)
> {
> for (hi = apr_hash_first(pool, entries); hi; hi = apr_hash_next(hi))
> {
Here the conversion is fine, because we're not going to pass it along
to the server.
> @@ -135,9 +136,15 @@
> }
>
> /* If a directory and recursive... */
> - else if ((depth == svn_depth_infinity)
> + else if ((depth == svn_depth_infinity
> + || depth == svn_depth_immediates)
> && (current_entry->kind == svn_node_dir))
> {
> + svn_depth_t depth_below_here = depth;
> +
> + if (depth == svn_depth_immediates)
> + depth_below_here = svn_depth_empty;
> +
> /* If the directory is 'missing', remove it. This is safe as
> long as this function is only called as a helper to
> svn_wc__do_update_cleanup, since the update will already have
Yeah, I guess it's not worth importing that macro to use here :-).
Vlad, bravo for making depth-upgrades work! The only thing I'm
worried about is that we're not accurately reporting wc depth to the
server in all cases (unless I'm misunderstanding something?). Is
there a way to fix the logic so that we're always reporting requested
depth accurately, but we still get the right upgrade results?
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Jun 4 08:44:03 2007