You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Karl Fogel <kf...@red-bean.com> on 2007/10/31 04:22:05 UTC
Re: svn commit: r27405 - trunk/subversion/libsvn_wc
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
Re: svn commit: r27405 - trunk/subversion/libsvn_wc
Posted by Karl Fogel <kf...@red-bean.com>.
"David Glasser" <gl...@davidglasser.net> writes:
>> > OK, and we might as well leave out the requested_depth parameter while
>> > we're at it?
>>
>> Yup, exactly.
>>
>> Note that you'll have to play some const games to get the in-out
>> behavior with 'editor' and '&editor', but it should be doable.
>
> r27640.
Reviewed, looks good to me.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r27405 - trunk/subversion/libsvn_wc
Posted by David Glasser <gl...@davidglasser.net>.
On 11/1/07, Karl Fogel <kf...@red-bean.com> wrote:
> "David Glasser" <gl...@davidglasser.net> writes:
> >> 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?
>
> Yup, exactly.
>
> Note that you'll have to play some const games to get the in-out
> behavior with 'editor' and '&editor', but it should be doable.
r27640.
--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
Re: svn commit: r27405 - trunk/subversion/libsvn_wc
Posted by Karl Fogel <kf...@red-bean.com>.
"David Glasser" <gl...@davidglasser.net> writes:
>> 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?
Yup, exactly.
Note that you'll have to play some const games to get the in-out
behavior with 'editor' and '&editor', but it should be doable.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r27405 - trunk/subversion/libsvn_wc
Posted by David Glasser <gl...@davidglasser.net>.
On 10/30/07, Karl Fogel <kf...@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