You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Jean-Marc Godbout <jm...@gmail.com> on 2005/08/29 01:18:28 UTC
[PATCH] Fix for issue 2151 "'svn ls' is slow over ra_dav"
[[[
Fix for issue 2151 "'svn ls' is slow over ra_dav"
This patch implements a solution to issue 2151. We now only request
the needed props in the PROPFIND for server listings. 'svn ls' is now
noticably faster. In most cases 'ls' takes about half the time and
half the bandwidth - In some case even better results.
For backward compatibility, before doing the long PROPFIND we make
another simpler PROPFIND to see if the server supports the new type of
request (supports the deadprop-count prop). If it does we use the new
scheme and performance is improved - If not then we use the old scheme
and the slowness persists.
To summarise: only patched servers and patched clients have improved
speed. Mismatched configurations (old client or server) are not
improved but still work. Regular dav
clients are still slow.
Patch by: Jean-Marc Godbout <je...@computrad.com>
Suggested by: Erik Scrafford <er...@scrafford.org>
Ben Collins-Sussman <su...@collab.net>
Everyone else who contributed to issue 2151
* subversion/mod_dav_svn/liveprops.c
(SVN_PROPID_deadprop_count): Added a PROPID to support the new prop
(dav_svn_props[]): Added the deadprop-count prop to the list of
supported props.
(case SVN_PROPID_deadprop_count): Returns the number of deadprops
found in the fs for that file.
* subversion/libsvn_ra_dav/ra_dav.h
(SVN_RA_DAV__PROP_DEADPROP_COUNT): Added the prop name for deadprop
(ELEM_deadprop_count): Added the prop to the list of props
* subversion/libsvn_ra_dav/props.c
(elem_definition, propfind_elements): Added ELEM_deadprop_count
to the list of propfind props
* subversion/libsvn_ra_dav/fetch.c
(dirent_props[]): list of props to get in a dirent PROPFIND
(root_props[]): list of props to get in a root PROPFIND
(supports_deadprop_count): indicates if the server supports
the deadprop_count prop
(prop_count): the number of user props for the current dirent
as returned by the server
]]]
Re: [PATCH] Fix for issue 2151 "'svn ls' is slow over ra_dav"
Posted by Ben Collins-Sussman <su...@collab.net>.
Thanks, this patch looks pretty good!
This is a review from me and kfogel.
The review includes both formatting problems, minor nits, and also
some substantive comments on the changes. We look forward to the next
iteration of the patch. :-)
> [[[
> Fix for issue 2151 "'svn ls' is slow over ra_dav"
>
> This patch implements a solution to issue 2151. We now only request
> the needed props in the PROPFIND for server listings. 'svn ls' is now
> noticably faster. In most cases 'ls' takes about half the time and
> half the bandwidth - In some case even better results.
>
> For backward compatibility, before doing the long PROPFIND we make
> another simpler PROPFIND to see if the server supports the new
type of
> request (supports the deadprop-count prop). If it does we use the new
> scheme and performance is improved - If not then we use the old
scheme
> and the slowness persists.
>
Here's another strategy that we've been pondering:
We see that the new 'deadprop-count' property is already in your short
list of properties to fetch. Instead of unconditionally doing an
extra PROPFIND for the new 'deadprop-count' property, why not just
*always* request the short list in the depth-1 PROPFIND? If the new
property doesn't come back, then repeat the depth-1 PROPFIND, asking
for <allprops>.
The problem with this is that 'svn ls' will now be about twice as slow
against *old* servers -- doing two depth-1 PROPFINDs instead of one.
So we wouldn't just be rewarding those who upgrade servers with 2x
speed, we'd be actively *punishing* people who don't upgrade their
servers with 1/2 speed. This may cause riots.
So we're thinking that maybe we should stick with your strategy for
now, but say, two releases from now, go with our strategy. By then,
most servers will have upgraded, and it'll be silly for clients to be
wasting time on an extra unconditional turnaround.
Does that sound like a good idea?
> To summarise: only patched servers and patched clients have improved
> speed. Mismatched configurations (old client or server) are not
> improved but still work. Regular dav
> clients are still slow.
>
> Patch by: Jean-Marc Godbout <je...@computrad.com>
>
> Suggested by: Erik Scrafford <er...@scrafford.org>
> Ben Collins-Sussman <su...@collab.net>
> Everyone else who contributed to issue 2151
>
>
> * subversion/mod_dav_svn/liveprops.c
> (SVN_PROPID_deadprop_count): Added a PROPID to support the new prop
> (dav_svn_props[]): Added the deadprop-count prop to the list of
> supported props.
> (case SVN_PROPID_deadprop_count): Returns the number of deadprops
> found in the fs for that file.
>
The name of the affected function goes in the parentheses: the 'case'
shouldn't be in there, but rather "(dav_svn_insert_prop)".
> * subversion/libsvn_ra_dav/ra_dav.h
> (SVN_RA_DAV__PROP_DEADPROP_COUNT): Added the prop name for deadprop
> (ELEM_deadprop_count): Added the prop to the list of props
>
> * subversion/libsvn_ra_dav/props.c
> (elem_definition, propfind_elements): Added ELEM_deadprop_count
> to the list of propfind props
>
> * subversion/libsvn_ra_dav/fetch.c
> (dirent_props[]): list of props to get in a dirent PROPFIND
> (root_props[]): list of props to get in a root PROPFIND
Just a nit: when we add a new top-level symbol to the codebase, our
general convention is to indicate that it's new, e.g.
(dirent_props[]): New array, a list of props to get in a dirent
PROPFIND.
(root_props[]): New array, a list of props to get in a root PROPFIND.
> (supports_deadprop_count): indicates if the server supports
> the deadprop_count prop
> (prop_count): the number of user props for the current dirent
> as returned by the server
> ]]]
Non-top-level symbols don't get listed in parens. Instead, mention
the function in which these variables were added, and describe the
overall change to the function, e.g.
(svn_ra_dav__get_dir): check if server supports 'deadprop-count',
and if so, request fewer properties.
>
>
>
> Index: subversion/mod_dav_svn/liveprops.c
> ===================================================================
> --- subversion/mod_dav_svn/liveprops.c (revision 15946)
> +++ subversion/mod_dav_svn/liveprops.c (working copy)
> @@ -67,7 +67,8 @@
> enum {
> SVN_PROPID_baseline_relative_path = 1,
> SVN_PROPID_md5_checksum,
> - SVN_PROPID_repository_uuid
> + SVN_PROPID_repository_uuid,
> + SVN_PROPID_deadprop_count
> };
>
> static const dav_liveprop_spec dav_svn_props[] =
> @@ -96,6 +97,7 @@
> SVN_RO_SVN_PROP(baseline_relative_path, baseline-relative-path),
> SVN_RO_SVN_PROP(md5_checksum, md5-checksum),
> SVN_RO_SVN_PROP(repository_uuid, repository-uuid),
> + SVN_RO_SVN_PROP(deadprop_count, deadprop-count),
>
> { 0 } /* sentinel */
> };
> @@ -563,6 +565,30 @@
> }
> break;
>
> + case SVN_PROPID_deadprop_count:
> + {
> + if (resource->type != DAV_RESOURCE_TYPE_REGULAR)
> + return DAV_PROP_INSERT_NOTSUPP;
> +
> + unsigned int propcount;
> +
> + apr_hash_t *proplist;
We write to C89 standards (not C99), which means you can't declare
variables right in the middle of a block like that. It needs to be
declared at the beginning of a block.
> + serr = svn_fs_node_proplist(&proplist,
> + resource->info->root.root,
> + resource->info->repos_path, p);
> + if (serr != NULL)
> + {
> + /* ### what to do? */
> + svn_error_clear(serr);
> + value = "###error###";
> + break;
> + }
> +
> + propcount = apr_hash_count(proplist);
> + value = apr_psprintf(p, "%u", propcount);
> + break;
> + }
> +
> default:
> /* ### what the heck was this property? */
> return DAV_PROP_INSERT_NOTDEF;
> Index: subversion/libsvn_ra_dav/ra_dav.h
> ===================================================================
> --- subversion/libsvn_ra_dav/ra_dav.h (revision 15946)
> +++ subversion/libsvn_ra_dav/ra_dav.h (working copy)
> @@ -368,6 +368,7 @@
>
> #define SVN_RA_DAV__PROP_REPOSITORY_UUID SVN_DAV_PROP_NS_DAV
"repository-uuid"
>
> +#define SVN_RA_DAV__PROP_DEADPROP_COUNT "deadprop-count"
Our liveprops all live in the SVN_DAV_PROP_NS_DAV namespace; see the
repository-uuid definition as an example. Just add that constant to
your declaration.
>
> typedef struct {
> /* what is the URL for this resource */
> @@ -707,7 +708,8 @@
> ELEM_lock_owner,
> ELEM_lock_comment,
> ELEM_lock_creationdate,
> - ELEM_lock_expirationdate
> + ELEM_lock_expirationdate,
> + ELEM_deadprop_count,
> };
In C89, you can't put a comma after the last element in a list. If
only this were python. :-)
>
> /* ### docco */
> Index: subversion/libsvn_ra_dav/props.c
> ===================================================================
> --- subversion/libsvn_ra_dav/props.c (revision 15946)
> +++ subversion/libsvn_ra_dav/props.c (working copy)
> @@ -111,6 +111,7 @@
> { ELEM_baseline_relpath, SVN_RA_DAV__PROP_BASELINE_RELPATH, 1 },
> { ELEM_md5_checksum, SVN_RA_DAV__PROP_MD5_CHECKSUM, 1 },
> { ELEM_repository_uuid, SVN_RA_DAV__PROP_REPOSITORY_UUID, 1 },
> + { ELEM_deadprop_count, SVN_RA_DAV__PROP_DEADPROP_COUNT, 1 },
> { 0 }
> };
>
> @@ -147,6 +148,8 @@
> SVN_RA_DAV__XML_CDATA },
> { SVN_DAV_PROP_NS_DAV, "repository-uuid", ELEM_repository_uuid,
> SVN_RA_DAV__XML_CDATA },
> + { SVN_DAV_PROP_NS_DAV, "deadprop-count", ELEM_deadprop_count,
> + SVN_RA_DAV__XML_CDATA },
>
> /* Unknowns */
> { "", "", ELEM_unknown, SVN_RA_DAV__XML_COLLECT },
> Index: subversion/libsvn_ra_dav/fetch.c
> ===================================================================
> --- subversion/libsvn_ra_dav/fetch.c (revision 15946)
> +++ subversion/libsvn_ra_dav/fetch.c (working copy)
> @@ -914,7 +914,25 @@
> return SVN_NO_ERROR;
> }
>
> +/* the properties we need to fill in an svn_dirent_t, used by
> + svn_ra_get_dir(). */
> +static const ne_propname dirent_props[] =
> +{
> + { "DAV:", "resourcetype" }, /* kind */
> + { "DAV:", "getcontentlength" }, /* size */
> + { "DAV:", "version-name" }, /* created_rev */
> + { "DAV:", "creationdate" }, /* time */
> + { "DAV:", "creator-displayname" }, /* last_author */
> + { SVN_DAV_PROP_NS_DAV, "deadprop-count" }, /* has_props */
> + { NULL }
> +};
Just for readability, can you make the comments line up?
>
> +static const ne_propname root_props[] =
> +{
> + { SVN_DAV_PROP_NS_DAV, "deadprop-count" },
> + { NULL }
> +};
> +
Can we have a different name for this array? We're not sure what
'root' has to do with anything...
Also, can you add a docstring before the array, just like
dirent_props[] has?
(Also, assuming you agree with our long-term strategy, this array will
eventually go away, since we won't be doing the extra PROPFIND.)
> svn_error_t *svn_ra_dav__get_dir(svn_ra_session_t *session,
> const char *path,
> svn_revnum_t revision,
> @@ -955,13 +973,46 @@
> *fetched_rev = got_rev;
> }
>
> + /* For issue 2151:
> + See if we are dealing with a server that understand the
> + deadprop-count prop. If it doesn't we'll need to do an
> + allprop PROPFIND - If it does, we'll execute a more
> + targetted PROPFIND.
> + */
> + svn_boolean_t supports_deadprop_count;
> +
Again, new variables need to be declared at the beginning of a block.
> + {
> + const svn_string_t *deadprop_count;
> +
> + SVN_ERR( svn_ra_dav__get_props_resource(&rsrc, ras->sess,
> + final_url, NULL,
root_props,
> + pool) );
> +
> + deadprop_count = apr_hash_get(rsrc->propset, "deadprop-
count", APR_HASH_KEY_STRING);
Oops, this line is more than 80 chars. Please fix.
> +
> + if (deadprop_count == NULL)
> + supports_deadprop_count = FALSE;
> + else
> + supports_deadprop_count = TRUE;
> + }
> +
Someday this whole block will go away, when we stop doing the extra
PROPFIND.
> if (dirents)
> {
> /* 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) );
> + if (supports_deadprop_count == TRUE)
> + {
> + SVN_ERR( svn_ra_dav__get_props(&resources, ras->sess,
> + final_url, NE_DEPTH_ONE,
> + NULL, dirent_props,
pool) );
> + }
> + else /* The server doesn't support the deadprop_count prop,
fallback */
> + {
> + SVN_ERR( svn_ra_dav__get_props(&resources, ras->sess,
> + final_url, NE_DEPTH_ONE,
> + NULL, NULL /* allprops */,
> + pool) );
> + }
>
> /* Count the number of path components in final_url. */
> final_url_n_components = svn_path_component_count(final_url);
> @@ -981,6 +1032,7 @@
> const svn_string_t *propval;
> apr_hash_index_t *h;
> svn_dirent_t *entry;
> + apr_int64_t prop_count;
>
> apr_hash_this (hi, &key, NULL, &val);
> childname = key;
> @@ -1009,20 +1061,41 @@
>
> /* 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 (supports_deadprop_count == TRUE)
> {
> - const void *kkey;
> - void *vval;
> - apr_hash_this (h, &kkey, NULL, &vval);
> + propval = apr_hash_get(resource->propset,
> +
SVN_RA_DAV__PROP_DEADPROP_COUNT,
> + APR_HASH_KEY_STRING);
> +
> + if (propval == NULL)
> + entry->has_props = FALSE;
If (supports_deadprop_count == TRUE) and there's no property
value... that's an error, isn't it? It means the server has sent
bogus data, no? Shouldn't we treat this as an error condition, rather
than just setting has_props = FALSE?
> + else
> + {
> + prop_count = svn__atoui64(propval->data);
>
> - 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;
> + if (prop_count == 0)
> + entry->has_props = FALSE;
> + else
> + entry->has_props = TRUE;
> + }
> + }
> + else /* The server doesn't support the deadprop_count
prop, fallback */
> + {
> + for (h = apr_hash_first (pool, resource->propset);
Whoops! You accidentally inserted a tab character in the line above.
Make sure your editor isn't doing that. We use only spaces in our
source code.
> + h; h = apr_hash_next (h))
> + {
> + const void *kkey;
> + void *vval;
> + apr_hash_this (h, &kkey, NULL, &vval);
> +
We know this isn't your code here, but we just noticed that 'vval' is
never used. Can you clean it up by just removing that variable and
passing NULL as the last argument to apr_hash_this() ?
> + 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;
> + }
> }
>
> /* created_rev & friends */
--
www.collab.net <> CollabNet | Distributed Development On Demand
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix for issue 2151 "'svn ls' is slow over ra_dav"
Posted by Julian Reschke <ju...@gmx.de>.
Ben Collins-Sussman wrote:
> ...
>> Another change that would require an upgrade would be to make the
>> server be more DeltaV compliant by simply not sending any DeltaV
>> props on an allprop request (as described in issue #2151). This would
>> mean that all older clients couldn't do "ls"s against patched servers...
>
>
> You keep saying this, but why do you believe it to be true? If the
> server stops sending DeltaV props in response to <allprop> requests,
> which svn client commands break? And why?
> ...
Assuming this is true, this really should be fixed in the client now, so
that the server can be made compliant in a later release...
Best regards, Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix for issue 2151 "'svn ls' is slow over ra_dav"
Posted by Ben Collins-Sussman <su...@collab.net>.
On Sep 2, 2005, at 1:20 AM, Jean-Marc Godbout wrote:
>
> Well thanks for creating doubt in my mind. I was certain that on
> the unpatched "ls"used to complain when it couldn't find the
> "checked-in" property. But I might be wrong, i might have done
> something wrong here. I'll verify. And yes, if there are problems
> then they should be fixed asap so that eventually this behavior
> could be phased out. I'll get back to you on that.
>
I can't imagine why svn_ra_get_dir() would care about the DAV:checked-
in property; it's not necessary at all to fill in the svn_dirent_t.
--
www.collab.net <> CollabNet | Distributed Development On Demand
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix for issue 2151 "'svn ls' is slow over ra_dav"
Posted by Jean-Marc Godbout <jm...@gmail.com>.
Yes, having read the code, it doesn't make sense to me either. So I guess I
should try it and be certain.
On 9/2/05, Ben Collins-Sussman <su...@collab.net> wrote:
>
>
> On Sep 2, 2005, at 1:20 AM, Jean-Marc Godbout wrote:
> >
> > Well thanks for creating doubt in my mind. I was certain that on
> > the unpatched "ls"used to complain when it couldn't find the
> > "checked-in" property. But I might be wrong, i might have done
> > something wrong here. I'll verify. And yes, if there are problems
> > then they should be fixed asap so that eventually this behavior
> > could be phased out. I'll get back to you on that.
> >
>
> I can't imagine why svn_ra_get_dir() would care about the DAV:checked-
> in property; it's not necessary at all to fill in the svn_dirent_t.
>
>
>
>
> --
> www.collab.net <http://www.collab.net> <> CollabNet | Distributed
> Development On Demand
>
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
>
Re: [PATCH] Fix for issue 2151 "'svn ls' is slow over ra_dav"
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Fri, 9 Sep 2005, Jean-Marc Godbout wrote:
> I just wanted to check up on the status of this patch. Is there anything I
> need to change or additional comments about the patch?
>
I'm not that much into DAV, so I'm not going to review this, so I can't
speka about the quality of this patch. Just checking: did you attach this
patch to the issue? If not, it's a good idea (linking to the mail is
good).
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix for issue 2151 "'svn ls' is slow over ra_dav"
Posted by Jean-Marc Godbout <jm...@gmail.com>.
Wow, that's great news for Ben, congrats! And the patch can wait. The issue
it covers was scheduled as 1.4-consider anyways.
Good luck guys!
On 9/16/05, kfogel@collab.net <kf...@collab.net> wrote:
>
> Jean-Marc Godbout <jm...@gmail.com> writes:
> > I just want to know if there is anything else I need to do to the patch
> in
> > this thread (http://svn.haxx.se/dev/archive-2005-09/0009.shtml). I'm
> just
> > wondering as there hasn't been any activity on this for the past week.
>
> Some of us who are qualified to review it have just been a bit busy,
> http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=105356
> explains why :-). Sorry for the delay, someone will get to it as soon
> as possible.
>
> Best,
> -Karl
>
> > On 9/13/05, Jean-Marc Godbout <jm...@gmail.com> wrote:
> > >
> > > Done, thanks Peter.
> > >
> > > On 9/10/05, Peter N. Lundblad <pe...@famlundblad.se> wrote:
> > > >
> > > > On Fri, 9 Sep 2005, Jean-Marc Godbout wrote:
> > > >
> > > > > I just wanted to check up on the status of this patch. Is there
> > > > anything I
> > > > > need to change or additional comments about the patch?
> > > > >
> > > > I'm not that much into DAV, so I'm not going to review this, so I
> can't
> > > > speka about the quality of this patch. Just checking: did you attach
> > > > this
> > > > patch to the issue? If not, it's a good idea (linking to the mail is
> > > > good).
> > > >
> > > > Thanks,
> > > > //Peter
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
>
Re: [PATCH] Fix for issue 2151 "'svn ls' is slow over ra_dav"
Posted by Jean-Marc Godbout <jm...@gmail.com>.
Done, thanks Peter.
On 9/10/05, Peter N. Lundblad <pe...@famlundblad.se> wrote:
>
> On Fri, 9 Sep 2005, Jean-Marc Godbout wrote:
>
> > I just wanted to check up on the status of this patch. Is there anything
> I
> > need to change or additional comments about the patch?
> >
> I'm not that much into DAV, so I'm not going to review this, so I can't
> speka about the quality of this patch. Just checking: did you attach this
> patch to the issue? If not, it's a good idea (linking to the mail is
> good).
>
> Thanks,
> //Peter
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
>
Re: [PATCH] Fix for issue 2151 "'svn ls' is slow over ra_dav"
Posted by kf...@collab.net.
Jean-Marc Godbout <jm...@gmail.com> writes:
> I just want to know if there is anything else I need to do to the patch in
> this thread (http://svn.haxx.se/dev/archive-2005-09/0009.shtml). I'm just
> wondering as there hasn't been any activity on this for the past week.
Some of us who are qualified to review it have just been a bit busy,
http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=105356
explains why :-). Sorry for the delay, someone will get to it as soon
as possible.
Best,
-Karl
> On 9/13/05, Jean-Marc Godbout <jm...@gmail.com> wrote:
> >
> > Done, thanks Peter.
> >
> > On 9/10/05, Peter N. Lundblad <pe...@famlundblad.se> wrote:
> > >
> > > On Fri, 9 Sep 2005, Jean-Marc Godbout wrote:
> > >
> > > > I just wanted to check up on the status of this patch. Is there
> > > anything I
> > > > need to change or additional comments about the patch?
> > > >
> > > I'm not that much into DAV, so I'm not going to review this, so I can't
> > > speka about the quality of this patch. Just checking: did you attach
> > > this
> > > patch to the issue? If not, it's a good idea (linking to the mail is
> > > good).
> > >
> > > Thanks,
> > > //Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix for issue 2151 "'svn ls' is slow over ra_dav"
Posted by Jean-Marc Godbout <jm...@gmail.com>.
Hi List,
I just want to know if there is anything else I need to do to the patch in
this thread (http://svn.haxx.se/dev/archive-2005-09/0009.shtml). I'm just
wondering as there hasn't been any activity on this for the past week.
Thanks,
Jean-Marc
On 9/13/05, Jean-Marc Godbout <jm...@gmail.com> wrote:
>
> Done, thanks Peter.
>
> On 9/10/05, Peter N. Lundblad <pe...@famlundblad.se> wrote:
> >
> > On Fri, 9 Sep 2005, Jean-Marc Godbout wrote:
> >
> > > I just wanted to check up on the status of this patch. Is there
> > anything I
> > > need to change or additional comments about the patch?
> > >
> > I'm not that much into DAV, so I'm not going to review this, so I can't
> > speka about the quality of this patch. Just checking: did you attach
> > this
> > patch to the issue? If not, it's a good idea (linking to the mail is
> > good).
> >
> > Thanks,
> > //Peter
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> > For additional commands, e-mail: dev-help@subversion.tigris.org
> >
> >
> >
>
Re: [PATCH] Fix for issue 2151 "'svn ls' is slow over ra_dav"
Posted by Jean-Marc Godbout <jm...@gmail.com>.
Hello list,
I just wanted to check up on the status of this patch. Is there anything I
need to change or additional comments about the patch?
Thanks,
Jean-Marc
On 9/2/05, Jean-Marc Godbout <jm...@gmail.com> wrote:
>
> Yes, having read the code, it doesn't make sense to me either. So I guess
> I should try it and be certain.
>
> On 9/2/05, Ben Collins-Sussman < sussman@collab.net> wrote:
> >
> >
> > On Sep 2, 2005, at 1:20 AM, Jean-Marc Godbout wrote:
> > >
> > > Well thanks for creating doubt in my mind. I was certain that on
> > > the unpatched "ls"used to complain when it couldn't find the
> > > "checked-in" property. But I might be wrong, i might have done
> > > something wrong here. I'll verify. And yes, if there are problems
> > > then they should be fixed asap so that eventually this behavior
> > > could be phased out. I'll get back to you on that.
> > >
> >
> > I can't imagine why svn_ra_get_dir() would care about the DAV:checked-
> > in property; it's not necessary at all to fill in the svn_dirent_t.
> >
> >
> >
> >
> > --
> > www.collab.net <http://www.collab.net> <> CollabNet | Distributed
> > Development On Demand
> >
> >
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> > For additional commands, e-mail: dev-help@subversion.tigris.org
> >
> >
> >
>
Re: [PATCH] Fix for issue 2151 "'svn ls' is slow over ra_dav"
Posted by Jean-Marc Godbout <jm...@gmail.com>.
On 9/1/05, Julian Reschke <ju...@gmx.de> wrote:
>
> Ben Collins-Sussman wrote:
> > ...
> >> Another change that would require an upgrade would be to make the
> >> server be more DeltaV compliant by simply not sending any DeltaV
> >> props on an allprop request (as described in issue #2151). This would
> >> mean that all older clients couldn't do "ls"s against patched
> servers...
> >
> >
> > You keep saying this, but why do you believe it to be true? If the
> > server stops sending DeltaV props in response to <allprop> requests,
> > which svn client commands break? And why?
> > ...
Well thanks for creating doubt in my mind. I was certain that on the
unpatched "ls"used to complain when it couldn't find the "checked-in"
property. But I might be wrong, i might have done something wrong here. I'll
verify. And yes, if there are problems then they should be fixed asap so
that eventually this behavior could be phased out. I'll get back to you on
that.
Assuming this is true, this really should be fixed in the client now, so
> that the server can be made compliant in a later release...
>
> Best regards, Julian
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
>
Re: [PATCH] Fix for issue 2151 "'svn ls' is slow over ra_dav"
Posted by Ben Collins-Sussman <su...@collab.net>.
On Sep 1, 2005, at 12:19 AM, Jean-Marc Godbout wrote:
>
> 1. Add the supports_deadprop_count variable to the session. If 2
> "svn ls" are done in the same session, then the second time around
> you wouldn't need to do that discovery propfind. This also
> simplifies #2.
>
> 2. Make an OPTIONS request to get the server options, then set the
> variable in the session. "ls" would then always know and wouldn't
> need to do the propfind.
I do like the idea of using OPTIONS -- it's very purpose is to
discover features, and so we'd be doing the right thing by making use
of it. I also like the idea of caching the feature's existence in
the svn_ra_dav_session_t.
>
> Another change that would require an upgrade would be to make the
> server be more DeltaV compliant by simply not sending any DeltaV
> props on an allprop request (as described in issue #2151). This
> would mean that all older clients couldn't do "ls"s against patched
> servers...
You keep saying this, but why do you believe it to be true? If the
server stops sending DeltaV props in response to <allprop> requests,
which svn client commands break? And why?
>
> Fixed. Also, is there a way to have the build fail if the code
> isn't C89 compliant?
>
We used to use a gcc flag to enforce C89, but it caused some other
side-effects, so we removed it. I can't remember the details...?
Haven't reviewed your new patch yet... will do so next.
--
www.collab.net <> CollabNet | Distributed Development On Demand
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix for issue 2151 "'svn ls' is slow over ra_dav"
Posted by Julian Reschke <ju...@gmx.de>.
Jean-Marc Godbout wrote:
> Woo, thanks for the comments. My notes below. As well I have attached my
> revised patch.
> ...
Question: does this patch include changes to the client so that it
doesn't rely anymore on the server returning RFC3253's live properties
upon PROPFIND/allprop? If this would be done now, future releases of the
server could be stop doing this...
Best regards, Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix for issue 2151 "'svn ls' is slow over ra_dav"
Posted by Jean-Marc Godbout <jm...@gmail.com>.
I didn't specifically look for that. "ls" no longer relies on RFC3253 live
props, but I don't know how the rest behaves. More testing would be needed -
I didn't make it an objective of this patch.
But I agree, it should be done everywhere.
On 9/1/05, Julian Reschke <ju...@gmx.de> wrote:
>
> Jean-Marc Godbout wrote:
> > Woo, thanks for the comments. My notes below. As well I have attached my
> > revised patch.
> > ...
>
> Question: does this patch include changes to the client so that it
> doesn't rely anymore on the server returning RFC3253's live properties
> upon PROPFIND/allprop? If this would be done now, future releases of the
> server could be stop doing this...
>
> Best regards, Julian
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
>
Re: [PATCH] Fix for issue 2151 "'svn ls' is slow over ra_dav"
Posted by Jean-Marc Godbout <jm...@gmail.com>.
Woo, thanks for the comments. My notes below. As well I have attached my
revised patch.
[[[
Fix for issue 2151 "'svn ls' is slow over ra_dav"
This patch implements a solution to issue 2151.
We now only request the needed props in the PROPFIND
for server listings. 'svn ls' is now noticably faster.
In most cases 'ls' takes about half the time and half
the bandwidth - In some case even better results.
For backward compatibility, before doing the long PROPFIND
we make another simpler PROPFIND to see if the server
supports the new type of request (supports the
deadprop-count prop). If it does we use the new scheme
and performance is improved - If not then we use the old
scheme and the slowness persists.
To summarise: only patched servers and patched clients
have improved speed. Mismatched configurations (old client
or server) are not improved but still work. Regular dav
clients are still slow.
Patch by: Jean-Marc Godbout <je...@computrad.com>
Suggested by: Erik Scrafford <er...@scrafford.org>
Ben Collins-Sussman <su...@collab.net>
Everyone else who contributed to issue 2151
* subversion/mod_dav_svn/liveprops.c
(SVN_PROPID_deadprop_count): Added a PROPID to support the new prop
(dav_svn_props[]): Added the deadprop-count prop to the list of
supported props.
(dav_svn_insert_prop): Returns the number of deadprops
found in the fs for that file.
* subversion/libsvn_ra_dav/ra_dav.h
(SVN_RA_DAV__PROP_DEADPROP_COUNT): Added the prop name for deadprop
(ELEM_deadprop_count): Added the prop to the list of props
* subversion/libsvn_ra_dav/props.c
(elem_definition, propfind_elements): Added ELEM_deadprop_count
to the list of propfind props
* subversion/libsvn_ra_dav/fetch.c
(dirent_props[]): New array, list of props to get in a dirent PROPFIND
(deadprop_count_support_props[ ]): New array, list of props to
get in a root PROPFIND
(svn_ra_dav__get_dir): check if server supports 'deadprop-count',
and if so, request fewer properties
]]]
- Show quoted text -
On 8/30/05, Ben Collins-Sussman <su...@collab.net> wrote:
>
> Thanks, this patch looks pretty good!
>
> This is a review from me and kfogel.
>
> The review includes both formatting problems, minor nits, and also
> some substantive comments on the changes. We look forward to the next
> iteration of the patch. :-)
>
>
> > [[[
> > Fix for issue 2151 "'svn ls' is slow over ra_dav"
> >
> > This patch implements a solution to issue 2151. We now only request
> > the needed props in the PROPFIND for server listings. 'svn ls' is now
> > noticably faster. In most cases 'ls' takes about half the time and
> > half the bandwidth - In some case even better results.
> >
> > For backward compatibility, before doing the long PROPFIND we make
> > another simpler PROPFIND to see if the server supports the new
> type of
> > request (supports the deadprop-count prop). If it does we use the new
> > scheme and performance is improved - If not then we use the old
> scheme
> > and the slowness persists.
> >
>
> Here's another strategy that we've been pondering:
>
> We see that the new 'deadprop-count' property is already in your short
> list of properties to fetch. Instead of unconditionally doing an
> extra PROPFIND for the new 'deadprop-count' property, why not just
> *always* request the short list in the depth-1 PROPFIND? If the new
> property doesn't come back, then repeat the depth-1 PROPFIND, asking
> for <allprops>.
>
> The problem with this is that 'svn ls' will now be about twice as slow
> against *old* servers -- doing two depth-1 PROPFINDs instead of one.
> So we wouldn't just be rewarding those who upgrade servers with 2x
> speed, we'd be actively *punishing* people who don't upgrade their
> servers with 1/2 speed. This may cause riots.
>
> So we're thinking that maybe we should stick with your strategy for
> now, but say, two releases from now, go with our strategy. By then,
> most servers will have upgraded, and it'll be silly for clients to be
> wasting time on an extra unconditional turnaround.
>
> Does that sound like a good idea?
Yes. There is another strategy which is to add the state of the server to
the session. That way you would only do the propfind if you are unsure if
the server supports the deadprop-count prop because you can't determine it
from other clues (result of an OPTIONS request for example). This could be
transferred to issue 1161 (excess propfinds). I will add this to the bottom
of the issue's discussion once I get home (I'm currently on the road).
1. Add the supports_deadprop_count variable to the session. If 2 "svn ls"
are done in the same session, then the second time around you wouldn't need
to do that discovery propfind. This also simplifies #2.
2. Make an OPTIONS request to get the server options, then set the variable
in the session. "ls" would then always know and wouldn't need to do the
propfind.
The additional OPTIONS would be a round trip that would replace the
propfind. It's currently not better than the propfind (it's still a round
trip) but if we also use it for other things, then we could roll in the
deadprop-count discovery in that.
This is another patch though I think. Another note about your solution is
that this extra propfind is not that expensive - we already have 7-8
comparable propfinds, this is simply one more.. and it could be removed on
implementation of the OPTIONS solution eventually. But I agree that the
ultimate goal would be to have fully patched clients and servers be as
efficient as possible, but I'm thinking that in this case the drawback of
unpatched servers would be very dramatic!
Another change that would require an upgrade would be to make the server be
more DeltaV compliant by simply not sending any DeltaV props on an allprop
request (as described in issue #2151). This would mean that all older
clients couldn't do "ls"s against patched servers... but it would make other
dav clients be much faster withe the subversion server.
> To summarise: only patched servers and patched clients have improved
> > speed. Mismatched configurations (old client or server) are not
> > improved but still work. Regular dav
> > clients are still slow.
> >
> > Patch by: Jean-Marc Godbout <je...@computrad.com>
> >
> > Suggested by: Erik Scrafford < erik@scrafford.org>
> > Ben Collins-Sussman <su...@collab.net>
> > Everyone else who contributed to issue 2151
> >
> >
> > * subversion/mod_dav_svn/liveprops.c
> > (SVN_PROPID_deadprop_count): Added a PROPID to support the new prop
> > (dav_svn_props[]): Added the deadprop-count prop to the list of
> > supported props.
> > (case SVN_PROPID_deadprop_count): Returns the number of deadprops
> > found in the fs for that file.
> >
>
>
> The name of the affected function goes in the parentheses: the 'case'
> shouldn't be in there, but rather "(dav_svn_insert_prop)".
Fixed
> * subversion/libsvn_ra_dav/ra_dav.h
> > (SVN_RA_DAV__PROP_DEADPROP_COUNT): Added the prop name for deadprop
> > (ELEM_deadprop_count): Added the prop to the list of props
> >
> > * subversion/libsvn_ra_dav/props.c
> > (elem_definition, propfind_elements): Added ELEM_deadprop_count
> > to the list of propfind props
> >
> > * subversion/libsvn_ra_dav/fetch.c
> > (dirent_props[]): list of props to get in a dirent PROPFIND
> > (root_props[]): list of props to get in a root PROPFIND
>
>
> Just a nit: when we add a new top-level symbol to the codebase, our
> general convention is to indicate that it's new, e.g.
>
> (dirent_props[]): New array, a list of props to get in a dirent
> PROPFIND.
> (root_props[]): New array, a list of props to get in a root PROPFIND.
Fixed
> (supports_deadprop_count): indicates if the server supports
> > the deadprop_count prop
> > (prop_count): the number of user props for the current dirent
> > as returned by the server
> > ]]]
>
>
> Non-top-level symbols don't get listed in parens. Instead, mention
> the function in which these variables were added, and describe the
> overall change to the function, e.g.
>
> (svn_ra_dav__get_dir): check if server supports 'deadprop-count',
> and if so, request fewer properties.
Done
- Show quoted text -
> >
> >
> >
> > Index: subversion/mod_dav_svn/liveprops.c
> > ===================================================================
> > --- subversion/mod_dav_svn/liveprops.c (revision 15946)
> > +++ subversion/mod_dav_svn/liveprops.c (working copy)
> > @@ -67,7 +67,8 @@
> > enum {
> > SVN_PROPID_baseline_relative_path = 1,
> > SVN_PROPID_md5_checksum,
> > - SVN_PROPID_repository_uuid
> > + SVN_PROPID_repository_uuid,
> > + SVN_PROPID_deadprop_count
> > };
> >
> > static const dav_liveprop_spec dav_svn_props[] =
> > @@ -96,6 +97,7 @@
> > SVN_RO_SVN_PROP(baseline_relative_path, baseline-relative-path),
> > SVN_RO_SVN_PROP(md5_checksum, md5-checksum),
> > SVN_RO_SVN_PROP(repository_uuid, repository-uuid),
> > + SVN_RO_SVN_PROP(deadprop_count, deadprop-count),
> >
> > { 0 } /* sentinel */
> > };
> > @@ -563,6 +565,30 @@
> > }
> > break;
> >
> > + case SVN_PROPID_deadprop_count:
> > + {
> > + if (resource->type != DAV_RESOURCE_TYPE_REGULAR)
> > + return DAV_PROP_INSERT_NOTSUPP;
> > +
> > + unsigned int propcount;
> > +
> > + apr_hash_t *proplist;
>
>
> We write to C89 standards (not C99), which means you can't declare
> variables right in the middle of a block like that. It needs to be
> declared at the beginning of a block.
Fixed. Also, is there a way to have the build fail if the code isn't C89
compliant?
> + serr = svn_fs_node_proplist(&proplist,
> > + resource->info->root.root,
> > + resource->info->repos_path, p);
> > + if (serr != NULL)
> > + {
> > + /* ### what to do? */
> > + svn_error_clear(serr);
> > + value = "###error###";
> > + break;
> > + }
> > +
> > + propcount = apr_hash_count(proplist);
> > + value = apr_psprintf(p, "%u", propcount);
> > + break;
> > + }
> > +
> > default:
> > /* ### what the heck was this property? */
> > return DAV_PROP_INSERT_NOTDEF;
> > Index: subversion/libsvn_ra_dav/ra_dav.h
> > ===================================================================
> > --- subversion/libsvn_ra_dav/ra_dav.h (revision 15946)
> > +++ subversion/libsvn_ra_dav/ra_dav.h (working copy)
> > @@ -368,6 +368,7 @@
> >
> > #define SVN_RA_DAV__PROP_REPOSITORY_UUID SVN_DAV_PROP_NS_DAV
> "repository-uuid"
> >
> > +#define SVN_RA_DAV__PROP_DEADPROP_COUNT "deadprop-count"
>
>
> Our liveprops all live in the SVN_DAV_PROP_NS_DAV namespace; see the
> repository-uuid definition as an example. Just add that constant to
> your declaration.
Done
>
> > typedef struct {
> > /* what is the URL for this resource */
> > @@ -707,7 +708,8 @@
> > ELEM_lock_owner,
> > ELEM_lock_comment,
> > ELEM_lock_creationdate,
> > - ELEM_lock_expirationdate
> > + ELEM_lock_expirationdate,
> > + ELEM_deadprop_count,
> > };
>
>
> In C89, you can't put a comma after the last element in a list. If
> only this were python. :-)
Done,
- Show quoted text -
>
> > /* ### docco */
> > Index: subversion/libsvn_ra_dav/props.c
> > ===================================================================
> > --- subversion/libsvn_ra_dav/props.c (revision 15946)
> > +++ subversion/libsvn_ra_dav/props.c (working copy)
> > @@ -111,6 +111,7 @@
> > { ELEM_baseline_relpath, SVN_RA_DAV__PROP_BASELINE_RELPATH, 1 },
> > { ELEM_md5_checksum, SVN_RA_DAV__PROP_MD5_CHECKSUM, 1 },
> > { ELEM_repository_uuid, SVN_RA_DAV__PROP_REPOSITORY_UUID, 1 },
> > + { ELEM_deadprop_count, SVN_RA_DAV__PROP_DEADPROP_COUNT, 1 },
> > { 0 }
> > };
> >
> > @@ -147,6 +148,8 @@
> > SVN_RA_DAV__XML_CDATA },
> > { SVN_DAV_PROP_NS_DAV, "repository-uuid", ELEM_repository_uuid,
> > SVN_RA_DAV__XML_CDATA },
> > + { SVN_DAV_PROP_NS_DAV, "deadprop-count", ELEM_deadprop_count,
> > + SVN_RA_DAV__XML_CDATA },
> >
> > /* Unknowns */
> > { "", "", ELEM_unknown, SVN_RA_DAV__XML_COLLECT },
> > Index: subversion/libsvn_ra_dav/fetch.c
> > ===================================================================
> > --- subversion/libsvn_ra_dav/fetch.c (revision 15946)
> > +++ subversion/libsvn_ra_dav/fetch.c (working copy)
> > @@ -914,7 +914,25 @@
> > return SVN_NO_ERROR;
> > }
> >
> > +/* the properties we need to fill in an svn_dirent_t, used by
> > + svn_ra_get_dir(). */
> > +static const ne_propname dirent_props[] =
> > +{
> > + { "DAV:", "resourcetype" }, /* kind */
> > + { "DAV:", "getcontentlength" }, /* size */
> > + { "DAV:", "version-name" }, /* created_rev */
> > + { "DAV:", "creationdate" }, /* time */
> > + { "DAV:", "creator-displayname" }, /* last_author */
> > + { SVN_DAV_PROP_NS_DAV, "deadprop-count" }, /* has_props */
> > + { NULL }
> > +};
>
>
> Just for readability, can you make the comments line up?
Yes sir.
>
> > +static const ne_propname root_props[] =
> > +{
> > + { SVN_DAV_PROP_NS_DAV, "deadprop-count" },
> > + { NULL }
> > +};
> > +
>
>
> Can we have a different name for this array? We're not sure what
> 'root' has to do with anything...
>
> Also, can you add a docstring before the array, just like
> dirent_props[] has?
>
> (Also, assuming you agree with our long-term strategy, this array will
> eventually go away, since we won't be doing the extra PROPFIND.)
Good point, I'm sorry about that unfortunate name.. for a while it made
sense in my head. Renamed to deadprop_count_support_props. More descriptive.
Also the array will disappear, yes - whichever strategy is used.
> svn_error_t *svn_ra_dav__get_dir(svn_ra_session_t *session,
> > const char *path,
> > svn_revnum_t revision,
> > @@ -955,13 +973,46 @@
> > *fetched_rev = got_rev;
> > }
> >
> > + /* For issue 2151:
> > + See if we are dealing with a server that understand the
> > + deadprop-count prop. If it doesn't we'll need to do an
> > + allprop PROPFIND - If it does, we'll execute a more
> > + targetted PROPFIND.
> > + */
> > + svn_boolean_t supports_deadprop_count;
> > +
>
>
> Again, new variables need to be declared at the beginning of a block.
Done
> + {
> > + const svn_string_t *deadprop_count;
> > +
> > + SVN_ERR( svn_ra_dav__get_props_resource(&rsrc, ras->sess,
> > + final_url, NULL,
> root_props,
> > + pool) );
> > +
> > + deadprop_count = apr_hash_get(rsrc->propset, "deadprop-
> count", APR_HASH_KEY_STRING);
>
>
> Oops, this line is more than 80 chars. Please fix.
Done
> +
> > + if (deadprop_count == NULL)
> > + supports_deadprop_count = FALSE;
> > + else
> > + supports_deadprop_count = TRUE;
> > + }
> > +
>
>
> Someday this whole block will go away, when we stop doing the extra
> PROPFIND.
Yes.
- Show quoted text -
> if (dirents)
> > {
> > /* 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) );
> > + if (supports_deadprop_count == TRUE)
> > + {
> > + SVN_ERR( svn_ra_dav__get_props(&resources, ras->sess,
> > + final_url, NE_DEPTH_ONE,
> > + NULL, dirent_props,
> pool) );
> > + }
> > + else /* The server doesn't support the deadprop_count prop,
> fallback */
> > + {
> > + SVN_ERR( svn_ra_dav__get_props(&resources, ras->sess,
> > + final_url, NE_DEPTH_ONE,
> > + NULL, NULL /* allprops */,
> > + pool) );
> > + }
> >
> > /* Count the number of path components in final_url. */
> > final_url_n_components = svn_path_component_count(final_url);
> > @@ -981,6 +1032,7 @@
> > const svn_string_t *propval;
> > apr_hash_index_t *h;
> > svn_dirent_t *entry;
> > + apr_int64_t prop_count;
> >
> > apr_hash_this (hi, &key, NULL, &val);
> > childname = key;
> > @@ -1009,20 +1061,41 @@
> >
> > /* 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 (supports_deadprop_count == TRUE)
> > {
> > - const void *kkey;
> > - void *vval;
> > - apr_hash_this (h, &kkey, NULL, &vval);
> > + propval = apr_hash_get(resource->propset,
> > +
> SVN_RA_DAV__PROP_DEADPROP_COUNT,
> > + APR_HASH_KEY_STRING);
> > +
> > + if (propval == NULL)
> > + entry->has_props = FALSE;
>
>
> If (supports_deadprop_count == TRUE) and there's no property
> value... that's an error, isn't it? It means the server has sent
> bogus data, no? Shouldn't we treat this as an error condition, rather
> than just setting has_props = FALSE?
My reasoning behind that is that if you were listing something NOT
DAV_RESOURCE_TYPE_REGULAR, then the server wouldn't have a deadprop-count
for that resource. This would mean that has_props should be false. We don't
list anything that's not DAV_RESOURCE_TYPE_REGULAR though, so this is moot.
Changed. I was debating between SVN_ERR_INCOMPLETE_DATA and
SVN_ERR_PROPERTY_NOT_FOUND. I settled on the former as from the point of vue
of the user who would recieve this error it provides a better picture of
what actually happenned (users don't care about properties in this case -
they care about a server sending invalid data). I don't feel too strongly
about this and you might have a better idea.
> + else
> > + {
> > + prop_count = svn__atoui64(propval->data);
> >
> > - 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;
> > + if (prop_count == 0)
> > + entry->has_props = FALSE;
> > + else
> > + entry->has_props = TRUE;
> > + }
> > + }
> > + else /* The server doesn't support the deadprop_count
> prop, fallback */
> > + {
> > + for (h = apr_hash_first (pool, resource->propset);
>
>
> Whoops! You accidentally inserted a tab character in the line above.
> Make sure your editor isn't doing that. We use only spaces in our
> source code.
Indeed I had changed my editor mid way through my changes. I tried to remove
all tabs. I missed that one evidently. Fixed.
> + h; h = apr_hash_next (h))
> > + {
> > + const void *kkey;
> > + void *vval;
> > + apr_hash_this (h, &kkey, NULL, &vval);
> > +
>
> We know this isn't your code here, but we just noticed that 'vval' is
> never used. Can you clean it up by just removing that variable and
> passing NULL as the last argument to apr_hash_this() ?
Done.
> + 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;
> > + }
> > }
> >
> > /* created_rev & friends */