On Tue, Dec 10, 2019 at 6:35 PM Nathan Hartman <hartman.nat...@gmail.com> wrote:
> On Mon, Dec 9, 2019 at 10:54 AM Sergey Raevskiy
> <sergey.raevs...@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
