You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Sergey Raevskiy <Se...@visualsvn.com> on 2019/12/09 15:54:03 UTC
[Patch] Rework of r1866425 ('Last-Modified' header)
Hello!
I've spent some time examining r1866425 [1] ('Last-Modified' header) and I
would like to suggest a patch with some rework related to this code.
I see two main problems in r1866425: usage of the pointer comparison (which is
hackish and relies on the implementation of parse_uri()) and using -1 for
'bad time' value.
APR_DATE_BAD is defined in APR headers by the way and it's value is 0 (zero),
not -1. Anyway, it is better to avoid 'special values' for APR_TIME_T if
possible.
I've attached a patch that removes both problems and also reduces code
coupling (since get_last_modified() function cannot be actually reused anywhere
but in set_headers()). Also I've removed RESOURCE_LACKS_ETAG_POTENTIAL() check
which is now redundant.
Log message:
[[[
Rework r1866425 ('Last-Modified' header). Do not use pointer comparison
and 'special value' for APR_TIME_T.
* subversion/mod_dav_svn/dav_svn.h
(dav_resource_private): Add new member IS_PUBLIC_URI.
* subversion/mod_dav_svn/repos.c
(parse_uri,
get_parentpath_resource): Set IS_PUBLIC_URI to TRUE for 'public' URIs (not
under '/!svn/')
(get_last_modified): Delete.
(set_headers): Rework of 'Last-Modified' related code.
Patch by: sergey.raevskiy{_AT_}visualsvn.com
]]]
[1] https://svn.apache.org/r1866425
Re: [Patch] Rework of r1866425 ('Last-Modified' header)
Posted by Nathan Hartman <ha...@gmail.com>.
On Mon, Dec 9, 2019 at 10:54 AM Sergey Raevskiy
<Se...@visualsvn.com> wrote:
>
> Hello!
>
> I've spent some time examining r1866425 [1] ('Last-Modified' header) and I
> would like to suggest a patch with some rework related to this code.
>
> I see two main problems in r1866425: usage of the pointer comparison (which is
> hackish and relies on the implementation of parse_uri()) and using -1 for
> 'bad time' value.
>
> APR_DATE_BAD is defined in APR headers by the way and it's value is 0 (zero),
> not -1. Anyway, it is better to avoid 'special values' for APR_TIME_T if
> possible.
>
> I've attached a patch that removes both problems and also reduces code
> coupling (since get_last_modified() function cannot be actually reused anywhere
> but in set_headers()). Also I've removed RESOURCE_LACKS_ETAG_POTENTIAL() check
> which is now redundant.
>
> Log message:
> [[[
> Rework r1866425 ('Last-Modified' header). Do not use pointer comparison
> and 'special value' for APR_TIME_T.
>
> * subversion/mod_dav_svn/dav_svn.h
> (dav_resource_private): Add new member IS_PUBLIC_URI.
>
> * subversion/mod_dav_svn/repos.c
> (parse_uri,
> get_parentpath_resource): Set IS_PUBLIC_URI to TRUE for 'public' URIs (not
> under '/!svn/')
> (get_last_modified): Delete.
> (set_headers): Rework of 'Last-Modified' related code.
>
> Patch by: sergey.raevskiy{_AT_}visualsvn.com
> ]]]
>
> [1] https://svn.apache.org/r1866425
Committed, r1872381.
Cheers,
Nathan
Re: [Patch] Rework of r1866425 ('Last-Modified' header)
Posted by Johan Corveleyn <jc...@gmail.com>.
On Tue, Dec 10, 2019 at 6:35 PM Nathan Hartman <ha...@gmail.com> wrote:
> On Mon, Dec 9, 2019 at 10:54 AM Sergey Raevskiy
> <Se...@visualsvn.com> wrote:
> > I've spent some time examining r1866425 [1] ('Last-Modified' header) and I
> > would like to suggest a patch with some rework related to this code.
> >
> > I see two main problems in r1866425: usage of the pointer comparison (which is
> > hackish and relies on the implementation of parse_uri()) and using -1 for
> > 'bad time' value.
> >
> > APR_DATE_BAD is defined in APR headers by the way and it's value is 0 (zero),
> > not -1. Anyway, it is better to avoid 'special values' for APR_TIME_T if
> > possible.
> >
> > I've attached a patch that removes both problems and also reduces code
> > coupling (since get_last_modified() function cannot be actually reused anywhere
> > but in set_headers()). Also I've removed RESOURCE_LACKS_ETAG_POTENTIAL() check
> > which is now redundant.
>
> Hello Sergey,
>
> Thank you for your work on svn and your patch.
>
> I studied the code and your changes. Also, I built it and ran the test
> suite successfully.
>
> +1 on avoiding the pointer comparison. That should help with
> "future-proofing" against changes in implementation details. Also,
> it's self-documenting.
>
> If I may nitpick a little, I'd nest the 2nd 'if' in the 1st 'if'
> block, i.e., changing this:
>
> [[[
>
> serr = svn_fs_node_created_rev(&created_rev, resource->info->root.root,
> resource->info->repos_path,
> resource->pool);
>
> if (serr == NULL)
> {
> serr = svn_fs_revision_prop2(&date_str, resource->info->repos->fs,
> created_rev, SVN_PROP_REVISION_DATE,
> TRUE, resource->pool, resource->pool);
> }
>
> if ((serr == NULL) && date_str && date_str->data)
> {
> apr_time_t mtime;
> serr = svn_time_from_cstring(&mtime, date_str->data, resource->pool);
>
> if (serr == NULL)
> {
> /* Note the modification time for the requested resource, and
> include the Last-Modified header in the response. */
> ap_update_mtime(r, mtime);
> ap_set_last_modified(r);
> }
> }
>
> ]]]
>
> to this:
>
> [[[
>
> serr = svn_fs_node_created_rev(&created_rev, resource->info->root.root,
> resource->info->repos_path,
> resource->pool);
>
> if (serr == NULL)
> {
> serr = svn_fs_revision_prop2(&date_str, resource->info->repos->fs,
> created_rev, SVN_PROP_REVISION_DATE,
> TRUE, resource->pool, resource->pool);
>
> if ((serr == NULL) && date_str && date_str->data)
> {
> apr_time_t mtime;
> serr = svn_time_from_cstring(&mtime, date_str->data,
> resource->pool);
>
> if (serr == NULL)
> {
> /* Note the modification time for the requested resource, and
> include the Last-Modified header in the response. */
> ap_update_mtime(r, mtime);
> ap_set_last_modified(r);
> }
> }
> }
>
> ]]]
>
> (Typically I would prefer to reduce levels of nesting but in this case
> we cannot 'return' if an error occurs because there is more code to
> execute later in the function. It could be left as a separate function
> as it was before, but I played with that idea for a while and found
> that your solution is simpler.)
>
> In general, I think this change is a step in the right direction. I
> hope someone more familiar with this part of the svn neighborhood will
> chime in soon...
>
> Once more, thank you.
>
> Nathan
I'm not very familiar with this part of the code either, but I was the
one who committed r1866425 [1].
FWIW: I did not write that get_last_modified function myself. I simply
resurrected it from before r1724790 [2], in which Ivan removed setting
of that header in all cases. So my recent commit was a partial revert
of Ivan's commit (with some tweaking so the header is only set for
external GETs).
Just for archeological reasons I did some further digging: the code in
question (get_last_modified function) was originally committed in
2005, in r857623 [3] by 'dlr'.
All that said, I'm all for improving the code, so definitely no
objections here (as long as it still works and does so performantly
:-)).
[1] https://svn.apache.org/r1866425
[2] https://svn.apache.org/r1724790
[3] https://svn.apache.org/r857623
--
Johan
Re: [Patch] Rework of r1866425 ('Last-Modified' header)
Posted by Nathan Hartman <ha...@gmail.com>.
On Mon, Dec 9, 2019 at 10:54 AM Sergey Raevskiy
<Se...@visualsvn.com> wrote:
> I've spent some time examining r1866425 [1] ('Last-Modified' header) and I
> would like to suggest a patch with some rework related to this code.
>
> I see two main problems in r1866425: usage of the pointer comparison (which is
> hackish and relies on the implementation of parse_uri()) and using -1 for
> 'bad time' value.
>
> APR_DATE_BAD is defined in APR headers by the way and it's value is 0 (zero),
> not -1. Anyway, it is better to avoid 'special values' for APR_TIME_T if
> possible.
>
> I've attached a patch that removes both problems and also reduces code
> coupling (since get_last_modified() function cannot be actually reused anywhere
> but in set_headers()). Also I've removed RESOURCE_LACKS_ETAG_POTENTIAL() check
> which is now redundant.
Hello Sergey,
Thank you for your work on svn and your patch.
I studied the code and your changes. Also, I built it and ran the test
suite successfully.
+1 on avoiding the pointer comparison. That should help with
"future-proofing" against changes in implementation details. Also,
it's self-documenting.
If I may nitpick a little, I'd nest the 2nd 'if' in the 1st 'if'
block, i.e., changing this:
[[[
serr = svn_fs_node_created_rev(&created_rev, resource->info->root.root,
resource->info->repos_path,
resource->pool);
if (serr == NULL)
{
serr = svn_fs_revision_prop2(&date_str, resource->info->repos->fs,
created_rev, SVN_PROP_REVISION_DATE,
TRUE, resource->pool, resource->pool);
}
if ((serr == NULL) && date_str && date_str->data)
{
apr_time_t mtime;
serr = svn_time_from_cstring(&mtime, date_str->data, resource->pool);
if (serr == NULL)
{
/* Note the modification time for the requested resource, and
include the Last-Modified header in the response. */
ap_update_mtime(r, mtime);
ap_set_last_modified(r);
}
}
]]]
to this:
[[[
serr = svn_fs_node_created_rev(&created_rev, resource->info->root.root,
resource->info->repos_path,
resource->pool);
if (serr == NULL)
{
serr = svn_fs_revision_prop2(&date_str, resource->info->repos->fs,
created_rev, SVN_PROP_REVISION_DATE,
TRUE, resource->pool, resource->pool);
if ((serr == NULL) && date_str && date_str->data)
{
apr_time_t mtime;
serr = svn_time_from_cstring(&mtime, date_str->data,
resource->pool);
if (serr == NULL)
{
/* Note the modification time for the requested resource, and
include the Last-Modified header in the response. */
ap_update_mtime(r, mtime);
ap_set_last_modified(r);
}
}
}
]]]
(Typically I would prefer to reduce levels of nesting but in this case
we cannot 'return' if an error occurs because there is more code to
execute later in the function. It could be left as a separate function
as it was before, but I played with that idea for a while and found
that your solution is simpler.)
In general, I think this change is a step in the right direction. I
hope someone more familiar with this part of the svn neighborhood will
chime in soon...
Once more, thank you.
Nathan