Justin Erenkrantz wrote:
The concept is fine. (And as Joshua pointed out 'CacheVaryOverride'
would be a better name.)
I'm not attatched to the name. So that sounds good to me.
A few issues with the implementation (modulo style nits)...
This was all done in a few hours today, including testing. Some stuff,
especially in cache_util, looks really ugly :)
+ if((header = apr_table_get(r->subprocess_env, o)) == NULL) {
+ header = "-";
+ }
+ }
I don't understand what the assignment to "-" is doing here.
Just a default value when the environment is not set. Need to have a
default to handle cases when header is not set. I thought about having
this default be configurable per override. In short, "-" was just as
good a default as any.
+ ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
+ "ap_cache_override_header: %s => %s", key, header);
I don't think its necessary to log in the case where it is a no-op - so
I'd stick this in an else branch of the conditional above.
Probably not necessary to log at all. I left this in here until me/we
feel confident with it.
-static const char* regen_key(apr_pool_t *p, apr_table_t *headers,
+static const char* regen_key(request_rec *r,
apr_array_header_t *varray, const char
*oldkey)
I would prefer that we keep the headers argument explicit rather than
hardcoding that it is r->headers_in. -- justin
Sure. I just noticed everytime regen_key was called, it was called with
r->headers_in.
--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies
