Re: Last-Modified HTTP header from mod_dav_svn

On Tue, 29 Nov 2005, Greg Stein wrote:

> On Mon, Nov 28, 2005 at 09:32:19PM -0800, Daniel Rall wrote:
...
> > http://svn.collab.net/viewcvs/svn/trunk/subversion/mod_dav_svn/repos.c?r1=17547&r2=17549
...
> > > > For (at least) version resources, we should also be setting the
> > > > Cache-Control header. The max-age should be set to some ridiculously
> > > > high number since a version resource can't change.
> > >
> > > Greg, are you referring to this specific type (from mod_dav.h)?:
> > >
> > > DAV_RESOURCE_TYPE_VERSION, /* version or baseline URL */
>
> Yes. Essentially that type refers to a specific <rev, path> pair.
>
> > Or did you mean versioned resources like files and directories?
>
> "version resource" is a DAV term with a specific meaning. We're
> probably talking about the same thing :-)

Yes, we are, thanks for the clarification (and thanks to sussman as
well!). I didn't understand that DAV_RESOURCE_TYPE_VERSION was an
indicator for a <rev, path> pair.

...
> Note that the LACKS_ETAG macro also provides an etag for REGULAR
> resources, which you can't do. Those change over time, so they
> shouldn't use Cache-Control (let the proxy use the etag and L-M header
> to see if the resource has changed).
>
> I'm not sure in which cases dav_svn_set_headers() is called, but
> hopefully just for GET/HEAD requests. Should be double-checked.

I've confirmed that it's not called for PROPFIND requests, and is
called for HEAD and GET requests. I haven't checked other HTTP
methods, but given that it is used in the definition of mod_dav_svn's
dav_hooks_repository, I'm fairly certain dav_svn_set_headers()
functions as desired. Here's the API it conforms to (as defined by
mod_dav.h):

    /*
    ** If a GET is processed using a stream (open_stream, read_stream)
    ** rather than via a sub-request (on get_pathname), then this function
    ** is used to provide the repository with a way to set the headers
    ** in the response.
    **
    ** This function may be called without a following deliver(), to
    ** handle a HEAD request.
    **
    ** This may be NULL if handle_get is FALSE.
    */
    dav_error * (*set_headers)(request_rec *r,
                               const dav_resource *resource);

> Oh, and note that setting MAX_SECONDS to even just a day would be a
> big win. Make it a year if you want, but if you grow hinky with that
> duration, then something less will still be a Good Thing. (I'd go with
> a week, I think)

I can't seem to get the conditional right. 'svn cat -r2 http://...'
is apparently neither a DAV_RESOURCE_TYPE_VERSION, nor a
resource->baselined.

--- subversion/mod_dav_svn/repos.c (revision 17559)
+++ subversion/mod_dav_svn/repos.c (working copy)
@@ -2176,6 +2176,11 @@
   apr_table_setn(r->headers_out, "ETag",
                  dav_svn_getetag(resource, resource->pool));

+ /* As version resources don't change, encourage caching. */
+ if (resource->type == DAV_RESOURCE_TYPE_VERSION)
+ /* Cache resource for one week (specified in seconds). */
+ apr_table_setn(r->headers_out, "Cache-Control", "max-age=604800");
+
   /* we accept byte-ranges */
   apr_table_setn(r->headers_out, "Accept-Ranges", "bytes");

Any suggestions as to how the conditional ought to be implemented?

-- 
Daniel Rall
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org