On Sat, Jun 14, 2008 at 08:57:47PM -0400, Karl Fogel wrote:
> 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.
Well, I just followed the hacking guide.
>
> > --- 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...?
What if we are asked to pull in the not-existing-yet target?
> > @@ -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?
Yep! Done, waiting for commit.
>
> > --- 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.)
I currently choose not to crop the tree on a file basis. But even if I do crop
a single file, it will not happen here. The svn_wc_crop_tree() should handle
that situation. (I may also need to skip the notification in that situation.
I'll document this.)
In fact, I can also simply call svn_wc_remove_from_revision_control() for
excluded subdirectory (even though my code may be a bit faster?) -- the document
of svn_wc_remove_from_revision_control() seems a bit misleading. If I
understand correctly, it can handle any target rather than just 'file and
this_dir'.
> > @@ -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?
Yep!
>
> > }
> > - /* 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 :-).
Shy. It dates back to the time that I created the file. I just have no idea if
I need to shrow an exception here.
> > --- 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?
Nope. Just to make my comment more specific. :-)
Rui
---------------------------------------------------------------------
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 08:11:17 CEST