Nice commit! I've committed a few doc tweaks in r27514. Review
questions below:
glasser@tigris.org writes:
> --- (empty file)
> +++ trunk/subversion/libsvn_wc/ambient_depth_filter_editor.c
> @@ -0,0 +1,572 @@
>
> [...]
>
> +static svn_error_t *
> +open_directory(const char *path,
> + void *parent_baton,
> + svn_revnum_t base_revision,
> + apr_pool_t *pool,
> + void **child_baton)
> +{
> + struct dir_baton *pb = parent_baton;
> + struct edit_baton *eb = pb->edit_baton;
> + struct dir_baton *b;
> + const svn_wc_entry_t *entry;
> +
> + SVN_ERR(make_dir_baton(&b, path, eb, pb, pool));
> + *child_baton = b;
> +
> + if (b->ambient_depth == svn_depth_exclude)
> + return SVN_NO_ERROR;
> +
> + SVN_ERR(eb->wrapped_editor->open_directory(path, pb->wrapped_baton,
> + base_revision, pool,
> + &b->wrapped_baton));
> + /* Note that for the update editor, the open_directory above will
> + flush the logs of pb's directory, which might be important for
> + this svn_wc_entry call. */
> + SVN_ERR(svn_wc_entry(&entry, b->path, eb->adm_access, FALSE, pool));
> + if (entry)
> + b->ambient_depth = entry->depth;
> +
> + return SVN_NO_ERROR;
> +}
Could you explain how it might be important? That is, does the
correctness of the result of the svn_wc_entry() call *depend* on the
wrapped_editor->open_directory() call in some way?
> --- trunk/subversion/libsvn_wc/wc.h (original)
> +++ trunk/subversion/libsvn_wc/wc.h Thu Oct 25 16:40:35 2007
> @@ -274,6 +274,33 @@
> void *walk_baton,
> apr_pool_t *pool);
>
> +/* Set *EDITOR and *EDIT_BATON to an ambient-depth-based filtering
> + * editor that wraps WRAPPED_EDITOR and WRAPPED_BATON. Only required
> + * if REQUESTED_DEPTH is svn_depth_unknown and the editor driver
> + * doesn't understand depth.
> + *
> + * ANCHOR, TARGET, and ADM_ACCESS are as in svn_wc_get_update_editor3.
> + *
> + * @a requested_depth must be one of the following depth values:
> + * @c svn_depth_infinity, @c svn_depth_empty, @c svn_depth_files,
> + * @c svn_depth_immediates, or @c svn_depth_unknown.
> + *
> + * If filtering is deemed unncessary (REQUESTED_DEPTH is not
> + * svn_depth_unknown), *EDITOR and *EDIT_BATON will be set to
> + * WRAPPED_EDITOR and WRAPPED_BATON, respectively; otherwise,
> + * they'll be set to new objects allocated from POOL.
> + */
There's a minor conceptual inconsistency here, I think.
svn_wc__ambient_depth_filter_editor() takes a 'requested_depth'
parameter, which it only uses in a local conditional (e.g., it never
stores the requested_depth in the wrapper's edit_baton). This is the
conditional:
if (requested_depth != svn_depth_unknown)
{
*editor = wrapped_editor;
*edit_baton = wrapped_edit_baton;
return SVN_NO_ERROR;
}
Great. But from the doc string above, it's not clear whether we
should have called svn_wc__ambient_depth_filter_editor() at all...
That is, is it the caller's responsibility to check requested_depth
and decide whether or not to wrap? Or should the caller always wrap
(passing requested_depth), but the returned editor might be the
original editor, if requested_depth == svn_depth_unknown?
We should pick one way and enforce it. I think the way to do that is:
if (requested_depth != svn_depth_unknown)
{
svn_wc__ambient_depth_filter_editor(&editor, &edit_baton,
editor, edit_baton,
anchor, target, adm_access,
requested_depth, pool);
}
...and just ensure that it is written in such a way that that is safe!
We'd change the calling code so that it wouldn't have the extra
variables to hold the wrapper editor, and change the callee to not
take a requested_depth parameter at all.
Thoughts?
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Oct 31 05:22:16 2007