firemeteor_at_tigris.org writes:
> Log:
> On issue-2843-dev branch
>
> According to Karl: 1) report 'D' on those locally modified items is Okay. And
> 2) crop a subtree to svn_depth_empty should not take the root away. This
> purpose should be covered by a dedicated enum, such as svn_depth_excluded.
>
> * subversion/tests/cmdline/depth_tests.py
> (fold_tree_with_unversioned_modified_items): Tune comments.
> (test_list): The above test is no longer XFAIL.
>
> * subversion/libsvn_wc/crop.c
> (svn_wc_crop_tree): don't crop the root on svn_depth_empty.
The log message is accurate, but perhaps a bit incomplete? In
svn_wc_crop_tree(), you've done more than just make it not crop root in
the svn_depth_empty case: you've also added preliminary code to handle
svn_depth_exclude.
> --- branches/issue-2843-dev/subversion/libsvn_wc/crop.c (r31654)
> +++ branches/issue-2843-dev/subversion/libsvn_wc/crop.c (r31655)
> @@ -187,38 +187,42 @@ svn_wc_crop_tree(svn_wc_adm_access_t *an
> if (!entry || entry->kind != svn_node_dir)
> return SVN_NO_ERROR;
>
> - /* Check to see if the target itself should be cropped. */
> - if (depth == svn_depth_empty)
> + /* Crop the target itself if we are requested to. */
> +
> + /* XXX: As you can see, this is currently a dead branch, since we are not
> + yet ready for svn_depth_exclude as other parts of the code in libsv_wc.
> + */
> + if (depth == svn_depth_exclude)
> {
> const svn_wc_entry_t *parent_entry;
> SVN_ERR(svn_wc_entry(&parent_entry,
> svn_path_dirname(full_path, pool),
> anchor, FALSE, pool));
>
> - if (parent_entry && parent_entry->depth <= svn_depth_files)
> + if (parent_entry && parent_entry->depth > svn_depth_files)
> {
> - /* Crop the target with the subtree altogether if the parent
> - does not want sub-directories. */
> - SVN_ERR(svn_wc_adm_retrieve(&dir_access, anchor, full_path, pool));
> - SVN_ERR_IGNORE_LOCAL_MOD
> - (svn_wc_remove_from_revision_control(dir_access,
> - SVN_WC_ENTRY_THIS_DIR,
> - TRUE, /* destroy */
> - FALSE, /* instant error */
> - cancel_func,
> - cancel_baton,
> - pool));
> + /* TODO: mark the target as excluded, since the parent require it by
> + default. */
> + }
So, the comment above means "mark the target as excluded in the parent's
entries list", right? And I wasn't sure what the "since the parent
requires it by default" part meant -- but I see you changed "since" to
"if" in a later commit, which clarifies things a bit: it means, if the
parent's depth is deep enough to still keep the empty subdir, right?
And that's what the conditional change
"if (parent_entry && parent_entry->depth > svn_depth_files)"
above was about too, I guess.
By the way, sometimes you say "XXX", and sometimes you say "TODO" :-).
May I suggest what we did on the sparse-dirs branch? Use a special TODO
form, like "/* TODO(#2843): ...", so that we can always find all of the
TODO spots, even after merging the branch.
Rest looks good. On to the next commit...
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-06-15 02:57:52 CEST