vgeorgescu@tigris.org writes:
> Log:
> Add depth support to svn_client_proplist3.
>
> [...]
Yay!
A few comments below...
> --- trunk/subversion/include/svn_client.h (original)
> +++ trunk/subversion/include/svn_client.h Sun May 13 10:10:40 2007
> @@ -3083,15 +3083,13 @@
> * baton cached in @a ctx for authentication if contacting the
> * repository.
> *
> - * If @a recurse is false, or @a target is a file, @a *props will contain
> - * only a single element. Otherwise, it will contain one element for each
> - * versioned entry below (and including) @a target.
> - *
> - * ### TODO(sd): I don't see any reason to change this recurse parameter
> - * ### to a depth right now; it's not exactly part of the
> - * ### sparse-directories feature, although it's related. Usually
> - * ### you would just name the target carefully... Is there a
> - * ### situation where depth support would be useful here?
> + * If @a depth is @c svn_depth_empty, list only the properties of
> + * @a path_or_url itself. If @a depth is @c svn_depth_files, and
> + * @a path_or_url is a directory, list the properties of @a path_or_url
> + * and its file entries. If @c svn_depth_immediates, list the properties
> + * of its immediate file and directory entries. If @c svn_depth_infinity,
> + * list the properties of its file entries and recurse (with
> + * @c svn_depth_infinity) on directory entries.
> *
> * If @a target is not found, return the error @c SVN_ERR_ENTRY_NOT_FOUND.
> *
Should we also document the behavior for svn_depth_unknown? Or at
least document that the behavior for depths not mentioned here is
undefined? I think we haven't been consistent about doing that
elsewhere, but that might just mean those other places should be fixed
too :-).
> @@ -3110,7 +3108,9 @@
> /**
> * Similar to svn_client_proplist3(), except the properties are returned
> * as an array of @c svn_client_proplist_item_t * structures, instead of
> - * by invoking the receiver function.
> + * by invoking the receiver function, and @a recurse is used instead of
> + * a svn_depth_t parameter (FALSE corresponds to svn_depth_empty, and TRUE
> + * to svn_depth_infinity).
> *
> * @since New in 1.2.
> *
I added the @c formatting codes to this doc string in r25020.
> ==============================================================================
> --- trunk/subversion/libsvn_client/prop_commands.c (original)
> +++ trunk/subversion/libsvn_client/prop_commands.c Sun May 13 10:10:40 2007
> @@ -931,8 +932,9 @@
>
> if (kind == svn_node_dir)
> {
> - SVN_ERR(svn_ra_get_dir2(ra_session, (recurse ? &dirents : NULL), NULL,
> - &prop_hash, target_relative, revnum,
> + SVN_ERR(svn_ra_get_dir2(ra_session,
> + (depth > svn_depth_empty) ? &dirents : NULL,
> + NULL, &prop_hash, target_relative, revnum,
> SVN_DIRENT_KIND, scratchpool));
> }
> else if (kind == svn_node_file)
I'm not sure we have any other code that depends on numeric comparison
of two depths; it encodes assumptions about the depths' values that
may not hold in the future. We can't predict what new depths we might
add, or what their interpretations might be relative to existing
depths. So maybe we should write like this instead:
if (kind == svn_node_dir)
{
apr_hash_t **dirents_p;
if (depth == svn_depth_files
|| depth == svn_depth_immediates
|| depth == svn_depth_infinity)
dirents_p = &dirents;
else
dirents_p = NULL;
SVN_ERR(svn_ra_get_dir2(ra_session, dirents_p, NULL,
&prop_hash, target_relative, revnum,
SVN_DIRENT_KIND, scratchpool));
}
(We could also make a macro if this idiom occurs often enough, but I'm
not sure it does.)
And of course, what we document for 'svn_depth_unknown' and other
invalid depths will affect exactly how we write the conditional above:
I don't mean to imply that just 'else' is completely correct, I'm just
drafting here.
> @@ -979,7 +981,8 @@
> final_hash, receiver, receiver_baton,
> pool);
>
> - if (recurse && (kind == svn_node_dir) && (apr_hash_count(dirents) > 0))
> + if (depth > svn_depth_empty
> + && (kind == svn_node_dir) && (apr_hash_count(dirents) > 0))
> {
> apr_pool_t *subpool = svn_pool_create(scratchpool);
Same concern about numeric comparison applies here.
> @@ -991,7 +994,7 @@
> void *val;
> const char *this_name;
> svn_dirent_t *this_ent;
> - const char *new_target_relative;
> + const char *new_target_relative;
>
> svn_pool_clear(subpool);
Spurious change there, just adds some space to the end of the line :-).
> @@ -1002,16 +1005,25 @@
> new_target_relative = svn_path_join(target_relative,
> this_name, subpool);
>
> - SVN_ERR(remote_proplist(target_prefix,
> - new_target_relative,
> - this_ent->kind,
> - revnum,
> - ra_session,
> - recurse,
> - receiver,
> - receiver_baton,
> - pool,
> - subpool));
> + if (this_ent->kind == svn_node_file
> + || depth > svn_depth_files)
> + {
Same numeric comparison concern here.
> + svn_depth_t depth_below_here = depth;
> +
> + if (depth == svn_depth_immediates)
> + depth_below_here = svn_depth_empty;
> +
> + SVN_ERR(remote_proplist(target_prefix,
> + new_target_relative,
> + this_ent->kind,
> + revnum,
> + ra_session,
> + depth_below_here,
> + receiver,
> + receiver_baton,
> + pool,
> + subpool));
> + }
> }
Not sure the temporary variable is necessary, even for clarity (here
is one place where you could do an inline "foo ? bar : baz"), but
that's a style judgement, feel free to ignore me.
> svn_pool_destroy(subpool);
> @@ -1073,7 +1085,7 @@
> svn_client_proplist3(const char *target,
> const svn_opt_revision_t *peg_revision,
> const svn_opt_revision_t *revision,
> - svn_boolean_t recurse,
> + svn_depth_t depth,
> svn_proplist_receiver_t receiver,
> void *receiver_baton,
> svn_client_ctx_t *ctx,
> @@ -1087,6 +1099,9 @@
>
> SVN_ERR(maybe_convert_to_url(&utarget, target, revision, pool));
>
> + if (depth == svn_depth_unknown)
> + depth = svn_depth_empty;
> +
Hey, this conversion isn't documented, is it? :-)
> /* Iff utarget is a url, that means we must use it, that is, the
> requested property information is not available locally. */
> if (svn_path_is_url(utarget))
> @@ -1104,16 +1119,22 @@
>
> SVN_ERR(remote_proplist(url, "",
> kind, revnum, ra_session,
> - recurse, receiver, receiver_baton,
> + depth, receiver, receiver_baton,
> pool, subpool));
> svn_pool_destroy(subpool);
> }
> else /* working copy path */
> {
> svn_boolean_t pristine;
> + int adm_depth = -1;
> +
> + if (depth == svn_depth_empty || depth == svn_depth_files)
> + adm_depth = 0;
> + else if (depth == svn_depth_immediates)
> + adm_depth = 1;
It might be good to have a comment explaining that "int adm_depth = -1;"
is the correct initialization for svn_depth_infinity. I had to look
twice before I realized what was going on.
> SVN_ERR(svn_wc_adm_probe_open3(&adm_access, NULL, target,
> - FALSE, recurse ? -1 : 0,
> + FALSE, adm_depth,
> ctx->cancel_func, ctx->cancel_baton,
> pool));
> SVN_ERR(svn_wc__entry_versioned(&node, target, adm_access, FALSE, pool));
> @@ -1132,7 +1153,7 @@
> }
>
> /* Fetch, recursively or not. */
> - if (recurse && (node->kind == svn_node_dir))
> + if (depth == svn_depth_infinity && (node->kind == svn_node_dir))
> {
> static const svn_wc_entry_callbacks_t walk_callbacks
> = { proplist_walk_cb };
> @@ -1150,11 +1171,55 @@
> }
> else
> {
> - apr_hash_t *hash;
> + apr_hash_t *hash;
>
> SVN_ERR(pristine_or_working_props(&hash, target, adm_access,
> pristine, pool));
> SVN_ERR(call_receiver(target, hash, receiver, receiver_baton, pool));
> +
> + /* If depth is at least svn_depth_files and target is a directory,
> + we'll need to iterate over the directory entries. */
> + if (depth > svn_depth_empty && (node->kind == svn_node_dir))
> + {
Same numeric comparison issue here. (I'm beginning to think that
maybe we *will* want a macro...)
> + apr_hash_t *entries;
> + apr_hash_index_t *hi;
> + apr_pool_t *iterpool = svn_pool_create(pool);
> +
> + SVN_ERR(svn_wc_entries_read(&entries, adm_access, FALSE, pool));
> + for (hi = apr_hash_first(pool, entries); hi; hi = apr_hash_next(hi))
> + {
> + const void *key;
> + void *val;
> + const svn_wc_entry_t *current_entry;
> + const char *path;
> +
> + apr_hash_this(hi, &key, NULL, &val);
> + current_entry = val;
> +
> + /* We already handled THIS_DIR. */
> + if (strcmp(key, SVN_WC_ENTRY_THIS_DIR) == 0)
> + continue;
> +
> + path = svn_path_join(target, key, iterpool);
> +
> + if (current_entry->kind == svn_node_file
> + || depth == svn_depth_immediates)
> + {
> + /* Ignore the entry if it does not exist at the time
> + of interest. */
> + if (current_entry->schedule
> + == (pristine ? svn_wc_schedule_add
> + : svn_wc_schedule_delete))
> + continue;
> +
> + SVN_ERR(pristine_or_working_props(&hash, path,
> + adm_access,
> + pristine, iterpool));
> + SVN_ERR(call_receiver(path, hash, receiver,
> + receiver_baton, iterpool));
> + }
> + }
> + }
> }
You declared an 'iterpool', but never clear or destroy it.
> SVN_ERR(svn_wc_adm_close(adm_access));
> @@ -1210,7 +1275,8 @@
> pl_baton.props = *props;
> pl_baton.pool = pool;
>
> - SVN_ERR(svn_client_proplist3(target, peg_revision, revision, recurse,
> + SVN_ERR(svn_client_proplist3(target, peg_revision, revision,
> + recurse ? svn_depth_infinity : svn_depth_empty,
> proplist_receiver_cb, &pl_baton, ctx, pool));
Ah, right, we can't use the SVN_DEPTH_FROM_RECURSE() macro here,
because proplist3() promises a slightly different behavior than most
things. Makes sense. Might be worth a comment, though.
Bravo for taking care of this API, Vlad! I hope the above comments help.
Best,
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue May 15 09:05:30 2007