You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by kf...@collab.net on 2004/10/11 18:10:22 UTC
Re: svn commit: r11285 - trunk/subversion/mod_dav_svn
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
Re: svn commit: r11285 - trunk/subversion/mod_dav_svn
Posted by kf...@collab.net.
Tobias Ringström <to...@ringstrom.mine.nu> writes:
> I decided not to because it's just a static helper function, and also
> because it's possible for that set of properties to change in the
> future. Since the function is only used for svn:author and svn:date,
> nobody will have to change this code if the set changes.
Okay, that sounds fine to me.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r11285 - trunk/subversion/mod_dav_svn
Posted by Tobias Ringström <to...@ringstrom.mine.nu>.
kfogel@collab.net wrote:
>Nice warning. So, question: should this function just error if the
>requested property is one which requires *all* changed paths to be
>readable?
>
>
I decided not to because it's just a static helper function, and also
because it's possible for that set of properties to change in the
future. Since the function is only used for svn:author and svn:date,
nobody will have to change this code if the set changes.
/Tobias
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r11285 - trunk/subversion/mod_dav_svn
Posted by kf...@collab.net.
Tobias Ringström <to...@ringstrom.mine.nu> writes:
> Thanks a lot for the review and the helpful comments, Karl. I've been
> wrestling with the docstring quite a bit now, and I've come up with
> the following proposal which is quite different from the original
> one. What do you think?
Much better! Comments below.
> /* Set *PROPVAL to the value for the revision property PROPNAME on
> COMMITTED_REV, in the repository identified by RESOURCE, if
> RESOURCE's path is readable. If it is not readable, SVN_NO_ERROR
> is returned and *PROPVAL is set to NULL. Use POOL for temporary
> allocations and the allocation of *PROPVAL.
That paragraph is great. But don't switch to the passive voice for
the second sentence. Just say "If it is not readable, set *PROPVAL to
NULL and return SVN_NO_ERROR."
> Note that this function does not check the readability of the
> revision property, but the readability of a path. The true
> readability of a revision property is determined by investigating
> the readability of all changed paths in the revision. For certain
> revision properties (e.g. svn:author and svn:date) to be readable,
> it is enough if at least one changed path is readable. When we
> already have a changed path, we can skip the check for the other
> changed paths in the revision and save a lot of work. This means
> that we will make a mistake when our path is unreadable and another
> changed path is readable, but we will at least only hide too much
> and not leak any protected properties.
>
> WARNING: This method of only checking the readability of a path is
> only valid to get revision properties for which it is enough if at
> least one changed path is readable. Using this function to get
> revision properties for which all changed paths must be readable
> might leak protected information because we will only test the
> readability of a single changed path.
> */
Nice warning. So, question: should this function just error if the
requested property is one which requires *all* changed paths to be
readable?
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r11285 - trunk/subversion/mod_dav_svn
Posted by Tobias Ringström <to...@ringstrom.mine.nu>.
Thanks a lot for the review and the helpful comments, Karl. I've been
wrestling with the docstring quite a bit now, and I've come up with the
following proposal which is quite different from the original one. What
do you think?
/Tobias
/* Set *PROPVAL to the value for the revision property PROPNAME on
COMMITTED_REV, in the repository identified by RESOURCE, if
RESOURCE's path is readable. If it is not readable, SVN_NO_ERROR
is returned and *PROPVAL is set to NULL. Use POOL for temporary
allocations and the allocation of *PROPVAL.
Note that this function does not check the readability of the
revision property, but the readability of a path. The true
readability of a revision property is determined by investigating
the readability of all changed paths in the revision. For certain
revision properties (e.g. svn:author and svn:date) to be readable,
it is enough if at least one changed path is readable. When we
already have a changed path, we can skip the check for the other
changed paths in the revision and save a lot of work. This means
that we will make a mistake when our path is unreadable and another
changed path is readable, but we will at least only hide too much
and not leak any protected properties.
WARNING: This method of only checking the readability of a path is
only valid to get revision properties for which it is enough if at
least one changed path is readable. Using this function to get
revision properties for which all changed paths must be readable
might leak protected information because we will only test the
readability of a single changed path.
*/
kfogel@collab.net wrote:
>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
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org