You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Garrett Rooney <ro...@electricjellyfish.net> on 2005/10/25 20:09:08 UTC
[PATCH] improve speed of non-verbose svn list
So I followed through on my recently discussed plans regarding
providing a way for the client to indicate which parts of the
svn_dirent_t they really care about when calling svn_client_ls.
Here's a patch that implements two new functions svn_client_ls4 and
svn_ra_get_dir2 that provide this capability and changes the command
line client to use svn_client_ls4. When we're not in verbose or XML
mode for list we only ask for the kind field of the dirent to be
filled in, and as a result the speed of the operation is greatly
improved.
It turns out that this requires no changes to mod_dav_svn, as DAV
already provides us enough functionality to control what properties
we're interested in. Note that there is still some inefficiency in
the way has_props is calculated, and the addition of the
deadprops-count liveprop (from Jean-Marc Godbout's patch in issue
2151) would certainly help in that case, but it isn't necessary to
improve the non-verbose case, so I've left that for another day.
The svnserve changes here are the minimum required to make this work.
It does result in a speed increase simply from not calculating the
information on the server side, but we are still using extra bandwidth
to transmit even the portions of the dirent we don't care about. If
someone is motivated enough it would be possible to fix this, but it
is a more complex change.
I haven't done any work on libsvn_ra_local here, so it is still
exactly the same speed it was before.
I've tested svnserve with both old and new servers and clients, and
everything works as expected. Since this hasn't required any
mod_dav_svn changes there are no compatibility problems there.
While investigating this I may have found some ways to speed up the
general case for ra_dav (which is still slow much slower than ra_svn
in the verbose case), but regardless I still think these changes have
merit, since no matter how fast we make the calculation of the dirent
fields it's still silly to calculate and send them when they are never
even used.
Any comments on this would be greatly appreciated.
-garrett
[[[
Speed up non-verbose 'svn list' by a scary amount by allowing the server
to avoid calculating parts of the svn_dirent_t objects that are never used.
* subversion/libsvn_ra/ra_loader.c
(svn_ra_get_dir2): new wrapper function.
* subversion/libsvn_ra/ra_loader.h
(svn_ra__vtable_t): add get_dir2 function.
* subversion/include/svn_types.h
(SVN_DIRENT_KIND,
SVN_DIRENT_SIZE,
SVN_DIRENT_HAS_PROPS,
SVN_DIRENT_CREATED_REV,
SVN_DIRENT_TIME,
SVN_DIRENT_LAST_AUTHOR): new bitfield values for each field in a dirent.
(SVN_DIRENT_ALL): combination of all the other bitfield values.
* subversion/include/svn_client.h
(svn_client_ls4): new function, like svn_client_ls3, but with a dirent
fields parameter.
(svn_client_ls3): deprecated in favor of svn_client_ls4.
* subversion/include/svn_ra.h
(svn_ra_get_dir2): new version of svn_ra_get_dir, takes a dirent fields
parameter.
(svn_ra_get_dir): deprecate in favor of svn_ra_get_dir2.
* subversion/libsvn_ra_local/ra_plugin.c
(svn_ra_local__get_dir2): new function, currently ignores the dirent
fields parameter.
(svn_ra_local__get_dir): call into svn_ra_local__get_dir, passing it
SVN_DIRENT_ALL for the dirent_fields parameter.
(ra_local_vtable): add svn_ra_Local__get_dir2.
* subversion/libsvn_client/ls.c
(get_dir_contents): add a dirent_fields parameter, use svn_ra_get_dir2,
update recursive call.
(svn_client_ls4): new list function, adds a dirent_fields parameter which
is passed to get_dir_contents.
(svn_client_ls3): backwards compat wrapper, calls svn_client_ls4 with the
dirent_fields set to SVN_DIRENT_ALL.
* subversion/clients/cmdline/ls-cmd.c
(svn_cl__ls): use svn_client_ls4, only asking for the dirent fields we
actually need.
* subversion/libsvn_ra_svn/client.c
(ra_svn_get_dir2): new version of get_dir, pass new dirent_fields
parameter along to the server.
(ra_svn_get_dir): pass through to new function, passing SVN_DIRENT_ALL
for dirent_fields.
(ra_svn_vtable): add ra_svn_get_dir2.
* subversion/libsvn_ra_dav/ra_dav.h
(svn_ra_dav__get_dir2): new prototype.
* subversion/libsvn_ra_dav/session.c
(dav_vtable): add svn_ra_dav__get_dir2.
* subversion/libsvn_ra_dav/fetch.c
(svn_ra_dav__get_dir2): new version of get_dir, takes a dirent fields
arg, dynamically figure out which props we need to ask for to fill in
the dirent fields that were asked for. fall back to asking for all
of them in the has_props case, since we currently have no other way
to get that data. only fill in the parts of the dirent we were asked
to get. create and destroy a new subpool, which is used to allocate
the temporary neon properties array.
(svn_ra_dav__get_dir): compat wrapper, passes SVN_DIRENT_ALL as the
dirent_flags arg to svn_ra_dav__get_dir2.
* subversion/svnserve/serve.c
(get_dir): accept an optional dirent fields parameter from the client,
defaulting to SVN_DIRENT_ALL if it's not present. only initialize the
parts of the dirents that the client asked for.
]]]
Re: [PATCH] improve speed of non-verbose svn list
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 10/25/05, Julian Foad <ju...@btopenworld.com> wrote:
> Garrett Rooney wrote:
> >
> > Any comments on this would be greatly appreciated.
>
> Seems to break recursive lists, making them non-recursive.
So it seems. Well, there's one problem in svn_client_ls4, it needs to
always ensure that we set SVN_DIRENT_KIND, since kind is used to see
if we should recurse... But that doesn't seem to fix things for
ra_dav. I'll dig in some more and see what the problem is.
Thanks for the bug report,
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] improve speed of non-verbose svn list
Posted by Julian Foad <ju...@btopenworld.com>.
Garrett Rooney wrote:
>
> Any comments on this would be greatly appreciated.
Seems to break recursive lists, making them non-recursive.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] improve speed of non-verbose svn list
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 10/25/05, Osku Salerma <os...@iki.fi> wrote:
> On Tue, 25 Oct 2005, Garrett Rooney wrote:
>
> > [...] and as a result the speed of the operation is greatly improved.
>
> Quantifying "greatly" with actual numbers would be nice.
Before the patch, listing a directory with 800 children over ra_dav
takes between 4 and 5 seconds, with both the client and the server
running on my local machine. After the patch listing the same
directory takes between 0.1 and 0.2 seconds. The speed increase for
ra_svn is similar, although since ra_svn started out faster the gain
is not as extreme.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] improve speed of non-verbose svn list
Posted by Osku Salerma <os...@iki.fi>.
On Tue, 25 Oct 2005, Garrett Rooney wrote:
> [...] and as a result the speed of the operation is greatly improved.
Quantifying "greatly" with actual numbers would be nice.
--
Osku Salerma - osku@iki.fi - http://www.iki.fi/osku/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] improve speed of non-verbose svn list
Posted by kf...@collab.net.
Garrett Rooney <ro...@electricjellyfish.net> writes:
> It's also been suggested that instead of sending the parameter over
> the wire as a bitfield it might make sense to send a list of words,
> since ra_svn doesn't currently send bitfields over the wire anywhere.
> I'm not sure how I feel about that.
I kind of agree. It makes the wire protocol a lot more debuggable,
for one thing...
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] improve speed of non-verbose svn list
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 25 Oct 2005 15:46:19 -0500, kfogel@collab.net <kf...@collab.net> wrote:
> > * subversion/libsvn_ra_dav/fetch.c
> > (svn_ra_dav__get_dir2): new version of get_dir, takes a dirent fields
> > arg, dynamically figure out which props we need to ask for to fill in
> > the dirent fields that were asked for. fall back to asking for all
> > of them in the has_props case, since we currently have no other way
> > to get that data. only fill in the parts of the dirent we were asked
> > to get. create and destroy a new subpool, which is used to allocate
> > the temporary neon properties array.
>
> Wow, this part reads like the function's doc string. Sure you want
> all that in the log message? :-)
Yeah, I'll see if some of that can be moved into someplace more useful...
> > + * @a dirent_fields controls which fields in the @c svn_dirent_t's are
> > + * filled in. To have them totally filled in use @c SVN_DIRENT_ALL,
> > + * otherwise simply binary or together the combination of @c SVN_DIRENT_
> > + * fields you care about.
> > + *
> > + * @since New in 1.4.
> > + */
>
> If you say "binary OR" it looks less like a typo. Actually, "bitwise OR"
> might be even better.
I agree, bitwise OR would be better.
> > +svn_ra_local__get_dir2 (svn_ra_session_t *session,
> > + const char *path,
> > + svn_revnum_t revision,
> > + apr_uint32_t dirent_fields,
> > + apr_hash_t **dirents,
> > + svn_revnum_t *fetched_rev,
> > + apr_hash_t **props,
> > + apr_pool_t *pool)
> > {
> > svn_fs_root_t *root;
> > svn_revnum_t youngest_rev;
>
> A "###" comment here indicating that dirent_fields is currently
> ignored might be a good idea.
Good call.
> I just realized that, formally, we don't need to use the deprecation
> conventions for all these double-underscore functions. But since
> they're mapping directly to public APIs, I guess following the same
> convention internally is the clearest thing to do, so, yay.
Actually, Peter Lundblad pointed out that a bunch of the get_dir2
stuff can go away, since these days we can rev the internal API
without a problem. My next rev of the patch avoids introducing a
get_dir2 method, and thus a bunch of that goes away entirely.
> > + SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "get-dir", "c(?r)bbn", path,
> > + rev, (props != NULL), (dirents != NULL),
> > + (apr_uint32_t) dirent_fields));
> > SVN_ERR(handle_auth_request(sess_baton, pool));
> > SVN_ERR(svn_ra_svn_read_cmd_response(conn, pool, "rll", &rev, &proplist,
> > &dirlist));
>
> You've extended the protocol to transmit dirent_fields as a number ("n").
>
> This is fine as long as both sides use apr_uint32_t, I guess. But the
> doc string for svn_ra_svn_write_cmd() says "n" means apr_uint64_t...
> Since dirent_fields is apr_uint32_t everywhere already, did you just
> mistype the cast?
That's a mistype of the cast. It should be apr_uint64_t.
> But stepping back for a moment: might it not be better just to make
> all 'dirent_fields' parameters apr_uint64_t, since that doubles the
> amount of room for future expansion? As a minor side benefit, that
> would allow us to remove the cast here entirely.
I don't feel strongly one way or the other on that. It seems unlikely
that svn_dirent_t will ever grow that large, but I suppose stranger
things have happened.
It's also been suggested that instead of sending the parameter over
the wire as a bitfield it might make sense to send a list of words,
since ra_svn doesn't currently send bitfields over the wire anywhere.
I'm not sure how I feel about that.
> > + assert (num_props == 0);
> > + }
>
> The assertion is a nice touch :-).
Had to make sure the code worked ;-)
> > + else
> > + {
> > + /* get all props, since we need them all to do has_props */
> > + which_props = NULL;
> > + }
> > +
>
> Why the subpool? There's no loop here, does it really save us anything?
> True, we allocate an array whose length is proportional to num_props,
> but num_props has a low maximum -- it's not like its magnitude is
> controlled by the client or anything.
In retrospect there's no real reason for it. I'll drop it in the next
version of the patch.
> Odd, why not just one conditional with an "||" ? <shrug> The shape
> of the code wasn't introduced by you, of course.
Yeah, I suppose that could be rewritten more cleanly.
> > + apr_uint64_t dirent_fields = SVN_DIRENT_ALL;
> >
> > - SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "c(?r)bb", &path, &rev,
> > - &want_props, &want_contents));
> > + SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "c(?r)bb?n", &path, &rev,
> > + &want_props, &want_contents, &dirent_fields));
>
> Good, here dirent_fields is an apr_uint64_t, which I guess it needs to
> be no matter how the parameter is casted back on the client side.
>
> Nice patch!
Thanks! There's still a problem with the kind field not getting
passed correctly down in ra_dav that I need to track down, but other
than that it seems to be working pretty well...
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] improve speed of non-verbose svn list
Posted by kf...@collab.net.
Garrett Rooney <ro...@electricjellyfish.net> writes:
> [[[
> Speed up non-verbose 'svn list' by a scary amount by allowing the server
> to avoid calculating parts of the svn_dirent_t objects that are never used.
>
> * subversion/libsvn_ra/ra_loader.c
> (svn_ra_get_dir2): new wrapper function.
>
> * subversion/libsvn_ra/ra_loader.h
> (svn_ra__vtable_t): add get_dir2 function.
>
> * subversion/include/svn_types.h
> (SVN_DIRENT_KIND,
> SVN_DIRENT_SIZE,
> SVN_DIRENT_HAS_PROPS,
> SVN_DIRENT_CREATED_REV,
> SVN_DIRENT_TIME,
> SVN_DIRENT_LAST_AUTHOR): new bitfield values for each field in a dirent.
> (SVN_DIRENT_ALL): combination of all the other bitfield values.
>
> * subversion/include/svn_client.h
> (svn_client_ls4): new function, like svn_client_ls3, but with a dirent
> fields parameter.
> (svn_client_ls3): deprecated in favor of svn_client_ls4.
>
> * subversion/include/svn_ra.h
> (svn_ra_get_dir2): new version of svn_ra_get_dir, takes a dirent fields
> parameter.
> (svn_ra_get_dir): deprecate in favor of svn_ra_get_dir2.
>
> * subversion/libsvn_ra_local/ra_plugin.c
> (svn_ra_local__get_dir2): new function, currently ignores the dirent
> fields parameter.
> (svn_ra_local__get_dir): call into svn_ra_local__get_dir, passing it
> SVN_DIRENT_ALL for the dirent_fields parameter.
> (ra_local_vtable): add svn_ra_Local__get_dir2.
>
> * subversion/libsvn_client/ls.c
> (get_dir_contents): add a dirent_fields parameter, use svn_ra_get_dir2,
> update recursive call.
> (svn_client_ls4): new list function, adds a dirent_fields parameter which
> is passed to get_dir_contents.
> (svn_client_ls3): backwards compat wrapper, calls svn_client_ls4 with the
> dirent_fields set to SVN_DIRENT_ALL.
>
> * subversion/clients/cmdline/ls-cmd.c
> (svn_cl__ls): use svn_client_ls4, only asking for the dirent fields we
> actually need.
>
> * subversion/libsvn_ra_svn/client.c
> (ra_svn_get_dir2): new version of get_dir, pass new dirent_fields
> parameter along to the server.
> (ra_svn_get_dir): pass through to new function, passing SVN_DIRENT_ALL
> for dirent_fields.
> (ra_svn_vtable): add ra_svn_get_dir2.
>
> * subversion/libsvn_ra_dav/ra_dav.h
> (svn_ra_dav__get_dir2): new prototype.
>
> * subversion/libsvn_ra_dav/session.c
> (dav_vtable): add svn_ra_dav__get_dir2.
>
> * subversion/libsvn_ra_dav/fetch.c
> (svn_ra_dav__get_dir2): new version of get_dir, takes a dirent fields
> arg, dynamically figure out which props we need to ask for to fill in
> the dirent fields that were asked for. fall back to asking for all
> of them in the has_props case, since we currently have no other way
> to get that data. only fill in the parts of the dirent we were asked
> to get. create and destroy a new subpool, which is used to allocate
> the temporary neon properties array.
Wow, this part reads like the function's doc string. Sure you want
all that in the log message? :-)
(By the way, my personal nit: If the first letters of sentences are
capitalized, it's a lot easier for me to read. I suspect this is true
of others as well, but don't have any evidence to back that up.)
Okay, on to more substantive comments...
> (svn_ra_dav__get_dir): compat wrapper, passes SVN_DIRENT_ALL as the
> dirent_flags arg to svn_ra_dav__get_dir2.
>
> * subversion/svnserve/serve.c
> (get_dir): accept an optional dirent fields parameter from the client,
> defaulting to SVN_DIRENT_ALL if it's not present. only initialize the
> parts of the dirents that the client asked for.
> ]]]
>
> [...]
>
> --- subversion/include/svn_client.h (revision 17006)
> +++ subversion/include/svn_client.h (working copy)
> @@ -2146,7 +2146,31 @@
> * If @a recurse is true (and @a path_or_url is a directory) this will
> * be a recursive operation.
> *
> + * @a dirent_fields controls which fields in the @c svn_dirent_t's are
> + * filled in. To have them totally filled in use @c SVN_DIRENT_ALL,
> + * otherwise simply binary or together the combination of @c SVN_DIRENT_
> + * fields you care about.
> + *
> + * @since New in 1.4.
> + */
If you say "binary OR" it looks less like a typo. Actually, "bitwise OR"
might be even better.
> --- subversion/libsvn_ra_local/ra_plugin.c (revision 17006)
> +++ subversion/libsvn_ra_local/ra_plugin.c (working copy)
> @@ -947,13 +947,14 @@
>
> /* Getting a directory's entries */
> static svn_error_t *
> -svn_ra_local__get_dir (svn_ra_session_t *session,
> - const char *path,
> - svn_revnum_t revision,
> - apr_hash_t **dirents,
> - svn_revnum_t *fetched_rev,
> - apr_hash_t **props,
> - apr_pool_t *pool)
> +svn_ra_local__get_dir2 (svn_ra_session_t *session,
> + const char *path,
> + svn_revnum_t revision,
> + apr_uint32_t dirent_fields,
> + apr_hash_t **dirents,
> + svn_revnum_t *fetched_rev,
> + apr_hash_t **props,
> + apr_pool_t *pool)
> {
> svn_fs_root_t *root;
> svn_revnum_t youngest_rev;
A "###" comment here indicating that dirent_fields is currently
ignored might be a good idea.
I just realized that, formally, we don't need to use the deprecation
conventions for all these double-underscore functions. But since
they're mapping directly to public APIs, I guess following the same
convention internally is the clearest thing to do, so, yay.
> --- subversion/libsvn_ra_svn/client.c (revision 17006)
> +++ subversion/libsvn_ra_svn/client.c (working copy)
> @@ -1018,11 +1018,13 @@
> return SVN_NO_ERROR;
> }
>
> -static svn_error_t *ra_svn_get_dir(svn_ra_session_t *session, const char *path,
> - svn_revnum_t rev, apr_hash_t **dirents,
> - svn_revnum_t *fetched_rev,
> - apr_hash_t **props,
> - apr_pool_t *pool)
> +static svn_error_t *ra_svn_get_dir2(svn_ra_session_t *session,
> + const char *path, svn_revnum_t rev,
> + apr_uint32_t dirent_fields,
> + apr_hash_t **dirents,
> + svn_revnum_t *fetched_rev,
> + apr_hash_t **props,
> + apr_pool_t *pool)
> {
> ra_svn_session_baton_t *sess_baton = session->priv;
> svn_ra_svn_conn_t *conn = sess_baton->conn;
> @@ -1035,8 +1037,9 @@
> apr_uint64_t size;
> svn_dirent_t *dirent;
>
> - SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "get-dir", "c(?r)bb", path,
> - rev, (props != NULL), (dirents != NULL)));
> + SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "get-dir", "c(?r)bbn", path,
> + rev, (props != NULL), (dirents != NULL),
> + (apr_uint32_t) dirent_fields));
> SVN_ERR(handle_auth_request(sess_baton, pool));
> SVN_ERR(svn_ra_svn_read_cmd_response(conn, pool, "rll", &rev, &proplist,
> &dirlist));
You've extended the protocol to transmit dirent_fields as a number ("n").
This is fine as long as both sides use apr_uint32_t, I guess. But the
doc string for svn_ra_svn_write_cmd() says "n" means apr_uint64_t...
Since dirent_fields is apr_uint32_t everywhere already, did you just
mistype the cast?
But stepping back for a moment: might it not be better just to make
all 'dirent_fields' parameters apr_uint64_t, since that doubles the
amount of room for future expansion? As a minor side benefit, that
would allow us to remove the cast here entirely.
> --- subversion/libsvn_ra_dav/fetch.c (revision 17006)
> +++ subversion/libsvn_ra_dav/fetch.c (working copy)
> @@ -936,6 +939,7 @@
> apr_size_t final_url_n_components;
> svn_ra_dav__session_t *ras = session->priv;
> const char *url = svn_path_url_add_component (ras->url->data, path, pool);
> + apr_pool_t *subpool = svn_pool_create (pool);
>
> /* If the revision is invalid (head), then we're done. Just fetch
> the public URL, because that will always get HEAD. */
> @@ -963,11 +967,82 @@
>
> if (dirents)
> {
> + ne_propname *which_props;
> +
> + /* if we didn't ask for the has_props field, we can get individual
> + properties. */
> + if ((SVN_DIRENT_HAS_PROPS & dirent_fields) == 0)
> + {
> + apr_size_t num_props = 1; /* start with one for the final NULL */
> +
> + if (dirent_fields & SVN_DIRENT_KIND)
> + ++num_props;
> +
> + if (dirent_fields & SVN_DIRENT_SIZE)
> + ++num_props;
> +
> + if (dirent_fields & SVN_DIRENT_CREATED_REV)
> + ++num_props;
> +
> + if (dirent_fields & SVN_DIRENT_TIME)
> + ++num_props;
> +
> + if (dirent_fields & SVN_DIRENT_LAST_AUTHOR)
> + ++num_props;
> +
> + which_props = apr_pcalloc (subpool,
> + num_props * sizeof (ne_propname));
> +
> + /* first, null out the end... */
> + which_props[num_props].nspace = NULL;
> + which_props[num_props--].name = NULL;
> +
> + /* Now, go through and fill in the ones we care about, moving along
> + the array as we go. */
> +
> + if (dirent_fields & SVN_DIRENT_KIND)
> + {
> + which_props[num_props].nspace = "DAV:";
> + which_props[num_props--].name = "resourcetype";
> + }
> +
> + if (dirent_fields & SVN_DIRENT_SIZE)
> + {
> + which_props[num_props].nspace = "DAV:";
> + which_props[num_props--].name = "getcontentlength";
> + }
> +
> + if (dirent_fields & SVN_DIRENT_CREATED_REV)
> + {
> + which_props[num_props].nspace = "DAV:";
> + which_props[num_props--].name = "version-name";
> + }
> +
> + if (dirent_fields & SVN_DIRENT_TIME)
> + {
> + which_props[num_props].nspace = "DAV:";
> + which_props[num_props--].name = "creationdate";
> + }
> +
> + if (dirent_fields & SVN_DIRENT_LAST_AUTHOR)
> + {
> + which_props[num_props].nspace = "DAV:";
> + which_props[num_props--].name = "creator-displayname";
> + }
> +
> + assert (num_props == 0);
> + }
The assertion is a nice touch :-).
> + else
> + {
> + /* get all props, since we need them all to do has_props */
> + which_props = NULL;
> + }
> +
Why the subpool? There's no loop here, does it really save us anything?
True, we allocate an array whose length is proportional to num_props,
but num_props has a low maximum -- it's not like its magnitude is
controlled by the client or anything.
> /* Just like Nautilus, Cadaver, or any other browser, we do a
> PROPFIND on the directory of depth 1. */
> SVN_ERR( svn_ra_dav__get_props(&resources, ras->sess,
> final_url, NE_DEPTH_ONE,
> - NULL, NULL /* all props */, pool) );
> + NULL, which_props, pool) );
>
> /* Count the number of path components in final_url. */
> final_url_n_components = svn_path_component_count(final_url);
> @@ -1000,57 +1075,76 @@
> continue;
>
> entry = apr_pcalloc (pool, sizeof(*entry));
> -
> - /* node kind */
> - entry->kind = resource->is_collection ? svn_node_dir : svn_node_file;
> -
> - /* size */
> - propval = apr_hash_get(resource->propset,
> - SVN_RA_DAV__PROP_GETCONTENTLENGTH,
> - APR_HASH_KEY_STRING);
> - if (propval == NULL)
> - entry->size = 0;
> - else
> - entry->size = svn__atoui64(propval->data);
> -
> - /* does this resource contain any 'svn' or 'custom' properties,
> - i.e. ones actually created and set by the user? */
> - for (h = apr_hash_first (pool, resource->propset);
> - h; h = apr_hash_next (h))
> +
> + if (dirent_fields & SVN_DIRENT_KIND)
> {
> - const void *kkey;
> - void *vval;
> - apr_hash_this (h, &kkey, NULL, &vval);
> + /* node kind */
> + entry->kind = resource->is_collection ? svn_node_dir
> + : svn_node_file;
> + }
> +
> + if (dirent_fields & SVN_DIRENT_SIZE)
> + {
> + /* size */
> + propval = apr_hash_get(resource->propset,
> + SVN_RA_DAV__PROP_GETCONTENTLENGTH,
> + APR_HASH_KEY_STRING);
> + if (propval == NULL)
> + entry->size = 0;
> + else
> + entry->size = svn__atoui64(propval->data);
> + }
> +
> + if (dirent_fields & SVN_DIRENT_HAS_PROPS)
> + {
> + /* does this resource contain any 'svn' or 'custom' properties,
> + i.e. ones actually created and set by the user? */
> + for (h = apr_hash_first (pool, resource->propset);
> + h; h = apr_hash_next (h))
> + {
> + const void *kkey;
> + void *vval;
> + apr_hash_this (h, &kkey, NULL, &vval);
>
> - if (strncmp((const char *)kkey, SVN_DAV_PROP_NS_CUSTOM,
> - sizeof(SVN_DAV_PROP_NS_CUSTOM) - 1) == 0)
> - entry->has_props = TRUE;
> + if (strncmp((const char *)kkey, SVN_DAV_PROP_NS_CUSTOM,
> + sizeof(SVN_DAV_PROP_NS_CUSTOM) - 1) == 0)
> + entry->has_props = TRUE;
>
> - else if (strncmp((const char *)kkey, SVN_DAV_PROP_NS_SVN,
> - sizeof(SVN_DAV_PROP_NS_SVN) - 1) == 0)
> - entry->has_props = TRUE;
> + else if (strncmp((const char *)kkey, SVN_DAV_PROP_NS_SVN,
> + sizeof(SVN_DAV_PROP_NS_SVN) - 1) == 0)
> + entry->has_props = TRUE;
> + }
> + }
Odd, why not just one conditional with an "||" ? <shrug> The shape
of the code wasn't introduced by you, of course.
> --- subversion/svnserve/serve.c (revision 17006)
> +++ subversion/svnserve/serve.c (working copy)
> @@ -1062,9 +1062,10 @@
> svn_fs_root_t *root;
> apr_pool_t *subpool;
> svn_boolean_t want_props, want_contents;
> + apr_uint64_t dirent_fields = SVN_DIRENT_ALL;
>
> - SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "c(?r)bb", &path, &rev,
> - &want_props, &want_contents));
> + SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "c(?r)bb?n", &path, &rev,
> + &want_props, &want_contents, &dirent_fields));
Good, here dirent_fields is an apr_uint64_t, which I guess it needs to
be no matter how the parameter is casted back on the client side.
Nice patch!
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org