I think all your comments are dead on correct. I'll
rework the patch this coming weekend and repost.
Thanks for the feedback.
Lee
--- kfogel@collab.net wrote:
> 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
>
>
__________________________________
Do you Yahoo!?
Yahoo! Mail - 50x more storage than other providers!
http://promotions.yahoo.com/new_mail
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Jun 21 21:59:36 2004