Julian Foad <julianfoad_at_btopenworld.com> writes:
> The attached patch simplifies code that walks a WC, by always using the
> "walk_entries" function, and no longer special-casing the single-file
> (or non-recursive directory) cases. The "walk_entries" function can
> handle those cases itself.
>
> This patch is NOT FINISHED: two merge tests fail, presumably due to some
> intentional difference between the single-item case and the recursive
> cases. I'll investigate and fix if there are no objections.
>
> Does anyone see a flaw in this approach?
Not only do I not see a flaw in this approach, I see a big improvement
to our code...
-K
> Simplify code that walks a WC by always using the "walk_entries" function,
> removing the special case processing of the single-file or non-recursive
> cases. The "walk_entries" function can handle those cases itself.
>
> There is no functional change.
>
> In one case, the behaviour was intentionally slightly different when
> processing a single target, and in this case the special behaviour is
> now implemented as an option in the generic code path.
>
> ### FAIL: merge_tests.py 3 20
>
> * subversion/libsvn_client/info.c
> (crawl_entries): Don't special-case a single file.
>
> * subversion/libsvn_client/merge.c
> (get_mergeinfo_paths): Don't special-case a single file.
>
> * subversion/libsvn_client/prop_commands.c
> (propset_walk_baton): Add an "ignore_illegal_targets" flag.
> (propset_walk_cb): Obey the "ignore_illegal_targets" flag.
> (svn_client_propset3): Don't special case a single file; instead, just
> set the "ignore_illegal_targets" flag appropriately.
> (svn_client__get_prop_from_wc): Don't special-case depth=empty.
> (svn_client_proplist3): Don't special-case depth=empty.
>
> * subversion/libsvn_wc/adm_ops.c
> (svn_wc_resolved_conflict3): Don't special-case depth=empty.
>
> Index: subversion/libsvn_client/info.c
> ===================================================================
> --- subversion/libsvn_client/info.c (revision 31702)
> +++ subversion/libsvn_client/info.c (working copy)
> @@ -257,35 +257,20 @@ crawl_entries(const char *wcpath,
> apr_pool_t *pool)
> {
> svn_wc_adm_access_t *adm_access;
> - const svn_wc_entry_t *entry;
> int adm_lock_level = SVN_WC__LEVELS_TO_LOCK_FROM_DEPTH(depth);
> + struct found_entry_baton fe_baton;
>
> SVN_ERR(svn_wc_adm_probe_open3(&adm_access, NULL, wcpath, FALSE,
> adm_lock_level, ctx->cancel_func,
> ctx->cancel_baton, pool));
> - SVN_ERR(svn_wc__entry_versioned(&entry, wcpath, adm_access, FALSE, pool));
>
> -
> - if (entry->kind == svn_node_file)
> - {
> - if (SVN_WC__CL_MATCH(changelist_hash, entry))
> - {
> - svn_info_t *info;
> - SVN_ERR(build_info_from_entry(&info, entry, pool));
> - return receiver(receiver_baton, wcpath, info, pool);
> - }
> - }
> - else if (entry->kind == svn_node_dir)
> - {
> - struct found_entry_baton fe_baton;
> - fe_baton.changelist_hash = changelist_hash;
> - fe_baton.receiver = receiver;
> - fe_baton.receiver_baton = receiver_baton;
> - SVN_ERR(svn_wc_walk_entries3(wcpath, adm_access,
> - &entry_walk_callbacks, &fe_baton,
> - depth, FALSE, ctx->cancel_func,
> - ctx->cancel_baton, pool));
> - }
> + fe_baton.changelist_hash = changelist_hash;
> + fe_baton.receiver = receiver;
> + fe_baton.receiver_baton = receiver_baton;
> + SVN_ERR(svn_wc_walk_entries3(wcpath, adm_access,
> + &entry_walk_callbacks, &fe_baton,
> + depth, FALSE, ctx->cancel_func,
> + ctx->cancel_baton, pool));
>
> return SVN_NO_ERROR;
> }
> Index: subversion/libsvn_client/merge.c
> ===================================================================
> --- subversion/libsvn_client/merge.c (revision 31702)
> +++ subversion/libsvn_client/merge.c (working copy)
> @@ -3392,15 +3392,11 @@ get_mergeinfo_paths(apr_array_header_t *
> /* Cover cases 1), 2), 6), and 7) by walking the WC to get all paths which
> have mergeinfo and/or are switched or are absent from disk or is the
> target of the merge. */
> - if (entry->kind == svn_node_file)
> - SVN_ERR(walk_callbacks.found_entry(merge_cmd_baton->target, entry, &wb,
> - pool));
> - else
> - SVN_ERR(svn_wc_walk_entries3(merge_cmd_baton->target, adm_access,
> - &walk_callbacks, &wb, depth, TRUE,
> - merge_cmd_baton->ctx->cancel_func,
> - merge_cmd_baton->ctx->cancel_baton,
> - pool));
> + SVN_ERR(svn_wc_walk_entries3(merge_cmd_baton->target, adm_access,
> + &walk_callbacks, &wb, depth, TRUE,
> + merge_cmd_baton->ctx->cancel_func,
> + merge_cmd_baton->ctx->cancel_baton,
> + pool));
>
> /* CHILDREN_WITH_MERGEINFO must be in depth first order, but
> svn_wc_walk_entries3() relies on svn_wc_entries_read() which means the
> Index: subversion/libsvn_client/prop_commands.c
> ===================================================================
> --- subversion/libsvn_client/prop_commands.c (revision 31702)
> +++ subversion/libsvn_client/prop_commands.c (working copy)
> @@ -86,6 +86,8 @@ struct propset_walk_baton
> const svn_string_t *propval; /* The value to set. */
> svn_wc_adm_access_t *base_access; /* Access for the tree being walked. */
> svn_boolean_t force; /* True iff force was passed. */
> + svn_boolean_t ignore_illegal_targets; /* Don't report failures to set
> + svn:executable on a dir, etc. */
> apr_hash_t *changelist_hash; /* Keys are changelists to filter on. */
> svn_wc_notify_func2_t notify_func;
> void *notify_baton;
> @@ -130,9 +132,11 @@ propset_walk_cb(const char *path,
> path, adm_access, wb->force, pool);
> if (err)
> {
> - if (err->apr_err != SVN_ERR_ILLEGAL_TARGET)
> + if (err->apr_err == SVN_ERR_ILLEGAL_TARGET
> + && wb->ignore_illegal_targets)
> + svn_error_clear(err);
> + else
> return err;
> - svn_error_clear(err);
> }
>
> /* Notify we updated a property value */
> @@ -388,6 +392,9 @@ svn_client_propset3(svn_commit_info_t **
> int adm_lock_level = SVN_WC__LEVELS_TO_LOCK_FROM_DEPTH(depth);
> const svn_wc_entry_t *entry;
> apr_hash_t *changelist_hash = NULL;
> + static const svn_wc_entry_callbacks2_t walk_callbacks
> + = { propset_walk_cb, svn_client__default_walker_error_handler };
> + struct propset_walk_baton wb;
>
> if (changelists && changelists->nelts)
> SVN_ERR(svn_hash_from_cstring_keys(&changelist_hash,
> @@ -399,28 +406,19 @@ svn_client_propset3(svn_commit_info_t **
> SVN_ERR(svn_wc__entry_versioned(&entry, target, adm_access,
> FALSE, pool));
>
> - if (depth >= svn_depth_files && entry->kind == svn_node_dir)
> - {
> - static const svn_wc_entry_callbacks2_t walk_callbacks
> - = { propset_walk_cb, svn_client__default_walker_error_handler };
> - struct propset_walk_baton wb;
> -
> - wb.base_access = adm_access;
> - wb.propname = propname;
> - wb.propval = propval;
> - wb.force = skip_checks;
> - wb.changelist_hash = changelist_hash;
> - wb.notify_func = ctx->notify_func2;
> - wb.notify_baton = ctx->notify_baton2;
> - SVN_ERR(svn_wc_walk_entries3(target, adm_access, &walk_callbacks,
> - &wb, depth, FALSE, ctx->cancel_func,
> - ctx->cancel_baton, pool));
> - }
> - else if (SVN_WC__CL_MATCH(changelist_hash, entry))
> - {
> - SVN_ERR(svn_wc_prop_set2(propname, propval, target,
> - adm_access, skip_checks, pool));
> - }
> + wb.base_access = adm_access;
> + wb.propname = propname;
> + wb.propval = propval;
> + wb.force = skip_checks;
> + wb.ignore_illegal_targets =
> + (depth >= svn_depth_files && entry->kind == svn_node_dir);
> + wb.changelist_hash = changelist_hash;
> + wb.notify_func = ctx->notify_func2;
> + wb.notify_baton = ctx->notify_baton2;
> + SVN_ERR(svn_wc_walk_entries3(target, adm_access, &walk_callbacks,
> + &wb, depth, FALSE, ctx->cancel_func,
> + ctx->cancel_baton, pool));
> +
> SVN_ERR(svn_wc_adm_close(adm_access));
> }
>
> @@ -842,17 +840,10 @@ svn_client__get_prop_from_wc(apr_hash_t
> wb.props = props;
>
> /* Fetch the property, recursively or for a single resource. */
> - if (depth >= svn_depth_files && entry->kind == svn_node_dir)
> - {
> - SVN_ERR(svn_wc_walk_entries3(target, adm_access,
> - &walk_callbacks, &wb, depth, FALSE,
> - ctx->cancel_func, ctx->cancel_baton,
> - pool));
> - }
> - else if (SVN_WC__CL_MATCH(changelist_hash, entry))
> - {
> - SVN_ERR(propget_walk_cb(target, entry, &wb, pool));
> - }
> + SVN_ERR(svn_wc_walk_entries3(target, adm_access,
> + &walk_callbacks, &wb, depth, FALSE,
> + ctx->cancel_func, ctx->cancel_baton,
> + pool));
>
> return SVN_NO_ERROR;
> }
> @@ -1236,15 +1227,15 @@ svn_client_proplist3(const char *path_or
> {
> svn_boolean_t pristine;
> int levels_to_lock = SVN_WC__LEVELS_TO_LOCK_FROM_DEPTH(depth);
> - const svn_wc_entry_t *entry;
> apr_hash_t *changelist_hash = NULL;
> + static const svn_wc_entry_callbacks2_t walk_callbacks
> + = { proplist_walk_cb, svn_client__default_walker_error_handler };
> + struct proplist_walk_baton wb;
>
> SVN_ERR(svn_wc_adm_probe_open3(&adm_access, NULL, path_or_url,
> FALSE, levels_to_lock,
> ctx->cancel_func, ctx->cancel_baton,
> pool));
> - SVN_ERR(svn_wc__entry_versioned(&entry, path_or_url, adm_access,
> - FALSE, pool));
>
> if ((revision->kind == svn_opt_revision_committed)
> || (revision->kind == svn_opt_revision_base))
> @@ -1260,34 +1251,16 @@ svn_client_proplist3(const char *path_or
> SVN_ERR(svn_hash_from_cstring_keys(&changelist_hash,
> changelists, pool));
>
> - /* Fetch, recursively or not. */
> - if (depth >= svn_depth_files && (entry->kind == svn_node_dir))
> - {
> - static const svn_wc_entry_callbacks2_t walk_callbacks
> - = { proplist_walk_cb, svn_client__default_walker_error_handler };
> - struct proplist_walk_baton wb;
> -
> - wb.base_access = adm_access;
> - wb.pristine = pristine;
> - wb.changelist_hash = changelist_hash;
> - wb.receiver = receiver;
> - wb.receiver_baton = receiver_baton;
> -
> - SVN_ERR(svn_wc_walk_entries3(path_or_url, adm_access,
> - &walk_callbacks, &wb, depth, FALSE,
> - ctx->cancel_func, ctx->cancel_baton,
> - pool));
> - }
> - else if (SVN_WC__CL_MATCH(changelist_hash, entry))
> - {
> - apr_hash_t *hash;
> -
> - SVN_ERR(pristine_or_working_props(&hash, path_or_url, adm_access,
> - pristine, pool));
> - SVN_ERR(call_receiver(path_or_url, hash,
> - receiver, receiver_baton, pool));
> -
> - }
> + /* Fetch the properties. */
> + wb.base_access = adm_access;
> + wb.pristine = pristine;
> + wb.changelist_hash = changelist_hash;
> + wb.receiver = receiver;
> + wb.receiver_baton = receiver_baton;
> + SVN_ERR(svn_wc_walk_entries3(path_or_url, adm_access,
> + &walk_callbacks, &wb, depth, FALSE,
> + ctx->cancel_func, ctx->cancel_baton,
> + pool));
>
> SVN_ERR(svn_wc_adm_close(adm_access));
> }
> Index: subversion/libsvn_wc/adm_ops.c
> ===================================================================
> --- subversion/libsvn_wc/adm_ops.c (revision 31702)
> +++ subversion/libsvn_wc/adm_ops.c (working copy)
> @@ -2909,20 +2909,9 @@ svn_wc_resolved_conflict3(const char *pa
> baton->notify_baton = notify_baton;
> baton->conflict_choice = conflict_choice;
>
> - if (depth == svn_depth_empty)
> - {
> - const svn_wc_entry_t *entry;
> - SVN_ERR(svn_wc__entry_versioned(&entry, path, adm_access, FALSE, pool));
> -
> - SVN_ERR(resolve_found_entry_callback(path, entry, baton, pool));
> - }
> - else
> - {
> - SVN_ERR(svn_wc_walk_entries3(path, adm_access,
> - &resolve_walk_callbacks, baton, depth,
> - FALSE, cancel_func, cancel_baton, pool));
> -
> - }
> + SVN_ERR(svn_wc_walk_entries3(path, adm_access,
> + &resolve_walk_callbacks, baton, depth,
> + FALSE, cancel_func, cancel_baton, pool));
>
> return SVN_NO_ERROR;
> }
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
> For additional commands, e-mail: dev-help_at_subversion.tigris.org
---------------------------------------------------------------------
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-12 09:55:31 CEST