Re: svn commit: r27405 - trunk/subversion/libsvn_wc

On 10/30/07, Karl Fogel <kfogel@red-bean.com> wrote:
> Nice commit! I've committed a few doc tweaks in r27514. Review
> questions below:

Thanks for the followup.

> 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?

I'm not sure if it is. I don't *think* it's possible for any of the
actions performed by the logs in the parent directory to affect the
directory being opened. However, I had to make the choice between
calling svn_wc_entry() before or after calling
wrapped_editor->open_directory(), and I decided to make the choice
which was consistent with how the code worked before extracting the
editor wrapper, and make it explicit that this *might* be a reason to
keep the svn_wc_entry() call below the open_directory(). But again,
I'm not sure.

> > --- 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.

You're right. I wasn't sure which choice to make and ended up making a
bit of both.

> 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.

OK, and we might as well leave out the requested_depth parameter while
we're at it?

--dave

-- 
David Glasser | glasser_at_davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org