bliss@tigris.org writes:
> --- trunk/subversion/mod_dav_svn/liveprops.c (original)
> +++ trunk/subversion/mod_dav_svn/liveprops.c Thu Oct 7 16:58:10 2004
> @@ -106,6 +106,56 @@
> &dav_svn_hooks_liveprop
> };
>
> +/* Return the revision property PROPNAME's value in PROPVAL for
> + RESOURCE in the revision COMMITTED_REV after verifying that the
> + path is readable. If the property if inaccessible, SVN_NO_ERROR is
> + returned and *PROPVAL is NULL.
We usually phrase this (or anyway, I do) as:
"Set *PROPVAL to the value for revision property PROPNAME on
COMMITTED_REV, in the repository identified by RESOURCE. ..."
(Note that this resolves the confusion as to why a RESOURCE is
necessary when we're examining a *revision* property, too.)
Try to reserve the verb "return" for what the function actually
returns, which is usually an error object or NULL.
> + This function must only be used to retrieve properties for which it
> + is sufficient to have read access to a single changed path in the
> + revision to have access to the revprop, e.g.
> + SVN_PROP_REVISION_AUTHOR or SVN_PROP_REVISION_DATE.
"must"? Will the world blow up? :-)
Try instead to say what happens if the condition is violated: "Return
an error if PROPNAME is not a property for which it is sufficient to
have read access..." You can even specify the exact error, if you know
it (but then make sure the entire error propagation chain documents
the error code, because it's now a promise all the way down to the
place where the error is generated).
Oh, but wait, that's not the behavior. What actually happens is that
*PROPVAL is set to NULL, and SVN_NO_ERROR is returned. That's fine,
but then the setting of *PROPVAL to NULL with no accompanying error is
part of the API, and its meaning must be described. Especially
considering that the calling code now depends on it.
> + The reason for this is that we only check readability of the
> + current path (which is one of the revisions' changed paths per
> + definition). If the current path is readable, the revprop is also
> + readable. While it's possible that the property is readable even
> + though the current path is not readable (because another path in
> + the same revision is readable), it's a silly situation worth
> + ignoring to gain the extra performance. */
The first sentence of this paragraph is misleading. It implies that
the paragraph will merely justify one aspect of the API already
defined. But in fact, the paragraph defines a part of the API not
described anywhere else! Try rewriting to make it clear that this is
part of the API definition too.
What is "the current path"? The path for RESOURCE? It appears
so. Then say "RESOURCE's path" or something similar.
By the way, what is POOL used for? (Temp work I presume, and for
allocating *PROPVAL, but this should be said explicitly.)
I reviewed the code portion of the change too, no problems that I
saw. Thanks for the fix!
-Karl
> +static svn_error_t *svn_svn_get_path_revprop(svn_string_t **propval,
> + const dav_resource *resource,
> + svn_revnum_t committed_rev,
> + const char *propname,
> + apr_pool_t *pool)
> +{
> + dav_svn_authz_read_baton arb;
> + svn_boolean_t allowed;
> + svn_fs_root_t *root;
> +
> + *propval = NULL;
> +
> + arb.r = resource->info->r;
> + arb.repos = resource->info->repos;
> + SVN_ERR(svn_fs_revision_root(&root,
> + resource->info->repos->fs,
> + committed_rev, pool));
> + SVN_ERR(dav_svn_authz_read(&allowed,
> + root,
> + resource->info->repos_path,
> + &arb, pool));
> +
> + if (! allowed)
> + return SVN_NO_ERROR;
> +
> + /* Get the property of the created revision. The authz is already
> + performed, so we don't need to do it here too. */
> + return svn_repos_fs_revision_prop(propval,
> + resource->info->repos->repos,
> + committed_rev,
> + propname,
> + NULL, NULL, pool);
> +}
>
> static dav_prop_insert dav_svn_insert_prop(const dav_resource *resource,
> int propid, dav_prop_insert what,
> @@ -183,8 +233,6 @@
> {
> svn_revnum_t committed_rev = SVN_INVALID_REVNUM;
> svn_string_t *last_author = NULL;
> - dav_svn_authz_read_baton arb;
> - svn_boolean_t allowed;
>
> /* ### for now, our global VCC has no such property. */
> if (resource->type == DAV_RESOURCE_TYPE_PRIVATE
> @@ -220,20 +268,11 @@
> return DAV_PROP_INSERT_NOTSUPP;
> }
>
> - /* Check if we have access to this path and return NOTDEF if
> - we don't. It is enough to determine if we have read access
> - to the current path because the rules dictate that svn:date
> - is accessible if at least one changed path is accessible.
> - While it's possible that the property is accessible even
> - though the current path is inaccessible (because another
> - path in the same revision is accessible), it's a silly
> - situation worth ignoring to gain the extra performance. */
> - arb.r = resource->info->r;
> - arb.repos = resource->info->repos;
> - serr = dav_svn_authz_read(&allowed,
> - resource->info->root.root,
> - resource->info->repos_path,
> - &arb, p);
> + serr = svn_svn_get_path_revprop(&last_author,
> + resource,
> + committed_rev,
> + SVN_PROP_REVISION_AUTHOR,
> + p);
> if (serr)
> {
> /* ### what to do? */
> @@ -241,23 +280,6 @@
> value = "###error###";
> break;
> }
> - if (! allowed)
> - return DAV_PROP_INSERT_NOTDEF;
> -
> - /* Get the svn:author property of the created revision. The authz
> - is already performed, so we don't need to do it here too. */
> - serr = svn_repos_fs_revision_prop(&last_author,
> - resource->info->repos->repos,
> - committed_rev,
> - SVN_PROP_REVISION_AUTHOR,
> - NULL, NULL, p);
> - if (serr != NULL)
> - {
> - /* ### what to do? */
> - svn_error_clear(serr);
> - value = "###error###";
> - break;
> - }
>
> if (last_author == NULL)
> return DAV_PROP_INSERT_NOTDEF;
> @@ -688,34 +710,10 @@
> svn_string_t *committed_date = NULL;
> svn_error_t *serr;
> apr_time_t timeval_tmp;
> - dav_svn_authz_read_baton arb;
> - svn_boolean_t allowed;
>
> if ((datestring == NULL) && (timeval == NULL))
> return 0;
>
> - /* Check if we have access to this path and return NOTDEF if we
> - don't. It is enough to determine if we have read access to the
> - current path because the rules dictate that svn:date is
> - accessible if at least one changed path is accessible. While
> - it's possible that the property is accessible even though the
> - current path is inaccessible (because another path in the same
> - revision is accessible), it's a silly situation worth ignoring to
> - gain the extra performance. */
> - arb.r = resource->info->r;
> - arb.repos = resource->info->repos;
> - serr = dav_svn_authz_read(&allowed,
> - resource->info->root.root,
> - resource->info->repos_path,
> - &arb, pool);
> - if (serr)
> - {
> - svn_error_clear(serr);
> - return 1;
> - }
> - if (! allowed)
> - return 1;
> -
> if (resource->baselined && resource->type == DAV_RESOURCE_TYPE_VERSION)
> {
> /* A baseline URI. */
> @@ -740,24 +738,19 @@
> return 1;
> }
>
> - /* Get the svn:date property of the CR. The authz is already
> - performed, so we don't need to do it here too. */
> - serr = svn_repos_fs_revision_prop(&committed_date,
> - resource->info->repos->repos,
> - committed_rev,
> - SVN_PROP_REVISION_DATE,
> - NULL, NULL,
> - pool);
> - if (serr != NULL)
> + serr = svn_svn_get_path_revprop(&committed_date,
> + resource,
> + committed_rev,
> + SVN_PROP_REVISION_DATE,
> + pool);
> + if (serr)
> {
> svn_error_clear(serr);
> return 1;
> }
> -
> +
> if (committed_date == NULL)
> - {
> - return 1;
> - }
> + return 1;
>
> /* return the ISO8601 date as an apr_time_t */
> serr = svn_time_from_cstring(&timeval_tmp, committed_date->data, pool);
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: svn-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: svn-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Oct 11 21:59:42 2004