You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@lyra.org> on 2002/05/20 21:00:10 UTC
FS tree walking (was: Re: wcprops)
On Mon, May 20, 2002 at 03:22:09PM -0500, cmpilato@collab.net wrote:
> Ben Collins-Sussman <su...@collab.net> writes:
> > Greg Stein <gs...@lyra.org> writes:
>...
> > > The flat list seems fine. I dislike using dir_delta as a mechanism to do a
> > > walk, though. It smacks too much of "everything is a nail for dir_delta to
> > > smack."
>..
> > manually walking an fs tree? Is it *that* much faster than using
> > dir_delta against rev 0?
>
> Much faster? I doubt it. At least, from the server side, dir_deltas
> is doing the the same things: svn_fs_dir_entries; loop; recurse.
It isn't about speed; it is about complexity. Setting up a call to
dir_delta, and the corresponding editor, is a PITA. And its semantic
overhead is way larger than the problem being solved.
Consider somebody coming along and reading the code. Consider how much they
are going to have to wade through to realize that you're doing a simple walk
over revision REV of the tree. At first, they're going to see a comparison
of two trees, then they'll realize that one is rev 0, and after another
ka-chunk, they'll realize you're just walking a tree.
That's a lot of brain effort for a simple "walk" operation.
Consider the dir_delta function, and a hypothetical walk function:
svn_error_t *
svn_repos_dir_delta (svn_fs_root_t *src_root,
const char *src_parent_dir,
const char *src_entry,
svn_fs_root_t *tgt_root,
const char *tgt_path,
const svn_delta_edit_fns_t *editor,
void *edit_baton,
svn_boolean_t text_deltas,
svn_boolean_t recurse,
svn_boolean_t entry_props,
svn_boolean_t use_copy_history,
apr_pool_t *pool);
compare that to:
svn_error_t *
svn_repos_walk_tree (svn_fs_root_t *root,
const char *path, /* starting path locn */
svn_repos_walk_func_t callback,
void *cb_baton,
apr_pool_t *pool);
where you have:
typedef svn_error_t * (*svn_repos_walk_func_t) (const char *path,
void *baton,
apr_pool_t *pool);
As soon as a reader sees the call to walk_tree, they will know exactly what
is going on. No further reading necessary.
[ and if anybody says "well, just insert a comment before the call to
dir_delta to explain that a walk is occurring", I'm going to have to break
their legs. as I've said before, comments can be crutches to writing clear
code in the first place. this is a perfect example of that. ]
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: FS tree walking (was: Re: wcprops)
Posted by Greg Stein <gs...@lyra.org>.
On Mon, May 20, 2002 at 04:22:58PM -0500, Ben Collins-Sussman wrote:
> Greg Stein <gs...@lyra.org> writes:
> > On Mon, May 20, 2002 at 04:00:56PM -0500, Ben Collins-Sussman wrote:
> > >...
> > > uc.resource_walk = TRUE;
> >
> > And let's also not ignore the fact that the above line puts the editor into
> > a different "mode". The editor callbacks now have two potential code paths,
> > based on a giant mode switch set by uc.resource_walk.
> >
> > (i.e. the original editor used to describe the changes to do the switch is
> > now complicated because it has this second code-path wound through it)
>...
> It's not conceptually clean to do such a thing, but it turned out [in
> this particular case] to require almost no change to the editor at
> all. Just lucky.
I just saw your checkin. To me, it looks like a pretty big change to the
editor. There are six separate "if (uc->resource_walk)" tests. To me, that
means that six new alternate code paths have been introduced. While it
happens to "fit in" with the editor, I don't see it as "almost no change".
*shrug*
When the walker function comes around, then we can clean the stuff up.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: FS tree walking (was: Re: wcprops)
Posted by Ben Collins-Sussman <su...@collab.net>.
Greg Stein <gs...@lyra.org> writes:
> On Mon, May 20, 2002 at 04:00:56PM -0500, Ben Collins-Sussman wrote:
> >...
> > uc.resource_walk = TRUE;
>
> And let's also not ignore the fact that the above line puts the editor into
> a different "mode". The editor callbacks now have two potential code paths,
> based on a giant mode switch set by uc.resource_walk.
>
> (i.e. the original editor used to describe the changes to do the switch is
> now complicated because it has this second code-path wound through it)
Heh, I originally read the phrase "wound through it", and imagined a
long vowel, and thought to myself, "why yes, it is like a wound, an
ugly gash across the editor." :-)
It's not conceptually clean to do such a thing, but it turned out [in
this particular case] to require almost no change to the editor at
all. Just lucky.
Anyone want to write a generic fs walker?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: FS tree walking (was: Re: wcprops)
Posted by Greg Stein <gs...@lyra.org>.
On Mon, May 20, 2002 at 04:00:56PM -0500, Ben Collins-Sussman wrote:
>...
> uc.resource_walk = TRUE;
And let's also not ignore the fact that the above line puts the editor into
a different "mode". The editor callbacks now have two potential code paths,
based on a giant mode switch set by uc.resource_walk.
(i.e. the original editor used to describe the changes to do the switch is
now complicated because it has this second code-path wound through it)
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: FS tree walking (was: Re: wcprops)
Posted by Ben Collins-Sussman <su...@collab.net>.
Greg Stein <gs...@lyra.org> writes:
> svn_error_t *
> svn_repos_walk_tree (svn_fs_root_t *root,
> const char *path, /* starting path locn */
> svn_repos_walk_func_t callback,
> void *cb_baton,
> apr_pool_t *pool);
>
> where you have:
>
> typedef svn_error_t * (*svn_repos_walk_func_t) (const char *path,
> void *baton,
> apr_pool_t *pool);
I agree with Greg. If we had a nice interface such as the one above,
then I wouldn't need to use dir_delta as a convenience, and things
would be easier to understand.
My code in mod_dav_svn looks like this right now. Not horrible, but
could be simpler:
if (dst_path) /* this was a 'switch' operation */
{
/* send a second embedded <S:resource-walk> tree that contains
the new vsn-rsc-urls for the switched dir. this walk
contains essentially nothing but <add> tags. */
svn_fs_root_t *zero_root;
svn_fs_revision_root(&zero_root, repos->fs, 0, resource->pool);
send_xml(&uc, "<S:resource-walk>" DEBUG_CR);
uc.resource_walk = TRUE;
/* Compare subtree DST_PATH within a pristine revision to
revision 0. This should result in nothing but 'add' calls
to the editor. */
serr = svn_repos_dir_delta(/* source is revision 0: */
zero_root, "", NULL,
/* target is 'switch' location: */
uc.rev_root, dst_path,
/* re-use the editor */
editor, &uc,
FALSE, /* no text deltas */
recurse,
TRUE, /* send entryprops */
FALSE, /* no copy history */
resource->pool);
send_xml(&uc, "</S:resource-walk>" DEBUG_CR);
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org