On Mon, Jun 21, 2004 at 11:33:31AM -0500, kfogel@collab.net wrote:
> Lee Thompson <bm55b@yahoo.com> writes:
>...
> > --- subversion-1.0.4-orig/subversion/mod_dav_svn/repos.c
> > +++ subversion-1.0.4/subversion/mod_dav_svn/repos.c
>...
The patch should use Apache's mechanisms to set this header. Note the
#if'd sections at the top of the dav_svn_set_headers() function.
(also note that the comment about r->filename is obsolete)
The very first comment in the function also notes that considerations for
the resource type much be made. dav_svn_get_last_modified_time() handles
that, so the comment is probably obsolete. Though I'd suggest just
changing it to a big NOTE so that developers will know care must be taken
within this function.
Note that you don't want a string from dav_svn_get_last_modified_time.
Just ask it for the apr_time_t value, and then pass that to
ap_update_mtime() (if a time is returned, of course).
> > + If you add Last-Modified to the request header you can mirror the
> > + subversion web site with utilities like wget.
>...
> > + Adding Last-Modified probably could be added for most requests
> > + and at that point, issue 971 could be closed. */
Nope. That issue also discusses setting the cache headers. The contents of
a specific revision will *never* change, so it is possible to set a
caching header to say "cache this forever".
> > + 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.
I'll strengthen this: client-specific checks should NOT be in the code.
Simple as that. The check for the method is also done improperly, but it
doesn't matter: just strike the whole thing.
> > + 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.
Agreed. This stuff is easy to get from the apache logs.
> > + if (dav_svn_get_last_modified_time (&datestr, &timeval,
No space before the paren.
>...
> > + {
> > + /* 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'?
No, it won't. More information should be included into the log. However,
that function will return non-zero for perfectly normal situations (the
resource is a collection, for example). Instead, you should change the
return value of the function to an svn_error_t to really indicate error
situations (hunh; actually, right now, it will "leak" errors). But then
you'll also need to determine what should happen for a non-error
situation, but where the function doesn't want to return a value. You may
need an "out" boolean variable to distinguish whether a NULL return means
"no error, and you have a value" and "no error, and you don't have a
value". So... then you'd have an svn_error_t which *can* get logged when
it happens.
>...
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Jun 22 08:00:49 2004