[prev in list] [next in list] [prev in thread] [next in thread] 

List:       subversion-dev
Subject:    Re: Last-Modified HTTP header from mod_dav_svn
From:       Daniel Rall <dlr () collab ! net>
Date:       2005-11-29 22:56:19
Message-ID: 20051129225619.GZ496 () anvil ! sp ! corp ! collab ! net
[Download RAW message or body]

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

[prev in list] [next in list] [prev in thread] [next in thread]