Lee Thompson <bm55b@yahoo.com> writes:
> Any subversion developers out there who can take a
> look at issue 971? I think I have most of it figured
> out, but it needs review and check-in.
>
> http://subversion.tigris.org/issues/show_bug.cgi?id=971
Lee, I'm rethreading this with "[PATCH]" in the subject, so people
realize there's actual code to review.
Folks, this is the
"Last-Modified and caching header not sent on GET/HEAD"
issue. Lee's patch is short, and he has (most excellently) linked to
discussion threads in the issue.
Regarding the patch itself:
> --- subversion-1.0.4-orig/subversion/mod_dav_svn/repos.c
> +++ subversion-1.0.4/subversion/mod_dav_svn/repos.c
> @@ -1822,6 +1822,44 @@
> /* we accept byte-ranges */
> apr_table_setn(r->headers_out, "Accept-Ranges", "bytes");
>
> + /* Experimental fix for issue 971, by Lee Thompson, bm55b@yahoo.com.
This line can be part of the log message. We try to avoid putting
names in source files, as it can make others feel that someone "owns"
the territory, and thereby discourage people from getting involved.
(Minor nit, of course, no big deal.)
> + If you add Last-Modified to the request header you can mirror the
> + subversion web site with utilities like wget.
> + To limit exposure to this experiment until subversion developers,
> + can review this change for accuracy, only activate for HEAD
> + or GET requests with user agent set to WGET or CURL.
> + The original bug was for things like SQUID to work, but I didn't
> + open this mod to squid as I didn't have a test for it.
> + Adding Last-Modified probably could be added for most requests
> + and at that point, issue 971 could be closed. */
> + if(r->headers_in && r->method
> + && (strstr(r->method, "HEAD") || strstr(r->method, "GET"))
> + && (r->method_number == M_GET))
> + {
> + const char *ua = apr_table_get(r->headers_in, "User-Agent");
> + if(ua && *ua && (strstr(ua, "curl") || strstr(ua, "Wget")))
> + {
Interesting. Since we need to remove all those extra conditionals to
really test this patch with Subversion clients anyway, let's not
bother with them. No point testing different code than what we're
planning to check in.
I think you can just rewrite the patch to not have these checks (for
HEAD, GET, curl, Wget). The comment can just speak about the general
goal of sending a Last-modified header and why it's good, and then
move on to that code.
> + const char *datestr;
> + apr_time_t timeval;
> + enum dav_svn_time_format format = dav_svn_time_format_rfc1123;
> +
> + /* ap_log_rerror(APLOG_MARK, APLOG_ERR, APR_EGENERAL, r,
> + ** "User-Agent: %s", ua);
> + */
I can see why you had a debugging log here, but I don't think it needs
to be in the production code.
> + if (dav_svn_get_last_modified_time (&datestr, &timeval,
> + resource, format, resource->pool))
> + {
> + /* log failure, move on */
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, APR_EGENERAL, r,
> + "dav_svn_get_last_modifed_time failed");
> + }
The requested path will be printed out in this log, right, because
it's available in 'r'? (I just want to make sure the log provides
enough information to prevent admins from scratching their heads in
perplexitude.)
> + else
> + {
> + apr_table_setn(r->headers_out, "Last-Modified", datestr);
> + }
> + }
> + }
> +
> /* For a directory, we will send text/html or text/xml. If we have a delta
> base, then we will always be generating an svndiff. Otherwise,
> we need to fetch the appropriate MIME type from the resource's
It looks good to me! Have you run 'make davcheck' on this? See
subversion/tests/clients/cmdline/README
for instructions on how to configure your local Apache for davcheck.
Thanks for taking this on!
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Jun 21 19:58:18 2004