firemeteor_at_tigris.org writes:
> Log:
> In issue-2843-dev branch
>
> --- branches/issue-2843-dev/subversion/include/svn_wc.h (r31552)
> +++ branches/issue-2843-dev/subversion/include/svn_wc.h (r31553)
> @@ -4857,6 +4857,45 @@ svn_wc_set_changelist(const char *path,
> void *notify_baton,
> apr_pool_t *pool);
>
> +/** Crop the @a target according to @a depth. Any item that exceed the
> + * boundary of @a depth (relative to @a target) will be removed from revision
> + * control. Modified items will be left unversioned. And the clean ones will
> + * be removed from the disk.
> + *
> + * If the @a target has a shallower depth at the beginning, it will not be
> + * upgraded to the requested one, since that will not be a cropping at all.
> + * However, the children will be checked an cropped properly according to the
> + * requested @a depth.
> + *
> + * The function returns immediately with no error if @a target is not a
> + * directory, or if the @a depth is not restrictive at all (e.g.
> + * svn_depth_infinity).
> + *
> + * @a anchor is an access baton, with a tree lock, for the local path to the
> + * working copy which will be used as the root of this operation. If @a
> + * target is not empty, it represents an entry in the @a anchor path
> + * (otherwise, the @a anchor is the subject).
> + *
> + * If @a cancel_func is not @c NULL, call it with @a cancel_baton to determine
> + * if the client has cancelled the operation.
> + *
> + * If @a notify_func is not @c NULL, call it with @a notify_baton to report
> + * the change.
> + *
> + * @note: svn_depth_exclude is currently not supported.
> + *
> + * @since New in 1.6
> + */
> +svn_error_t *
> +svn_wc_crop_tree(svn_wc_adm_access_t *anchor,
> + const char *target,
> + svn_depth_t depth,
> + svn_wc_notify_func2_t notify_func,
> + void *notify_baton,
> + svn_cancel_func_t cancel_func,
> + void *cancel_baton,
> + apr_pool_t *pool);
> +
> /** @} */
Interface looks good. I've tweaked the doc string a little bit (r31591).
Because you've made a few changes to crop.c since this commit, I'm going
to review the whole file as it stands today (r31590). So, that's what's
quoted below.
> /*
> * crop.c: Cropping the WC
> *
> * ====================================================================
> * Copyright (c) 2000-2007 CollabNet. All rights reserved.
> *
> * This software is licensed as described in the file COPYING, which
> * you should have received as part of this distribution. The terms
> * are also available at http://subversion.tigris.org/license-1.html.
> * If newer versions of this license are posted there, you may use a
> * newer version instead, at your option.
> *
> * This software consists of voluntary contributions made by many
> * individuals. For exact contribution history, see the revision
> * history and logs, available at http://subversion.tigris.org/.
> * ====================================================================
> */
>
> /* ==================================================================== */
>
> #include "svn_wc.h"
> #include "svn_pools.h"
> #include "svn_error.h"
> #include "svn_client.h"
> #include "svn_error_codes.h"
> #include "svn_path.h"
> #include "entries.h"
>
> #define SVN_ERR_IGNORE_LOCAL_MOD(expr) \
> do { \
> svn_error_t *__temp = (expr); \
> if (__temp) \
> { \
> if (__temp->apr_err == SVN_ERR_WC_LEFT_LOCAL_MOD) \
> svn_error_clear(__temp); \
> else \
> return __temp; \
> } \
> } while (0)
This doesn't want need the "SVN_ERR_" prefix, for two reasons.
One, it's local to this file, so it doesn't need any prefix at all. You
can just call it IGNORE_LOCAL_MOD or something.
Two, even if it did need a prefix, the "SVN_ERR_" prefix probably
wouldn't be a good choice, because it's already used for an existing
public namespace :-).
> /* Helper function that crops the children of the DIR_PATH, under the constraint
> * of DEPTH. The DIR_PATH itself will never be cropped. The ADM_ACCESS is the
> * access baton that contains DIR_PATH. And the whole subtree should have been
> * locked.
> *
> * If NOTIFY_FUNC is not null, each file and ROOT of subtree will be reported
> * upon remove.
> */
> svn_error_t *
> crop_children(svn_wc_adm_access_t *adm_access,
> const char *dir_path,
> svn_depth_t depth,
> svn_wc_notify_func2_t notify_func,
> void *notify_baton,
> svn_cancel_func_t cancel_func,
> void *cancel_baton,
> apr_pool_t *pool)
Try not to get so close to 80 columns in your doc string. I recommend
using 72 columns as a boundary. Remember, code gets quoted in emails,
diffs, and other contexts where it will be somewhat indented.
You want 'static svn_error_t *', by the way, because this function is
local to this file.
When you say it's a "helper" function, it's good to say helper to what
-- in this case, svn_wc_crop_tree(). In fact, they're so similar that
it might make sense to simply document this as being just like
svn_wc_crop_tree() except for X, Y, and Z (where X, Y, and Z are
whatever ways it differs).
> {
> apr_hash_t *entries;
> apr_hash_index_t *hi;
> svn_wc_adm_access_t *dir_access;
> svn_wc_entry_t *dot_entry;
> apr_pool_t *subpool = svn_pool_create(pool), *iterpool;
>
> SVN_ERR(svn_wc_adm_retrieve(&dir_access, adm_access, dir_path, subpool));
> SVN_ERR(svn_wc_entries_read(&entries, dir_access, TRUE, subpool));
> dot_entry = apr_hash_get(entries, SVN_WC_ENTRY_THIS_DIR,
> APR_HASH_KEY_STRING);
>
> /* Update the depth of target first, if needed*/
> if (dot_entry->depth > depth)
> {
> /* XXX: Do we need to restore the modified depth if the user cancel this
> operation?*/
> dot_entry->depth = depth;
> SVN_ERR(svn_wc__entries_write(entries, dir_access, subpool));
> }
As long as the state of the working copy is *consistent* (useable) at
every possible cancellation point, I do not think we need to roll back
to the old depth.
I fixed the formatting of that comment, by the way, and of various other
comments. See r31591.
> /* Looping over current directory's SVN entries: */
> iterpool = svn_pool_create(subpool);
>
> for (hi = apr_hash_first(subpool, entries); hi; hi = apr_hash_next(hi))
> {
> const void *key;
> const char *this_path;
> void *val;
> apr_ssize_t klen;
> svn_wc_entry_t *current_entry;
> svn_pool_clear(iterpool);
>
> /* Get the next entry */
> apr_hash_this(hi, &key, &klen, &val);
> if (! strcmp(key, SVN_WC_ENTRY_THIS_DIR))
> continue;
Our usual style is:
if (strcmp(key, SVN_WC_ENTRY_THIS_DIR) == 0)
It's not a big deal, of course.
> 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)
> {
> 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 (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)
> {
> svn_wc_adm_access_t *child_access;
> SVN_ERR(svn_wc_adm_retrieve(&child_access, dir_access,
> this_path, iterpool));
>
> SVN_ERR_IGNORE_LOCAL_MOD
> (svn_wc_remove_from_revision_control(child_access,
> SVN_WC_ENTRY_THIS_DIR,
> TRUE, /* destroy */
> FALSE, /* instant error */
> 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
> SVN_ERR(crop_children(dir_access,
> this_path,
> svn_depth_empty,
> notify_func,
> notify_baton,
> cancel_func,
> cancel_baton,
> iterpool));
> }
I put braces around the 'else' case too. If any clause has braces, we
usually put braces for all clauses, so the logical symmetry is reflected
in the formatting. Again, not a big deal.
So, looking at the logic, we have:
if (current_entry->kind == svn_node_file && depth == svn_depth_empty)
### remove it from revision control ###
else if (current_entry->kind == svn_node_dir)
{
if (depth < svn_depth_immediates)
### remove it from revision control ###
else
crop_children(dir_access, this_path, svn_depth_empty, ...);
}
I'm trying to understand why we immediately jump to svn_depth_empty in
the call to crop_children(). If the original depth were
svn_depth_files, wouldn't we want to decrement to svn_depth_immediates
first?
> /* XXX: What about svn_node_none & svn_node_unkown? Currently assume
> svn_node_dir*/
> }
I'm not sure I understand this comment. We don't assume dir; we handle
the dir case explicitly, but we fall through and do nothing if
current_entry is neither file nor dir.
> svn_error_t *
> svn_wc_crop_tree(svn_wc_adm_access_t *anchor,
> const char *target,
> svn_depth_t depth,
> svn_wc_notify_func2_t notify_func,
> void *notify_baton,
> svn_cancel_func_t cancel_func,
> void *cancel_baton,
> apr_pool_t *pool)
> {
> const svn_wc_entry_t *entry;
> 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))
> return SVN_NO_ERROR;
The doc string of crop_children() doesn't say anything about restricting
what values of DEPTH it can take, but I think in fact it depends on its
caller (here) filtering out certain depths. If so, crop_children()'s
doc string should mention its limitations.
> /* Only makes sense to crop a dir target. */
> full_path = svn_path_join(svn_wc_adm_access_path(anchor), target, pool);
> SVN_ERR(svn_wc_entry(&entry, full_path, anchor, FALSE, pool));
> 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)
> {
> 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)
> {
> /* 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));
>
> if (notify_func)
> {
> svn_wc_notify_t *notify;
> notify = svn_wc_create_notify(full_path,
> svn_wc_notify_delete,
> pool);
> (*notify_func)(notify_baton, notify, pool);
> }
> return SVN_NO_ERROR;
> }
> }
*nod* Okay, I see this is the behavior we're discussing on dev@ right now.
Guo Rui, I'm very impressed by how many of the Subversion conventions
you have learned in a short time (clearing iterpool at the start of the
loop, writing wrapper macros in do-while style, etc). Nice job!
-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-04 20:36:06 CEST