Yikes, sounds like dav_svn_get_last_modified_time()
needs a decent refactoring.
--- Greg Stein <gstein@lyra.org> wrote:
> 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
>
>
__________________________________
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 Tue Jun 22 08:34:37 2004