[prev in list] [next in list] [prev in thread] [next in thread] 

List:       subversion-dev
Subject:    Re: svn commit: r27405 - trunk/subversion/libsvn_wc
From:       "David Glasser" <glasser () davidglasser ! net>
Date:       2007-10-31 19:11:38
Message-ID: 1ea387f60710311211s3e5b3240g4c4362a3aeabcda6 () mail ! gmail ! com
[Download RAW message or body]

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

[prev in list] [next in list] [prev in thread] [next in thread]