On 1/10/07, Malcolm Rowe <malcolm-svn-dev@farside.org.uk> wrote:
> On Tue, Jan 09, 2007 at 02:20:48AM -0800, zhakov@tigris.org wrote:
> > Add option "--keep-local" to svn rm command to keep deleted path in
> > current working copy.
> >
>
> Nice! I made a followup commit in r22963 to clarify some of the
> comments (and document the new attribute in the README file in
> libsvn_wc).
Thank you very much for fixing that stuff!
> I've got a few comments on the implementation, see below:
>
> > --- trunk/subversion/libsvn_wc/adm_ops.c (original)
> > +++ trunk/subversion/libsvn_wc/adm_ops.c Tue Jan 9 02:20:46 2007
> > @@ -1095,12 +1114,13 @@
> >
> >
> > svn_error_t *
> > -svn_wc_delete2(const char *path,
> > +svn_wc_delete3(const char *path,
> > svn_wc_adm_access_t *adm_access,
> > svn_cancel_func_t cancel_func,
> > void *cancel_baton,
> > svn_wc_notify_func2_t notify_func,
> > void *notify_baton,
> > + svn_boolean_t keep_local,
> > apr_pool_t *pool)
> > {
> > svn_wc_adm_access_t *dir_access;
> > @@ -1175,8 +1195,10 @@
> > if (dir_access != adm_access)
> > {
> > /* Recursively mark a whole tree for deletion. */
> > - SVN_ERR(mark_tree(dir_access, SVN_WC__ENTRY_MODIFY_SCHEDULE,
> > - svn_wc_schedule_delete, FALSE,
> > + SVN_ERR(mark_tree(dir_access,
> > + SVN_WC__ENTRY_MODIFY_SCHEDULE
> > + | SVN_WC__ENTRY_MODIFY_KEEP_LOCAL,
> > + svn_wc_schedule_delete, FALSE, keep_local,
> > cancel_func, cancel_baton,
> > notify_func, notify_baton,
> > pool));
>
> According to the comments, we're trying to delete a directory that's
> missing. Why might we want to set the keep-local attribute in this
> case?
Directory is missing if dir_access == adm_access. I.e. when
adm_probe_retrieve returned parent adm access. This code isn't clear
for me too.
>
> > --- trunk/subversion/libsvn_wc/entries.c (original)
> > +++ trunk/subversion/libsvn_wc/entries.c Tue Jan 9 02:20:46 2007
> > @@ -646,6 +651,10 @@
> > modify_flags, SVN_WC__ENTRY_MODIFY_INCOMPLETE,
> > atts, SVN_WC__ENTRY_ATTR_INCOMPLETE, name));
> >
> > + /* Is this item should be kept in working copy after deletion? */
> > + SVN_ERR(do_bool_attr(&entry->keep_local,
> > + modify_flags, SVN_WC__ENTRY_MODIFY_KEEP_LOCAL,
> > + atts, SVN_WC__ENTRY_ATTR_KEEP_LOCAL, name));
> >
>
> (I wondered whether we should only copy the attribute across when the
> entry was schedule-deleted, but it doesn't look like we do that kind of
> checking here).
We perform changes only in fold_entry(). I think we don't cleanup
fields in svn_wc__atts_to_entry() because not all fields can be set to
valid entry combination. I mean that this function used to generate
xml entries for wc-log commands. So it can contain just log for
modifying one fields, keep_local for example, but doesn't contain
schedule. So we cannot check fields for invalid combination.
Other reason that better to reduce number on situation when fields are
cleaning up. Now we cleanup fields only when changing entries, not
when copying.
>
> > @@ -2178,6 +2197,13 @@
> > cur_entry->copyfrom_url = NULL;
> > }
> >
> > + /* keep_local makes sense only when we are going to delete directory. */
> > + if (modify_flags & SVN_WC__ENTRY_MODIFY_SCHEDULE
> > + && entry->schedule != svn_wc_schedule_delete)
> > + {
> > + cur_entry->keep_local = FALSE;
> > + }
> > +
>
> This is where we check for valid combinations. Should we also check
> that the entry we're setting keep-local for is the this_dir entry?
Hm. I think it's should be not entries.c behavior.
For me we should use meaningless fields cleanup only if have strong
reason for that. We have to cleanup keep_local for any change of
schedule. But we set keep_local in only one place so for me cleaner
set it to only this_dir and don't pile fold_entry function.
At least all WC code now is written in this way.
>
> > --- trunk/subversion/svn/main.c (original)
> > +++ trunk/subversion/svn/main.c Tue Jan 9 02:20:46 2007
> > @@ -337,14 +339,16 @@
> > "\n"
> > " 1. Each item specified by a PATH is scheduled for deletion upon\n"
> > " the next commit. Files, and directories that have not been\n"
> > - " committed, are immediately removed from the working copy.\n"
> > + " committed, are immediately removed from the working copy\n"
> > + " unless --keep-local option is given.\n"
> > " PATHs that are, or contain, unversioned or modified items will\n"
> > " not be removed unless the --force option is given.\n"
> > "\n"
> > " 2. Each item specified by a URL is deleted from the repository\n"
> > " via an immediate commit.\n"),
>
> This is incorrect: it implies that --keep-local only works for uncommitted
> (schedule-add) directories.
>
Oops. I see your discussion with David Glasser about this help text.
For me this text is very piled with useless information. For me is
better to remove text with explaining when PATH will be removed: after
command or after commit. What do you think?
--
Ivan Zhakov
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Jan 12 09:12:19 2007