List for cgit developers and users
 help / color / mirror / Atom feed
* [PATCH 0/2] TTL updates
@ 2016-02-26 20:57 tim.nordell
  2016-02-26 20:57 ` [PATCH 1/2] ttl: Consolidate TTL calculation tim.nordell
  2016-02-26 20:57 ` [PATCH 2/2] ttl: Support different TTL times based on cache-control tim.nordell
  0 siblings, 2 replies; 6+ messages in thread
From: tim.nordell @ 2016-02-26 20:57 UTC (permalink / raw)


This patchset simplifies the TTL calculation into one routine and uses
seconds internally through the cache routines and the routines in
cgit.c.  (The configuration file still uses minutes.)  Additionally,
this patchset adds a new syntax for the cache lines that allows a
different cache lifetime if the user refreshes the page.

Tim Nordell (2):
  ttl: Consolidate TTL calculation
  ttl: Support different TTL times based on cache-control

 cache.c     |  2 +-
 cgit.c      | 99 ++++++++++++++++++++++++++++++++++++++++++-------------------
 cgit.h      | 18 +++++++----
 ui-shared.c |  2 +-
 4 files changed, 82 insertions(+), 39 deletions(-)

-- 
2.4.9



^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/2] ttl: Consolidate TTL calculation
  2016-02-26 20:57 [PATCH 0/2] TTL updates tim.nordell
@ 2016-02-26 20:57 ` tim.nordell
  2016-02-28 12:22   ` john
  2016-02-26 20:57 ` [PATCH 2/2] ttl: Support different TTL times based on cache-control tim.nordell
  1 sibling, 1 reply; 6+ messages in thread
From: tim.nordell @ 2016-02-26 20:57 UTC (permalink / raw)


Currently part of the TTL calculation is done outside of the
calc_ttl() function.  Ideally a single variable in main()
would have the total seconds until the cache expires, and
ctx.page.expires would be incremented by this, as well
as the parameter to cache_process getting this value in
seconds.

Signed-off-by: Tim Nordell <tim.nordell at logicpd.com>

diff --git a/cache.c b/cache.c
index 6736a01..417f1a2 100644
--- a/cache.c
+++ b/cache.c
@@ -126,7 +126,7 @@ static int is_expired(struct cache_slot *slot)
 	if (slot->ttl < 0)
 		return 0;
 	else
-		return slot->cache_st.st_mtime + slot->ttl * 60 < time(NULL);
+		return slot->cache_st.st_mtime + slot->ttl < time(NULL);
 }
 
 /* Check if the slot has been modified since we opened it.
diff --git a/cgit.c b/cgit.c
index fc482be..963aee8 100644
--- a/cgit.c
+++ b/cgit.c
@@ -1005,25 +1005,28 @@ static void cgit_parse_args(int argc, const char **argv)
 
 static int calc_ttl(void)
 {
-	if (!ctx.repo)
-		return ctx.cfg.cache_root_ttl;
-
-	if (!ctx.qry.page)
-		return ctx.cfg.cache_repo_ttl;
-
-	if (!strcmp(ctx.qry.page, "about"))
-		return ctx.cfg.cache_about_ttl;
-
-	if (!strcmp(ctx.qry.page, "snapshot"))
-		return ctx.cfg.cache_snapshot_ttl;
+	const int ttl_conversion_multiplier = 60;
+	int ttl;
 
-	if (ctx.qry.has_sha1)
-		return ctx.cfg.cache_static_ttl;
+	if (!ctx.repo)
+		ttl = ctx.cfg.cache_root_ttl;
+	else if (!ctx.qry.page)
+		ttl = ctx.cfg.cache_repo_ttl;
+	else if (!strcmp(ctx.qry.page, "about"))
+		ttl = ctx.cfg.cache_about_ttl;
+	else if (!strcmp(ctx.qry.page, "snapshot"))
+		ttl = ctx.cfg.cache_snapshot_ttl;
+	else if (ctx.qry.has_sha1)
+		ttl = ctx.cfg.cache_static_ttl;
+	else if (ctx.qry.has_symref)
+		ttl = ctx.cfg.cache_dynamic_ttl;
+	else
+		ttl = ctx.cfg.cache_repo_ttl;
 
-	if (ctx.qry.has_symref)
-		return ctx.cfg.cache_dynamic_ttl;
+	if (ttl >= 0)
+		return ttl * ttl_conversion_multiplier;
 
-	return ctx.cfg.cache_repo_ttl;
+	return 10 * 365 * 24 * 60 * 60; /* 10 years */
 }
 
 int main(int argc, const char **argv)
@@ -1076,10 +1079,7 @@ int main(int argc, const char **argv)
 	authenticate_cookie();
 
 	ttl = calc_ttl();
-	if (ttl < 0)
-		ctx.page.expires += 10 * 365 * 24 * 60 * 60; /* 10 years */
-	else
-		ctx.page.expires += ttl * 60;
+	ctx.page.expires += ttl;
 	if (!ctx.env.authenticated || (ctx.env.request_method && !strcmp(ctx.env.request_method, "HEAD")))
 		ctx.cfg.nocache = 1;
 	if (ctx.cfg.nocache)
-- 
2.4.9



^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 2/2] ttl: Support different TTL times based on cache-control
  2016-02-26 20:57 [PATCH 0/2] TTL updates tim.nordell
  2016-02-26 20:57 ` [PATCH 1/2] ttl: Consolidate TTL calculation tim.nordell
@ 2016-02-26 20:57 ` tim.nordell
  2016-02-28 12:32   ` john
  1 sibling, 1 reply; 6+ messages in thread
From: tim.nordell @ 2016-02-26 20:57 UTC (permalink / raw)


Allow the client browser to pass in "max-age=0" and "no-cache"
to control a separate TTL time on the server for each type of
page.  This extends the TTL field to have the additional form
of:

    some-ttl=5:1

where 5 is the number of minutes if the user were to simply
load the page, and 1 is the number of minutes if the user were
to reload the page in their web browser (hit refresh).

Signed-off-by: Tim Nordell <tim.nordell at logicpd.com>

diff --git a/cgit.c b/cgit.c
index 963aee8..0ca1baa 100644
--- a/cgit.c
+++ b/cgit.c
@@ -29,6 +29,30 @@ static void add_mimetype(const char *name, const char *value)
 
 static void process_cached_repolist(const char *path);
 
+/**
+ * \brief Convert TTL field into a TTL structure
+ *
+ * \param value Value from field; should be one of the following forms:
+ *                      [normal]
+ *                      [normal]:[forced]
+ * \return Parsed TTL value
+ */
+static struct cgit_ttl ttl_value(const char *value)
+{
+	char *param2;
+	struct cgit_ttl ret;
+
+	ret.normal = atoi(value);
+
+	param2 = strchr(value, ':');
+	if(NULL != param2)
+		ret.forced = atoi(&param2[1]);
+	else
+		ret.forced = -1;
+
+	return ret;
+}
+
 static void repo_config(struct cgit_repo *repo, const char *name, const char *value)
 {
 	struct string_list_item *item;
@@ -187,19 +211,19 @@ static void config_cb(const char *name, const char *value)
 	else if (!strcmp(name, "cache-root"))
 		ctx.cfg.cache_root = xstrdup(expand_macros(value));
 	else if (!strcmp(name, "cache-root-ttl"))
-		ctx.cfg.cache_root_ttl = atoi(value);
+		ctx.cfg.cache_root_ttl = ttl_value(value);
 	else if (!strcmp(name, "cache-repo-ttl"))
-		ctx.cfg.cache_repo_ttl = atoi(value);
+		ctx.cfg.cache_repo_ttl = ttl_value(value);
 	else if (!strcmp(name, "cache-scanrc-ttl"))
 		ctx.cfg.cache_scanrc_ttl = atoi(value);
 	else if (!strcmp(name, "cache-static-ttl"))
-		ctx.cfg.cache_static_ttl = atoi(value);
+		ctx.cfg.cache_static_ttl = ttl_value(value);
 	else if (!strcmp(name, "cache-dynamic-ttl"))
-		ctx.cfg.cache_dynamic_ttl = atoi(value);
+		ctx.cfg.cache_dynamic_ttl = ttl_value(value);
 	else if (!strcmp(name, "cache-about-ttl"))
-		ctx.cfg.cache_about_ttl = atoi(value);
+		ctx.cfg.cache_about_ttl = ttl_value(value);
 	else if (!strcmp(name, "cache-snapshot-ttl"))
-		ctx.cfg.cache_snapshot_ttl = atoi(value);
+		ctx.cfg.cache_snapshot_ttl = ttl_value(value);
 	else if (!strcmp(name, "case-sensitive-sort"))
 		ctx.cfg.case_sensitive_sort = atoi(value);
 	else if (!strcmp(name, "about-filter"))
@@ -352,13 +376,19 @@ static void prepare_context(void)
 	ctx.cfg.cache_size = 0;
 	ctx.cfg.cache_max_create_time = 5;
 	ctx.cfg.cache_root = CGIT_CACHE_ROOT;
-	ctx.cfg.cache_about_ttl = 15;
-	ctx.cfg.cache_snapshot_ttl = 5;
-	ctx.cfg.cache_repo_ttl = 5;
-	ctx.cfg.cache_root_ttl = 5;
+	ctx.cfg.cache_about_ttl.normal = 15;
+	ctx.cfg.cache_about_ttl.forced = -1;
+	ctx.cfg.cache_snapshot_ttl.normal = 5;
+	ctx.cfg.cache_snapshot_ttl.forced = -1;
+	ctx.cfg.cache_repo_ttl.normal = 5;
+	ctx.cfg.cache_repo_ttl.forced = -1;
+	ctx.cfg.cache_root_ttl.normal = 5;
+	ctx.cfg.cache_root_ttl.forced = -1;
 	ctx.cfg.cache_scanrc_ttl = 15;
-	ctx.cfg.cache_dynamic_ttl = 5;
-	ctx.cfg.cache_static_ttl = -1;
+	ctx.cfg.cache_dynamic_ttl.normal = 5;
+	ctx.cfg.cache_dynamic_ttl.forced = -1;
+	ctx.cfg.cache_static_ttl.normal = -1;
+	ctx.cfg.cache_static_ttl.forced = -1;
 	ctx.cfg.case_sensitive_sort = 1;
 	ctx.cfg.branch_sort = 0;
 	ctx.cfg.commit_sort = 0;
@@ -405,6 +435,7 @@ static void prepare_context(void)
 	ctx.env.server_port = getenv("SERVER_PORT");
 	ctx.env.http_cookie = getenv("HTTP_COOKIE");
 	ctx.env.http_referer = getenv("HTTP_REFERER");
+	ctx.env.http_cache_control = getenv("HTTP_CACHE_CONTROL");
 	ctx.env.content_length = getenv("CONTENT_LENGTH") ? strtoul(getenv("CONTENT_LENGTH"), NULL, 10) : 0;
 	ctx.env.authenticated = 0;
 	ctx.page.mimetype = "text/html";
@@ -1006,7 +1037,7 @@ static void cgit_parse_args(int argc, const char **argv)
 static int calc_ttl(void)
 {
 	const int ttl_conversion_multiplier = 60;
-	int ttl;
+	struct cgit_ttl ttl;
 
 	if (!ctx.repo)
 		ttl = ctx.cfg.cache_root_ttl;
@@ -1023,8 +1054,14 @@ static int calc_ttl(void)
 	else
 		ttl = ctx.cfg.cache_repo_ttl;
 
-	if (ttl >= 0)
-		return ttl * ttl_conversion_multiplier;
+	if(ttl.forced >= 0 && NULL != ctx.env.http_cache_control &&
+	    (!strcmp(ctx.env.http_cache_control, "max-age=0") ||
+	     !strcmp(ctx.env.http_cache_control, "no-cache"))
+	  )
+		return ttl.forced * ttl_conversion_multiplier;
+
+	if (ttl.normal >= 0)
+		return ttl.normal * ttl_conversion_multiplier;
 
 	return 10 * 365 * 24 * 60 * 60; /* 10 years */
 }
diff --git a/cgit.h b/cgit.h
index 325432b..350d25d 100644
--- a/cgit.h
+++ b/cgit.h
@@ -185,6 +185,11 @@ struct cgit_query {
 	char *vpath;
 };
 
+struct cgit_ttl {
+	int normal;
+	int forced;
+};
+
 struct cgit_config {
 	char *agefile;
 	char *cache_root;
@@ -213,14 +218,14 @@ struct cgit_config {
 	char *virtual_root;	/* Always ends with '/'. */
 	char *strict_export;
 	int cache_size;
-	int cache_dynamic_ttl;
 	int cache_max_create_time;
-	int cache_repo_ttl;
-	int cache_root_ttl;
 	int cache_scanrc_ttl;
-	int cache_static_ttl;
-	int cache_about_ttl;
-	int cache_snapshot_ttl;
+	struct cgit_ttl cache_root_ttl;
+	struct cgit_ttl cache_repo_ttl;
+	struct cgit_ttl cache_about_ttl;
+	struct cgit_ttl cache_static_ttl;
+	struct cgit_ttl cache_dynamic_ttl;
+	struct cgit_ttl cache_snapshot_ttl;
 	int case_sensitive_sort;
 	int embedded;
 	int enable_filter_overrides;
@@ -295,6 +300,7 @@ struct cgit_environment {
 	const char *server_port;
 	const char *http_cookie;
 	const char *http_referer;
+	const char *http_cache_control;
 	unsigned int content_length;
 	int authenticated;
 };
diff --git a/ui-shared.c b/ui-shared.c
index 9a38aa9..2ab2fd9 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -789,7 +789,7 @@ void cgit_print_docend(void)
 void cgit_print_error_page(int code, const char *msg, const char *fmt, ...)
 {
 	va_list ap;
-	ctx.page.expires = ctx.cfg.cache_dynamic_ttl;
+	ctx.page.expires = ctx.cfg.cache_dynamic_ttl.normal;
 	ctx.page.status = code;
 	ctx.page.statusmsg = msg;
 	cgit_print_http_headers();
-- 
2.4.9



^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/2] ttl: Consolidate TTL calculation
  2016-02-26 20:57 ` [PATCH 1/2] ttl: Consolidate TTL calculation tim.nordell
@ 2016-02-28 12:22   ` john
  0 siblings, 0 replies; 6+ messages in thread
From: john @ 2016-02-28 12:22 UTC (permalink / raw)


On Fri, Feb 26, 2016 at 02:57:08PM -0600, Tim Nordell wrote:
> Currently part of the TTL calculation is done outside of the
> calc_ttl() function.  Ideally a single variable in main()
> would have the total seconds until the cache expires, and
> ctx.page.expires would be incremented by this, as well
> as the parameter to cache_process getting this value in
> seconds.

I think this needs to be split into two steps.  It would be much simpler
to review if the change to use seconds for the "ttl" variable was in its
own patch.  Having said that...

> Signed-off-by: Tim Nordell <tim.nordell at logicpd.com>
> 
> diff --git a/cache.c b/cache.c
> index 6736a01..417f1a2 100644
> --- a/cache.c
> +++ b/cache.c
> @@ -126,7 +126,7 @@ static int is_expired(struct cache_slot *slot)
>  	if (slot->ttl < 0)
>  		return 0;
>  	else
> -		return slot->cache_st.st_mtime + slot->ttl * 60 < time(NULL);
> +		return slot->cache_st.st_mtime + slot->ttl < time(NULL);
>  }
>  
>  /* Check if the slot has been modified since we opened it.
> diff --git a/cgit.c b/cgit.c
> index fc482be..963aee8 100644
> --- a/cgit.c
> +++ b/cgit.c
> @@ -1005,25 +1005,28 @@ static void cgit_parse_args(int argc, const char **argv)
>  
>  static int calc_ttl(void)
>  {
> -	if (!ctx.repo)
> -		return ctx.cfg.cache_root_ttl;
> -
> -	if (!ctx.qry.page)
> -		return ctx.cfg.cache_repo_ttl;
> -
> -	if (!strcmp(ctx.qry.page, "about"))
> -		return ctx.cfg.cache_about_ttl;
> -
> -	if (!strcmp(ctx.qry.page, "snapshot"))
> -		return ctx.cfg.cache_snapshot_ttl;
> +	const int ttl_conversion_multiplier = 60;
> +	int ttl;
>  
> -	if (ctx.qry.has_sha1)
> -		return ctx.cfg.cache_static_ttl;
> +	if (!ctx.repo)
> +		ttl = ctx.cfg.cache_root_ttl;
> +	else if (!ctx.qry.page)
> +		ttl = ctx.cfg.cache_repo_ttl;
> +	else if (!strcmp(ctx.qry.page, "about"))
> +		ttl = ctx.cfg.cache_about_ttl;
> +	else if (!strcmp(ctx.qry.page, "snapshot"))
> +		ttl = ctx.cfg.cache_snapshot_ttl;
> +	else if (ctx.qry.has_sha1)
> +		ttl = ctx.cfg.cache_static_ttl;
> +	else if (ctx.qry.has_symref)
> +		ttl = ctx.cfg.cache_dynamic_ttl;
> +	else
> +		ttl = ctx.cfg.cache_repo_ttl;

I find this harder to read than the original code.  It seems that the
whole point of this function is to allow us to encapsulate the lookup
logic.  I can accept an argument that "calc_ttl" is a bad name for that,
but this change ignores the entire point of having this as a separate
function.

This change also changes the behaviour if any of the above ttl values
is negative, which breaks the behaviour documented in cgitrc.5.txt.

> -	if (ctx.qry.has_symref)
> -		return ctx.cfg.cache_dynamic_ttl;
> +	if (ttl >= 0)
> +		return ttl * ttl_conversion_multiplier;
>  
> -	return ctx.cfg.cache_repo_ttl;
> +	return 10 * 365 * 24 * 60 * 60; /* 10 years */
>  }
>  
>  int main(int argc, const char **argv)
> @@ -1076,10 +1079,7 @@ int main(int argc, const char **argv)
>  	authenticate_cookie();
>  
>  	ttl = calc_ttl();
> -	if (ttl < 0)
> -		ctx.page.expires += 10 * 365 * 24 * 60 * 60; /* 10 years */
> -	else
> -		ctx.page.expires += ttl * 60;
> +	ctx.page.expires += ttl;
>  	if (!ctx.env.authenticated || (ctx.env.request_method && !strcmp(ctx.env.request_method, "HEAD")))
>  		ctx.cfg.nocache = 1;
>  	if (ctx.cfg.nocache)
> -- 
> 2.4.9
> 
> _______________________________________________
> CGit mailing list
> CGit at lists.zx2c4.com
> http://lists.zx2c4.com/mailman/listinfo/cgit


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 2/2] ttl: Support different TTL times based on cache-control
  2016-02-26 20:57 ` [PATCH 2/2] ttl: Support different TTL times based on cache-control tim.nordell
@ 2016-02-28 12:32   ` john
  2016-02-29 15:08     ` tim.nordell
  0 siblings, 1 reply; 6+ messages in thread
From: john @ 2016-02-28 12:32 UTC (permalink / raw)


On Fri, Feb 26, 2016 at 02:57:09PM -0600, Tim Nordell wrote:
> Allow the client browser to pass in "max-age=0" and "no-cache"
> to control a separate TTL time on the server for each type of
> page.  This extends the TTL field to have the additional form
> of:
> 
>     some-ttl=5:1
> 
> where 5 is the number of minutes if the user were to simply
> load the page, and 1 is the number of minutes if the user were
> to reload the page in their web browser (hit refresh).

Firefox seems to always send "max-age=0" when it has its own cached data
(along with an If-Modified-Since header).  I don't think we want to use
the forced value when the user's browser has an existing cached value.

Overall I'm not convinced CGit should be dealing with this, for advanced
caching isn't it better to stick Varnish or nginx in front of CGit.

> Signed-off-by: Tim Nordell <tim.nordell at logicpd.com>
> 
> diff --git a/cgit.c b/cgit.c
> index 963aee8..0ca1baa 100644
> --- a/cgit.c
> +++ b/cgit.c
> @@ -29,6 +29,30 @@ static void add_mimetype(const char *name, const char *value)
>  
>  static void process_cached_repolist(const char *path);
>  
> +/**
> + * \brief Convert TTL field into a TTL structure
> + *
> + * \param value Value from field; should be one of the following forms:
> + *                      [normal]
> + *                      [normal]:[forced]
> + * \return Parsed TTL value
> + */
> +static struct cgit_ttl ttl_value(const char *value)
> +{
> +	char *param2;
> +	struct cgit_ttl ret;
> +
> +	ret.normal = atoi(value);
> +
> +	param2 = strchr(value, ':');
> +	if(NULL != param2)
> +		ret.forced = atoi(&param2[1]);
> +	else
> +		ret.forced = -1;
> +
> +	return ret;
> +}
> +
>  static void repo_config(struct cgit_repo *repo, const char *name, const char *value)
>  {
>  	struct string_list_item *item;
> @@ -187,19 +211,19 @@ static void config_cb(const char *name, const char *value)
>  	else if (!strcmp(name, "cache-root"))
>  		ctx.cfg.cache_root = xstrdup(expand_macros(value));
>  	else if (!strcmp(name, "cache-root-ttl"))
> -		ctx.cfg.cache_root_ttl = atoi(value);
> +		ctx.cfg.cache_root_ttl = ttl_value(value);
>  	else if (!strcmp(name, "cache-repo-ttl"))
> -		ctx.cfg.cache_repo_ttl = atoi(value);
> +		ctx.cfg.cache_repo_ttl = ttl_value(value);
>  	else if (!strcmp(name, "cache-scanrc-ttl"))
>  		ctx.cfg.cache_scanrc_ttl = atoi(value);
>  	else if (!strcmp(name, "cache-static-ttl"))
> -		ctx.cfg.cache_static_ttl = atoi(value);
> +		ctx.cfg.cache_static_ttl = ttl_value(value);
>  	else if (!strcmp(name, "cache-dynamic-ttl"))
> -		ctx.cfg.cache_dynamic_ttl = atoi(value);
> +		ctx.cfg.cache_dynamic_ttl = ttl_value(value);
>  	else if (!strcmp(name, "cache-about-ttl"))
> -		ctx.cfg.cache_about_ttl = atoi(value);
> +		ctx.cfg.cache_about_ttl = ttl_value(value);
>  	else if (!strcmp(name, "cache-snapshot-ttl"))
> -		ctx.cfg.cache_snapshot_ttl = atoi(value);
> +		ctx.cfg.cache_snapshot_ttl = ttl_value(value);
>  	else if (!strcmp(name, "case-sensitive-sort"))
>  		ctx.cfg.case_sensitive_sort = atoi(value);
>  	else if (!strcmp(name, "about-filter"))
> @@ -352,13 +376,19 @@ static void prepare_context(void)
>  	ctx.cfg.cache_size = 0;
>  	ctx.cfg.cache_max_create_time = 5;
>  	ctx.cfg.cache_root = CGIT_CACHE_ROOT;
> -	ctx.cfg.cache_about_ttl = 15;
> -	ctx.cfg.cache_snapshot_ttl = 5;
> -	ctx.cfg.cache_repo_ttl = 5;
> -	ctx.cfg.cache_root_ttl = 5;
> +	ctx.cfg.cache_about_ttl.normal = 15;
> +	ctx.cfg.cache_about_ttl.forced = -1;
> +	ctx.cfg.cache_snapshot_ttl.normal = 5;
> +	ctx.cfg.cache_snapshot_ttl.forced = -1;
> +	ctx.cfg.cache_repo_ttl.normal = 5;
> +	ctx.cfg.cache_repo_ttl.forced = -1;
> +	ctx.cfg.cache_root_ttl.normal = 5;
> +	ctx.cfg.cache_root_ttl.forced = -1;
>  	ctx.cfg.cache_scanrc_ttl = 15;
> -	ctx.cfg.cache_dynamic_ttl = 5;
> -	ctx.cfg.cache_static_ttl = -1;
> +	ctx.cfg.cache_dynamic_ttl.normal = 5;
> +	ctx.cfg.cache_dynamic_ttl.forced = -1;
> +	ctx.cfg.cache_static_ttl.normal = -1;
> +	ctx.cfg.cache_static_ttl.forced = -1;
>  	ctx.cfg.case_sensitive_sort = 1;
>  	ctx.cfg.branch_sort = 0;
>  	ctx.cfg.commit_sort = 0;
> @@ -405,6 +435,7 @@ static void prepare_context(void)
>  	ctx.env.server_port = getenv("SERVER_PORT");
>  	ctx.env.http_cookie = getenv("HTTP_COOKIE");
>  	ctx.env.http_referer = getenv("HTTP_REFERER");
> +	ctx.env.http_cache_control = getenv("HTTP_CACHE_CONTROL");
>  	ctx.env.content_length = getenv("CONTENT_LENGTH") ? strtoul(getenv("CONTENT_LENGTH"), NULL, 10) : 0;
>  	ctx.env.authenticated = 0;
>  	ctx.page.mimetype = "text/html";
> @@ -1006,7 +1037,7 @@ static void cgit_parse_args(int argc, const char **argv)
>  static int calc_ttl(void)
>  {
>  	const int ttl_conversion_multiplier = 60;
> -	int ttl;
> +	struct cgit_ttl ttl;
>  
>  	if (!ctx.repo)
>  		ttl = ctx.cfg.cache_root_ttl;
> @@ -1023,8 +1054,14 @@ static int calc_ttl(void)
>  	else
>  		ttl = ctx.cfg.cache_repo_ttl;
>  
> -	if (ttl >= 0)
> -		return ttl * ttl_conversion_multiplier;
> +	if(ttl.forced >= 0 && NULL != ctx.env.http_cache_control &&
> +	    (!strcmp(ctx.env.http_cache_control, "max-age=0") ||
> +	     !strcmp(ctx.env.http_cache_control, "no-cache"))
> +	  )
> +		return ttl.forced * ttl_conversion_multiplier;
> +
> +	if (ttl.normal >= 0)
> +		return ttl.normal * ttl_conversion_multiplier;
>  
>  	return 10 * 365 * 24 * 60 * 60; /* 10 years */
>  }
> diff --git a/cgit.h b/cgit.h
> index 325432b..350d25d 100644
> --- a/cgit.h
> +++ b/cgit.h
> @@ -185,6 +185,11 @@ struct cgit_query {
>  	char *vpath;
>  };
>  
> +struct cgit_ttl {
> +	int normal;
> +	int forced;
> +};
> +
>  struct cgit_config {
>  	char *agefile;
>  	char *cache_root;
> @@ -213,14 +218,14 @@ struct cgit_config {
>  	char *virtual_root;	/* Always ends with '/'. */
>  	char *strict_export;
>  	int cache_size;
> -	int cache_dynamic_ttl;
>  	int cache_max_create_time;
> -	int cache_repo_ttl;
> -	int cache_root_ttl;
>  	int cache_scanrc_ttl;
> -	int cache_static_ttl;
> -	int cache_about_ttl;
> -	int cache_snapshot_ttl;
> +	struct cgit_ttl cache_root_ttl;
> +	struct cgit_ttl cache_repo_ttl;
> +	struct cgit_ttl cache_about_ttl;
> +	struct cgit_ttl cache_static_ttl;
> +	struct cgit_ttl cache_dynamic_ttl;
> +	struct cgit_ttl cache_snapshot_ttl;
>  	int case_sensitive_sort;
>  	int embedded;
>  	int enable_filter_overrides;
> @@ -295,6 +300,7 @@ struct cgit_environment {
>  	const char *server_port;
>  	const char *http_cookie;
>  	const char *http_referer;
> +	const char *http_cache_control;
>  	unsigned int content_length;
>  	int authenticated;
>  };
> diff --git a/ui-shared.c b/ui-shared.c
> index 9a38aa9..2ab2fd9 100644
> --- a/ui-shared.c
> +++ b/ui-shared.c
> @@ -789,7 +789,7 @@ void cgit_print_docend(void)
>  void cgit_print_error_page(int code, const char *msg, const char *fmt, ...)
>  {
>  	va_list ap;
> -	ctx.page.expires = ctx.cfg.cache_dynamic_ttl;
> +	ctx.page.expires = ctx.cfg.cache_dynamic_ttl.normal;
>  	ctx.page.status = code;
>  	ctx.page.statusmsg = msg;
>  	cgit_print_http_headers();
> -- 
> 2.4.9
> 
> _______________________________________________
> CGit mailing list
> CGit at lists.zx2c4.com
> http://lists.zx2c4.com/mailman/listinfo/cgit


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 2/2] ttl: Support different TTL times based on cache-control
  2016-02-28 12:32   ` john
@ 2016-02-29 15:08     ` tim.nordell
  0 siblings, 0 replies; 6+ messages in thread
From: tim.nordell @ 2016-02-29 15:08 UTC (permalink / raw)


On 02/28/16 06:32, John Keeping wrote:
> On Fri, Feb 26, 2016 at 02:57:09PM -0600, Tim Nordell wrote:
>> Allow the client browser to pass in "max-age=0" and "no-cache"
>> to control a separate TTL time on the server for each type of
>> page.  This extends the TTL field to have the additional form
>> of:
>>
>>      some-ttl=5:1
>>
>> where 5 is the number of minutes if the user were to simply
>> load the page, and 1 is the number of minutes if the user were
>> to reload the page in their web browser (hit refresh).
>
> Firefox seems to always send "max-age=0" when it has its own cached data
> (along with an If-Modified-Since header).  I don't think we want to use
> the forced value when the user's browser has an existing cached value.
>

Yes, although, for a new hit (as in not in your local cache yet), 
Firefox will not send "max-age=0".  It will only do this if you hit 
refresh on the page.  If you hold shift while clicking refresh, it'll 
change to "no-cache".

> Overall I'm not convinced CGit should be dealing with this, for advanced
> caching isn't it better to stick Varnish or nginx in front of CGit.

Maybe.  The use case I have is that I want to serve the cached value by 
default, but allow a user to force it to be refreshed on an earlier 
timetable than otherwise permitted.  However, I still want this time to 
be restricted so that one can't cause the server to /always/ generate a 
new page.

Anyone else thoughts?  I'm okay dropping this patchset for the main 
codebase if noone else is interested in this.  I'm just interested in 
not maintaining our own patchsets and would like to get our changes in 
the main codebase if possible.  (This one is low on the priority list 
anyways since it doesn't affect what is displayed - only caching behavior.)

- Tim


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2016-02-29 15:08 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-26 20:57 [PATCH 0/2] TTL updates tim.nordell
2016-02-26 20:57 ` [PATCH 1/2] ttl: Consolidate TTL calculation tim.nordell
2016-02-28 12:22   ` john
2016-02-26 20:57 ` [PATCH 2/2] ttl: Support different TTL times based on cache-control tim.nordell
2016-02-28 12:32   ` john
2016-02-29 15:08     ` tim.nordell

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).