[PATCH 1/2] ttl: Consolidate TTL calculation

[John Keeping]

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