[PATCH 0/2] Add an option to cache snapshots

[PATCH 0/2] Add an option to cache snapshots

[cgit]

It seems strange that one can only enable caching of both HTML pages and
snapshots or disable it for both. This patch series addresses this
shortcoming and allows for more fine-grained control over what is
cached.

I find this quite useful on my cgit setups where I usually do not want
to enable caching of HTML pages.

Lukas Fleischer (2):
  Skip cache slot when time-to-live is zero
  Add a cache-snapshot-ttl configuration variable

 cache.c      | 2 +-
 cgit.c       | 6 ++++++
 cgit.h       | 1 +
 cgitrc.5.txt | 5 +++++
 4 files changed, 13 insertions(+), 1 deletion(-)

-- 
1.8.5.3

[PATCH 1/2] Skip cache slot when time-to-live is zero

[cgit]

If time-to-live is set to zero, we don't need to regenerate the cache
slots on every request. Instead, just skip the caching process and
immediately provide the dynamically generated version of the page.
Setting time-to-live to zero is useful when you want to disable caching
for certain pages.

Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
---
 cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cache.c b/cache.c
index 9e7eeb0..801e63f 100644
--- a/cache.c
+++ b/cache.c
@@ -343,7 +343,7 @@ int cache_process(int size, const char *path, const char *key, int ttl,
 	int result;
 
 	/* If the cache is disabled, just generate the content */
-	if (size <= 0) {
+	if (size <= 0 || ttl == 0) {
 		fn();
 		return 0;
 	}
-- 
1.8.5.3

[PATCH 2/2] Add a cache-snapshot-ttl configuration variable

[cgit]

This can be used to specify the TTL for snapshots. Snapshots are usually
static and do not ever change. On the other hand, tarball generation is
CPU intensive.

One use case of this setting (apart from increasing the lifetime of
snapshot cache slots) is caching of snapshots while disabling the cache
for static/dynamic HTML pages (by setting TTL to zero for everything
except for snapshot requests).

Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
---
 cgit.c       | 6 ++++++
 cgit.h       | 1 +
 cgitrc.5.txt | 5 +++++
 3 files changed, 12 insertions(+)

diff --git a/cgit.c b/cgit.c
index 36251e7..1ca5735 100644
--- a/cgit.c
+++ b/cgit.c
@@ -184,6 +184,8 @@ static void config_cb(const char *name, const char *value)
 		ctx.cfg.cache_dynamic_ttl = atoi(value);
 	else if (!strcmp(name, "cache-about-ttl"))
 		ctx.cfg.cache_about_ttl = atoi(value);
+	else if (!strcmp(name, "cache-snapshot-ttl"))
+		ctx.cfg.cache_snapshot_ttl = atoi(value);
 	else if (!strcmp(name, "case-sensitive-sort"))
 		ctx.cfg.case_sensitive_sort = atoi(value);
 	else if (!strcmp(name, "about-filter"))
@@ -331,6 +333,7 @@ static void prepare_context(void)
 	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 = -1;
 	ctx.cfg.cache_repo_ttl = 5;
 	ctx.cfg.cache_root_ttl = 5;
 	ctx.cfg.cache_scanrc_ttl = 15;
@@ -995,6 +998,9 @@ static int calc_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;
+
 	if (ctx.qry.has_sha1)
 		return ctx.cfg.cache_static_ttl;
 
diff --git a/cgit.h b/cgit.h
index 496d0f6..0badc64 100644
--- a/cgit.h
+++ b/cgit.h
@@ -210,6 +210,7 @@ struct cgit_config {
 	int cache_scanrc_ttl;
 	int cache_static_ttl;
 	int cache_about_ttl;
+	int cache_snapshot_ttl;
 	int case_sensitive_sort;
 	int embedded;
 	int enable_filter_overrides;
diff --git a/cgitrc.5.txt b/cgitrc.5.txt
index 8eafc4a..33929f5 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -88,6 +88,11 @@ cache-about-ttl::
 	version of the repository about page. Negative values have infinite
 	ttl. Default value: "15".
 
+cache-snapshot-ttl::
+	Number which specifies the time-to-live, in minutes, for the cached
+	version of snapshots. Negative values have infinite ttl. Default
+	value: "-1".
+
 cache-size::
 	The maximum number of entries in the cgit cache. Default value: "0"
 	(i.e. caching is disabled).
-- 
1.8.5.3

[PATCH 2/2] Add a cache-snapshot-ttl configuration variable

[Jason]

On Wed, Feb 5, 2014 at 10:46 AM, Lukas Fleischer <cgit at cryptocrack.de> wrote:
> This can be used to specify the TTL for snapshots. Snapshots are usually
> static and do not ever change. On the other hand, tarball generation is
> CPU intensive.
>
> One use case of this setting (apart from increasing the lifetime of
> snapshot cache slots) is caching of snapshots while disabling the cache
> for static/dynamic HTML pages (by setting TTL to zero for everything
> except for snapshot requests).
>

I like this idea. But I don't think the default should be -1, since
that significantly changes the current behavior, and makes certain
tarballs get out of date perpetually, like downloading a zip of HEAD
or what happens when history is rewritten.

Currently it defaults to cache_repo_ttl, which is 5. I'd be willing to
compromise for 15 if you prefer.

[PATCH 2/2] Add a cache-snapshot-ttl configuration variable

[cgit]

On Wed, 05 Feb 2014 at 15:44:36, Jason A. Donenfeld wrote:
> On Wed, Feb 5, 2014 at 10:46 AM, Lukas Fleischer <cgit at cryptocrack.de> wrote:
> > This can be used to specify the TTL for snapshots. Snapshots are usually
> > static and do not ever change. On the other hand, tarball generation is
> > CPU intensive.
> >
> > One use case of this setting (apart from increasing the lifetime of
> > snapshot cache slots) is caching of snapshots while disabling the cache
> > for static/dynamic HTML pages (by setting TTL to zero for everything
> > except for snapshot requests).
> >
> 
> I like this idea. But I don't think the default should be -1, since
> that significantly changes the current behavior, and makes certain
> tarballs get out of date perpetually, like downloading a zip of HEAD
> or what happens when history is rewritten.

Right, I didn't think of that. Only thought of the main use case which
is downloading tagged snapshots. These are unlikely to change. I really
wish we had some kind of intelligent caching as described in [1]. Using
that, we could unquestionably use -1 as default everywhere.

> 
> Currently it defaults to cache_repo_ttl, which is 5. I'd be willing to
> compromise for 15 if you prefer.

I'm fine with either one. Whatever you prefer.

[1] http://lists.zx2c4.com/pipermail/cgit/2014-February/001974.html

[PATCH 1/2] Skip cache slot when time-to-live is zero

[Jason]

On Wed, Feb 5, 2014 at 10:46 AM, Lukas Fleischer <cgit at cryptocrack.de>wrote:
>
>         /* If the cache is disabled, just generate the content */
> -       if (size <= 0) {
> +       if (size <= 0 || ttl == 0) {
>                 fn();
>                 return 0;
>         }


Apparently we already special case ttl for < 0:

/* Check if the slot has expired */
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);
}

What should our behavior be for consistency?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20140206/82d4383c/attachment.html>

[PATCH 1/2] Skip cache slot when time-to-live is zero

[cgit]

On Thu, 06 Feb 2014 at 20:52:46, Jason A. Donenfeld wrote:
> On Wed, Feb 5, 2014 at 10:46 AM, Lukas Fleischer <cgit at cryptocrack.de>wrote:
> >
> >         /* If the cache is disabled, just generate the content */
> > -       if (size <= 0) {
> > +       if (size <= 0 || ttl == 0) {
> >                 fn();
> >                 return 0;
> >         }
> 
> 
> Apparently we already special case ttl for < 0:
> 
> /* Check if the slot has expired */
> 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);
> }
> 
> What should our behavior be for consistency?

This is different. -1 means "never expire". 0 means "always expire". We
cannot add the 0 special case to is_expired(), though, because that
would mean we would update and write the cache file to disk on every
request which seems to be a bad idea (actually, that is what already
happens now without having a special case).

[PATCH 2/2] Add a cache-snapshot-ttl configuration variable

[cgit]

On Wed, 05 Feb 2014 at 16:09:14, Lukas Fleischer wrote:
> On Wed, 05 Feb 2014 at 15:44:36, Jason A. Donenfeld wrote:
> [...]
> > Currently it defaults to cache_repo_ttl, which is 5. I'd be willing to
> > compromise for 15 if you prefer.
> 
> I'm fine with either one. Whatever you prefer.

Changed the default value to 5 in lf/staging.

[PATCH 1/2] Skip cache slot when time-to-live is zero

[Jason]

> On Feb 6, 2014 10:07 PM, "Lukas Fleischer" <cgit at cryptocrack.de> wrote:
>
> This is different. -1 means "never expire". 0 means "always expire".

Ahh perfect -- this is exactly the type of distinction I was looking for.
Do we have this documented?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20140208/e4f7c784/attachment.html>

[PATCH 1/2] Skip cache slot when time-to-live is zero

[cgit]

On Sat, 08 Feb 2014 at 14:41:56, Jason A. Donenfeld wrote:
> > On Feb 6, 2014 10:07 PM, "Lukas Fleischer" <cgit at cryptocrack.de> wrote:
> >
> > This is different. -1 means "never expire". 0 means "always expire".
> 
> Ahh perfect -- this is exactly the type of distinction I was looking for.
> Do we have this documented?

Yes, -1 is documented in the man page and it is quite clear that 0 means
"always expire" (directly follows from the general description of what
the *-ttl variables do).

[PATCH 2/2] Add a cache-snapshot-ttl configuration variable

[Jason]

Merged, thanks.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20140220/d2b002b8/attachment.html>

[PATCH 1/2] Skip cache slot when time-to-live is zero

[Jason]

On Sat, Feb 8, 2014 at 2:45 PM, Lukas Fleischer <cgit at cryptocrack.de> wrote:
>
> Yes, -1 is documented in the man page and it is quite clear that 0 means
> "always expire" (directly follows from the general description of what
> the *-ttl variables do).


I'd like to explicitly have this documented before merging it. Would you
mind resubmitting this patch with explicit documentation that says zero
means "never cache"?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20140220/c3a9177a/attachment.html>

[PATCH] Skip cache slot when time-to-live is zero

[cgit]

If time-to-live is set to zero, we don't need to regenerate the cache
slots on every request. Instead, just skip the caching process and
immediately provide the dynamically generated version of the page.
Setting time-to-live to zero is useful when you want to disable caching
for certain pages.

Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
---
 cache.c      |  2 +-
 cgitrc.5.txt | 22 ++++++++++++++--------
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/cache.c b/cache.c
index 9e7eeb0..801e63f 100644
--- a/cache.c
+++ b/cache.c
@@ -343,7 +343,7 @@ int cache_process(int size, const char *path, const char *key, int ttl,
 	int result;
 
 	/* If the cache is disabled, just generate the content */
-	if (size <= 0) {
+	if (size <= 0 || ttl == 0) {
 		fn();
 		return 0;
 	}
diff --git a/cgitrc.5.txt b/cgitrc.5.txt
index a437fc4..7158c10 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -61,37 +61,43 @@ cache-root::
 cache-static-ttl::
 	Number which specifies the time-to-live, in minutes, for the cached
 	version of repository pages accessed with a fixed SHA1. Negative
-	values have infinite ttl. Default value: -1".
+	values have infinite ttl, zero means that the cache is disabled for
+	this type of pages. Default value: -1".
 
 cache-dynamic-ttl::
 	Number which specifies the time-to-live, in minutes, for the cached
 	version of repository pages accessed without a fixed SHA1. Negative
-	values have infinite ttl. Default value: "5".
+	values have infinite ttl, zero means that the cache is disabled for this
+	type of pages. Default value: "5".
 
 cache-repo-ttl::
 	Number which specifies the time-to-live, in minutes, for the cached
 	version of the repository summary page. Negative values have infinite
-	ttl. Default value: "5".
+	ttl, zero means that the cache is disabled for this type of pages.
+	Default value: "5".
 
 cache-root-ttl::
 	Number which specifies the time-to-live, in minutes, for the cached
 	version of the repository index page. Negative values have infinite
-	ttl. Default value: "5".
+	ttl, zero means that the cache is disabled for this type of pages.
+	Default value: "5".
 
 cache-scanrc-ttl::
 	Number which specifies the time-to-live, in minutes, for the result
 	of scanning a path for git repositories. Negative values have infinite
-	ttl. Default value: "15".
+	ttl, zero means that the cache is disable for this type of pages.
+	Default value: "15".
 
 cache-about-ttl::
 	Number which specifies the time-to-live, in minutes, for the cached
 	version of the repository about page. Negative values have infinite
-	ttl. Default value: "15".
+	ttl, zero means that the cache is disable for this type of pages.
+	Default value: "15".
 
 cache-snapshot-ttl::
 	Number which specifies the time-to-live, in minutes, for the cached
-	version of snapshots. Negative values have infinite ttl. Default
-	value: "5".
+	version of snapshots. Negative values have infinite ttl, zero means
+	that the cache is disable for this type of pages. Default value: "5".
 
 cache-size::
 	The maximum number of entries in the cgit cache. Default value: "0"
-- 
1.9.0

[PATCH] Skip cache slot when time-to-live is zero

[Jason]

s/this type of pages/this type of page/g

--
Sent from my telephone.
 On Feb 20, 2014 8:59 PM, "Lukas Fleischer" <cgit at cryptocrack.de> wrote:

> If time-to-live is set to zero, we don't need to regenerate the cache
> slots on every request. Instead, just skip the caching process and
> immediately provide the dynamically generated version of the page.
> Setting time-to-live to zero is useful when you want to disable caching
> for certain pages.
>
> Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
> ---
>  cache.c      |  2 +-
>  cgitrc.5.txt | 22 ++++++++++++++--------
>  2 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/cache.c b/cache.c
> index 9e7eeb0..801e63f 100644
> --- a/cache.c
> +++ b/cache.c
> @@ -343,7 +343,7 @@ int cache_process(int size, const char *path, const
> char *key, int ttl,
>         int result;
>
>         /* If the cache is disabled, just generate the content */
> -       if (size <= 0) {
> +       if (size <= 0 || ttl == 0) {
>                 fn();
>                 return 0;
>         }
> diff --git a/cgitrc.5.txt b/cgitrc.5.txt
> index a437fc4..7158c10 100644
> --- a/cgitrc.5.txt
> +++ b/cgitrc.5.txt
> @@ -61,37 +61,43 @@ cache-root::
>  cache-static-ttl::
>         Number which specifies the time-to-live, in minutes, for the cached
>         version of repository pages accessed with a fixed SHA1. Negative
> -       values have infinite ttl. Default value: -1".
> +       values have infinite ttl, zero means that the cache is disabled for
> +       this type of pages. Default value: -1".
>
>  cache-dynamic-ttl::
>         Number which specifies the time-to-live, in minutes, for the cached
>         version of repository pages accessed without a fixed SHA1. Negative
> -       values have infinite ttl. Default value: "5".
> +       values have infinite ttl, zero means that the cache is disabled
> for this
> +       type of pages. Default value: "5".
>
>  cache-repo-ttl::
>         Number which specifies the time-to-live, in minutes, for the cached
>         version of the repository summary page. Negative values have
> infinite
> -       ttl. Default value: "5".
> +       ttl, zero means that the cache is disabled for this type of pages.
> +       Default value: "5".
>
>  cache-root-ttl::
>         Number which specifies the time-to-live, in minutes, for the cached
>         version of the repository index page. Negative values have infinite
> -       ttl. Default value: "5".
> +       ttl, zero means that the cache is disabled for this type of pages.
> +       Default value: "5".
>
>  cache-scanrc-ttl::
>         Number which specifies the time-to-live, in minutes, for the result
>         of scanning a path for git repositories. Negative values have
> infinite
> -       ttl. Default value: "15".
> +       ttl, zero means that the cache is disable for this type of pages.
> +       Default value: "15".
>
>  cache-about-ttl::
>         Number which specifies the time-to-live, in minutes, for the cached
>         version of the repository about page. Negative values have infinite
> -       ttl. Default value: "15".
> +       ttl, zero means that the cache is disable for this type of pages.
> +       Default value: "15".
>
>  cache-snapshot-ttl::
>         Number which specifies the time-to-live, in minutes, for the cached
> -       version of snapshots. Negative values have infinite ttl. Default
> -       value: "5".
> +       version of snapshots. Negative values have infinite ttl, zero means
> +       that the cache is disable for this type of pages. Default value:
> "5".
>
>  cache-size::
>         The maximum number of entries in the cgit cache. Default value: "0"
> --
> 1.9.0
>
> _______________________________________________
> CGit mailing list
> CGit at lists.zx2c4.com
> http://lists.zx2c4.com/mailman/listinfo/cgit
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20140220/8a49b82d/attachment-0001.html>

[PATCH] Skip cache slot when time-to-live is zero

[cgit]

On Thu, 20 Feb 2014 at 20:59:22, Lukas Fleischer wrote:
> [...]
> diff --git a/cgitrc.5.txt b/cgitrc.5.txt
> index a437fc4..7158c10 100644
> --- a/cgitrc.5.txt
> +++ b/cgitrc.5.txt
> @@ -61,37 +61,43 @@ cache-root::
>  cache-static-ttl::
>         Number which specifies the time-to-live, in minutes, for the cached
>         version of repository pages accessed with a fixed SHA1. Negative
> -       values have infinite ttl. Default value: -1".
> +       values have infinite ttl, zero means that the cache is disabled for
> +       this type of pages. Default value: -1".

Suggestions for better wording welcome, by the way. I also think that
the already existing text "Negative values have infinite ttl" sounds a
bit strange. Opinions?

> [...]

[PATCH] Skip cache slot when time-to-live is zero

[Jason]

On Thu, Feb 20, 2014 at 9:07 PM, Lukas Fleischer <cgit at cryptocrack.de> wrote:
> Suggestions for better wording welcome, by the way. I also think that
> the already existing text "Negative values have infinite ttl" sounds a
> bit strange. Opinions?

Fixed the docs up with an additional commit, and committed yours below it.