I would like to merge this to my branch, once it is ready. Great job.
Rui, Guo
On Thu, Jun 12, 2008 at 03:55:21AM -0400, Karl Fogel wrote:
> 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
>
---------------------------------------------------------------------
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 10:09:34 CEST