On Mon, Dec 9, 2019 at 10:54 AM Sergey Raevskiy
<Sergey.Raevskiy_at_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
Received on 2019-12-10 18:35:12 CET