You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ben Collins-Sussman <su...@collab.net> on 2002/05/20 16:36:20 UTC
wcprops
Beating a dead horse, regarding issue #704 ('switch' rewriting
vsn-rsc-urls)...
I've hit a wrench in the solution I'm implementing, and really, it
seems to speak against the solution gstein suggested as well.
The current solution I'm using (which is almost like gstein's solution):
- on the server side, we get a free fs 'walk' from dir_delta by
comparing the switched tree against revision 0. dir_delta is
driving the xml-output editor in a secondary <resource-walk> mode,
which produces a *flat* list of <resource> elements that contain
just full paths and vsn-rsc-urls.
- on the client side, we've expanded the parser's knowledge of the
two new xml elements. when it sees a <resource>, it simply calls
the set_wc_prop callback on the full path.
PROBLEM: It's useless to send back a flat list of absolute fs-paths
and vsn-rsc-urls, because we don't know *where* to apply them in the
wc. Only the update-editor has been properly "pre-anchored" in the
working copy, and thus the only safe way to set wcprops is through
this editor. (...unless we want to pass new arguments into
RA->do_switch() that indicate where the update-editor is anchored,
bleah!)
So if ra_dav is going to have to re-drive the update-editor to set
wcprops, then the resource-walk tree should *look* like an editor
drive, full of replace_* calls. (I mean, it could look completely
different, but what's the point? Why write a new parser to drive the
same editor?)
Following the logic through: if the resource-walk tree must look like
an editor drive, then it makes sense for mod_dav_svn to re-use its
xml-editor, and make 'adds' look like 'replaces' (passing
SVN_INVALID_REVNUM for the base-revision args.)
Back to square 1 again. Essentially what Karl suggested: tweak the
editor docstring promise, so that the base-revision sanity check is
optional. Grrrrrr.
---------------------------------------------------------------------
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
FS tree walking (was: Re: wcprops)
Posted by Greg Stein <gs...@lyra.org>.
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: wcprops
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On Mon, May 20, 2002 at 03:22:09PM -0500, cmpilato@collab.net wrote:
> I personally think that dir_delta is not such a wild hammer that it
> requires special caution in its use -- it's not a Hole Hawg; just a
> perfectly good DeWalt commercial-strength power drill.
good to see i wasn't the only one who read 'in the beginning was the
command line' ;-)
-garrett
--
garrett rooney Remember, any design flaw you're
rooneg@electricjellyfish.net sufficiently snide about becomes
http://electricjellyfish.net/ a feature. -- Dan Sugalski
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: wcprops
Posted by cm...@collab.net.
Ben Collins-Sussman <su...@collab.net> writes:
> Greg Stein <gs...@lyra.org> writes:
>
> > On Mon, May 20, 2002 at 11:36:20AM -0500, Ben Collins-Sussman wrote:
> > >...
> > > - on the server side, we get a free fs 'walk' from dir_delta by
> > > comparing the switched tree against revision 0. dir_delta is
> > > driving the xml-output editor in a secondary <resource-walk> mode,
> > > which produces a *flat* list of <resource> elements that contain
> > > just full paths and vsn-rsc-urls.
> >
> > 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."
>
> I guess I'm following svnlook's lead... it passes rev 0 to dir_delta
> as a way of printing full trees. Honestly, I've never understood why
> we don't use this method in our RA layers for performing checkouts.
> Why does each RA layer's do_checkout() have its own implementation of
> 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.
I personally think that dir_delta is not such a wild hammer that it
requires special caution in its use -- it's not a Hole Hawg; just a
perfectly good DeWalt commercial-strength power drill.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: wcprops
Posted by Ben Collins-Sussman <su...@collab.net>.
Greg Stein <gs...@lyra.org> writes:
> On Mon, May 20, 2002 at 11:36:20AM -0500, Ben Collins-Sussman wrote:
> >...
> > - on the server side, we get a free fs 'walk' from dir_delta by
> > comparing the switched tree against revision 0. dir_delta is
> > driving the xml-output editor in a secondary <resource-walk> mode,
> > which produces a *flat* list of <resource> elements that contain
> > just full paths and vsn-rsc-urls.
>
> 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."
I guess I'm following svnlook's lead... it passes rev 0 to dir_delta
as a way of printing full trees. Honestly, I've never understood why
we don't use this method in our RA layers for performing checkouts.
Why does each RA layer's do_checkout() have its own implementation of
manually walking an fs tree? Is it *that* much faster than using
dir_delta against rev 0?
I think what's bugging me here is that I don't want to write Yet A
Third Tree Walker for libsvn_fs. Where's the code re-use? I think
what we really want is a single generic fs-tree-walker with callbacks,
that *all* RA layers can use.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: wcprops
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Greg Stein <gs...@lyra.org> writes:
> > Back to square 1 again. Essentially what Karl suggested: tweak the
> > editor docstring promise, so that the base-revision sanity check is
> > optional. Grrrrrr.
>
> Nope nope... much simpler. Just tweak the return paths.
>
> The base-revision should remain a strong condition (and/or docs clarified)
> regardless of this change, but editor drives are overkill here.
I'd like to clarify: I wasn't suggesting that using the editor was the
cleanest solution, merely that the vague conditions imposed on the
base_revision argument shouldn't stand in the way. If using editor is
undesirable for other reasons, then that's different.
(I don't think base_revision should be a strong requirement anyway,
though. If you pass SVN_INVALID_REVNUM, it should just ignore the
revision.)
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: wcprops
Posted by Greg Stein <gs...@lyra.org>.
On Mon, May 20, 2002 at 11:36:20AM -0500, Ben Collins-Sussman wrote:
>...
> - on the server side, we get a free fs 'walk' from dir_delta by
> comparing the switched tree against revision 0. dir_delta is
> driving the xml-output editor in a secondary <resource-walk> mode,
> which produces a *flat* list of <resource> elements that contain
> just full paths and vsn-rsc-urls.
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."
>...
> PROBLEM: It's useless to send back a flat list of absolute fs-paths
> and vsn-rsc-urls, because we don't know *where* to apply them in the
Ben and I talked about this, and I'm copying the result here.
The problem is sending back absolute FS paths, rather than relative to where
the switch report was based. If the server simply sends back a relative
path, then the client can pass that along to set_wc_prop().
[ at the moment, the client doesn't need any additional prefixes; the base
of the report is always the same as the session root URL; the path to
set_wc_prop is relative to that point, so everything is aligned. In some
future situation, where we may have one root and multiple sub-reports,
then the returning paths would be relative to the sub-report and a prefix
would be needed... just not today. ]
>...
> So if ra_dav is going to have to re-drive the update-editor to set
> wcprops, then the resource-walk tree should *look* like an editor
> drive,
But it won't have to drive the update editor, since you've got correct paths
for set_wc_prop, so everything is happy-happy.
>...
> Back to square 1 again. Essentially what Karl suggested: tweak the
> editor docstring promise, so that the base-revision sanity check is
> optional. Grrrrrr.
Nope nope... much simpler. Just tweak the return paths.
The base-revision should remain a strong condition (and/or docs clarified)
regardless of this change, but editor drives are overkill here.
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