firemeteor_at_tigris.org writes:
> Log:
> On issue-2843-dev branch.
>
> Handle svn_depth_exclude in the cropping logic. However, other part of the
> libsvn_wc code still don't honor the svn_depth_exclude flag.
>
> [in subversion/libsvn_wc]
>
> * entries.c
> (write_entry): Store svn_depth_exclude for subdir entries.
>
> * README
> (depth): Document the change in entries.c.
>
> * lock.c
> (do_open): Skip entry with svn_depth_exclude depth.
>
> * adm_ops.c
> (svn_wc_remove_from_revision): Skip excluded subdir entries and preserve
> excluded entry in parent dir.
>
> * crop.c
> (svn_wc_crop_tree): Handle svn_depth_exclude properly, mark target as
> svn_depth_exclude if the parent expects it.
> (crop_children): Same. Also fold the notify branches.
>
> [in subversion/libsvn_client]
>
> * update.c
> (svn_client__update_internal): Skip update totally on svn_depth_exclude.
We usually give the full path (from source tree root) of each file.
It's not a big deal, just fix up the log message when you get a chance.
> --- branches/issue-2843-dev/subversion/libsvn_client/update.c (r31703)
> +++ branches/issue-2843-dev/subversion/libsvn_client/update.c (r31704)
> @@ -160,10 +160,18 @@ svn_client__update_internal(svn_revnum_t
>
> /* We may need to crop the tree if the depth is sticky */
> if (depth_is_sticky && depth < svn_depth_infinity)
> - SVN_ERR(svn_wc_crop_tree(adm_access, target, depth,
> - ctx->notify_func2, ctx->notify_baton2,
> - ctx->cancel_func, ctx->cancel_baton,
> - pool));
> + {
> + SVN_ERR(svn_wc_crop_tree(adm_access, target, depth,
> + ctx->notify_func2, ctx->notify_baton2,
> + ctx->cancel_func, ctx->cancel_baton,
> + pool));
> + /* If we are asked to exclude a target, we can just stop now. */
> + if (depth == svn_depth_exclude)
> + {
> + SVN_ERR(svn_wc_adm_close(adm_access));
> + return SVN_NO_ERROR;
> + }
> + }
What about in the svn_depth_empty case? I would think we'd also "just
stop now" in that case, but maybe I'm missing something...?
> --- branches/issue-2843-dev/subversion/libsvn_wc/README (r31703)
> +++ branches/issue-2843-dev/subversion/libsvn_wc/README (r31704)
> @@ -503,10 +503,18 @@ depth:
> entries and pulls new entries with depth infinity as well.
> Default is infinite (`').
>
> + Apart from above normal values, there is still a special `exclude',
> + which means the directory is excluded from wc even if the depth of
> + parent directory is infinity or immediates. The directory in
> + question will not physically exist on the disk. In other words, we
> + can only store the `exclude' value in the corresponding entry of
> + the entries file in parent directory.
> +
*nod* Thanks for remembering to update README :-).
> The only fields allowed in an entry for a directory (other than the
> this_dir entry) are name, absent, deleted, schedule, copied,
> -copyfrom-url, copyfrom-rev and kind. The other fields are only stored
> -in the this_dir entry for the directory in question.
> +copyfrom-url, copyfrom-rev, kind and depth (only when the value says
> +exclude). The other fields are only stored in the this_dir entry for
> +the directory in question.
We may need a few wording and formatting changes here, but this is good.
> --- branches/issue-2843-dev/subversion/libsvn_wc/adm_ops.c (r31703)
> +++ branches/issue-2843-dev/subversion/libsvn_wc/adm_ops.c (r31704)
> @@ -2500,11 +2500,12 @@ svn_wc_remove_from_revision_control(svn_
> current_entry_name,
> subpool);
>
> - if (svn_wc__adm_missing(adm_access, entrypath))
> + if (svn_wc__adm_missing(adm_access, entrypath)
> + || current_entry->depth == svn_depth_exclude)
> {
> - /* The directory is already missing, so don't try to
> - recurse, just delete the entry in the parent
> - directory. */
> + /* The directory is either missing or excluded,
> + so don't try to recurse, just delete the
> + entry in the parent directory. */
> svn_wc__entry_remove(entries, current_entry_name);
> }
> else
That all makes sense, yup. It might be even clearer to say
"subdirectory" instead of "directory", but it's comprehensible this way
too (I realize the original wording is not yours).
> @@ -2550,6 +2551,7 @@ svn_wc_remove_from_revision_control(svn_
> full_path. We need to remove that entry: */
> if (! is_root)
> {
> + svn_wc_entry_t *dir_entry;
> svn_wc_adm_access_t *parent_access;
>
> svn_path_split(full_path, &parent_dir, &base_name, pool);
> @@ -2558,8 +2560,19 @@ svn_wc_remove_from_revision_control(svn_
> parent_dir, pool));
> SVN_ERR(svn_wc_entries_read(&entries, parent_access, TRUE,
> pool));
> - svn_wc__entry_remove(entries, base_name);
> - SVN_ERR(svn_wc__entries_write(entries, parent_access, pool));
> +
> + /* An exception: When the path is at svn_depth_exclude,
> + the entry in the parent directory should be preserved
> + for bookkeeping purpose. This only happens when the
> + function is called by svn_wc_crop_tree(). */
> + dir_entry = apr_hash_get(entries, base_name,
> + APR_HASH_KEY_STRING);
> + if (dir_entry->depth != svn_depth_exclude)
> + {
> + svn_wc__entry_remove(entries, base_name);
> + SVN_ERR(svn_wc__entries_write(entries, parent_access, pool));
> +
> + }
> }
> }
I realize it's not your code, but I find the reuse of "entries"
confusing here, and wish there were a separate, block-local variable
named "parent_entries" or something. Do you agree?
> --- branches/issue-2843-dev/subversion/libsvn_wc/crop.c (r31703)
> +++ branches/issue-2843-dev/subversion/libsvn_wc/crop.c (r31704)
> @@ -96,29 +96,36 @@ crop_children(svn_wc_adm_access_t *adm_a
> current_entry = val;
> this_path = svn_path_join(dir_path, current_entry->name, iterpool);
>
> - if (current_entry->kind == svn_node_file && depth == svn_depth_empty)
> + if (current_entry->kind == svn_node_file)
> {
> - SVN_ERR_IGNORE_LOCAL_MOD
> - (svn_wc_remove_from_revision_control(dir_access,
> - current_entry->name,
> - TRUE, /* destroy */
> - FALSE, /* instant error */
> - cancel_func,
> - cancel_baton,
> - iterpool));
> + if (depth == svn_depth_empty)
> + SVN_ERR_IGNORE_LOCAL_MOD
> + (svn_wc_remove_from_revision_control(dir_access,
> + current_entry->name,
> + TRUE, /* destroy */
> + FALSE, /* instant error */
> + cancel_func,
> + cancel_baton,
> + iterpool));
> + else
> + continue;
Hmm. Okay, but could depth ever be svn_depth_exclude for a file? (If
so, we should handle it; if not, we should explain why not.)
> - if (notify_func)
> - {
> - svn_wc_notify_t *notify;
> - notify = svn_wc_create_notify(this_path,
> - svn_wc_notify_delete,
> - iterpool);
> - (*notify_func)(notify_baton, notify, iterpool);
> - }
> }
> else if (current_entry->kind == svn_node_dir)
> {
> - if (depth < svn_depth_immediates)
> + if (current_entry->depth == svn_depth_exclude)
> + {
> + /* Preserve the excluded entry if the parent need it.
> + Anyway, don't report on excluded subdir, since they are
> + logically not exist. */
> + if (depth < svn_depth_immediates)
> + {
> + svn_wc__entry_remove(entries, current_entry->name);
> + SVN_ERR(svn_wc__entries_write(entries, dir_access, iterpool));
> + }
> + continue;
> + }
> + else if (depth < svn_depth_immediates)
> {
> svn_wc_adm_access_t *child_access;
> SVN_ERR(svn_wc_adm_retrieve(&child_access, dir_access,
> @@ -132,14 +139,6 @@ crop_children(svn_wc_adm_access_t *adm_a
> cancel_func,
> cancel_baton,
> iterpool));
> - if (notify_func)
> - {
> - svn_wc_notify_t *notify;
> - notify = svn_wc_create_notify(this_path,
> - svn_wc_notify_delete,
> - iterpool);
> - (*notify_func)(notify_baton, notify, iterpool);
> - }
> }
> else
> {
> @@ -151,10 +150,23 @@ crop_children(svn_wc_adm_access_t *adm_a
> cancel_func,
> cancel_baton,
> iterpool));
> + continue;
> }
By 'continue'ing here, we skip the notification block down below.
That's okay because the recursive call will do notification for
this_path, right?
> }
> - /* XXX: What about svn_node_none & svn_node_unkown? Currently assume
> - svn_node_dir*/
> + else
> + {
> + /* XXX: What about svn_node_none & svn_node_unkown? Currently assume
> + svn_node_dir*/
> + }
As always, the second sentence of that comment seems inaccurate to me :-).
> @@ -176,9 +188,8 @@ svn_wc_crop_tree(svn_wc_adm_access_t *an
> const char *full_path;
> svn_wc_adm_access_t *dir_access;
>
> - /* Only makes sense when the depth is restrictive.
> - Currently does not support svn_depth_exclude. */
> - if (!(depth > svn_depth_exclude && depth < svn_depth_infinity))
> + /* Only makes sense when the depth is restrictive. */
> + if (!(depth >= svn_depth_exclude && depth < svn_depth_infinity))
> return SVN_NO_ERROR;
>
> /* Only makes sense to crop a dir target. */
> @@ -187,23 +198,40 @@ svn_wc_crop_tree(svn_wc_adm_access_t *an
> if (!entry || entry->kind != svn_node_dir)
> return SVN_NO_ERROR;
>
> - /* Crop the target itself if we are requested to. */
> + /* Defensive test against excluded target. We may need this until we filter
> + them out in svn_wc_entry(). */
> + if (entry->depth == svn_depth_exclude)
> + return SVN_NO_ERROR;
Are we planning to filter them out in svn_wc_entry()? (Might be clearer
to say "already-exluded" target, by the way.)
Rest of this file looks good.
> --- branches/issue-2843-dev/subversion/libsvn_wc/entries.c (r31703)
> +++ branches/issue-2843-dev/subversion/libsvn_wc/entries.c (r31704)
> @@ -1654,7 +1654,9 @@ write_entry(svn_stringbuf_t *buf,
> }
>
> /* Depth. */
> - if (is_subdir || entry->depth == svn_depth_infinity)
> + /* Accept `exclude' for subdir entry. */
> + if ((is_subdir && entry->depth != svn_depth_exclude)
> + || entry->depth == svn_depth_infinity)
> {
> write_val(buf, NULL, 0);
> }
I had to read that five times, for some reason, but now I see that it
does the right thing.
> --- branches/issue-2843-dev/subversion/libsvn_wc/lock.c (r31703)
> +++ branches/issue-2843-dev/subversion/libsvn_wc/lock.c (r31704)
> @@ -673,6 +673,11 @@ do_open(svn_wc_adm_access_t **adm_access
> if (entry->kind != svn_node_dir
> || ! strcmp(entry->name, SVN_WC_ENTRY_THIS_DIR))
> continue;
> +
> + /* Also skip the excluded subdir. */
> + if (entry->depth == svn_depth_exclude)
> + continue;
> +
> entry_path = svn_path_join(lock->path, entry->name, subpool);
>
> /* Don't use the subpool pool here, the lock needs to persist */
Looks good. Any particular reason you didn't group that condition with
the other "skip" conditions immediately above it?
---------------------------------------------------------------------
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:58:07 CEST