From mboxrd@z Thu Jan  1 00:00:00 1970
From: john at keeping.me.uk (John Keeping)
Date: Thu, 22 Aug 2013 20:19:22 +0100
Subject: [PATCH] Display the most recent modtime for the repolist
In-Reply-To: <1377198333-26798-1-git-send-email-fparent@baylibre.com>
References: <1377198333-26798-1-git-send-email-fparent@baylibre.com>
Message-ID: <20130822191922.GY2337@serenity.lan>

On Thu, Aug 22, 2013 at 09:05:33PM +0200, Fabien Parent wrote:
> Instead of using the modtime of the master branch cgit now uses the modtime
> of the lastest ref updated.
> ---
>  ui-repolist.c | 32 +++++++++++++++++++++++++++++---
>  1 file changed, 29 insertions(+), 3 deletions(-)
> 
>
> diff --git a/ui-repolist.c b/ui-repolist.c
> index 2ab6e9e..dcd74dc 100644
> --- a/ui-repolist.c
> +++ b/ui-repolist.c
> @@ -13,6 +13,11 @@
>  #include "ui-shared.h"
>  #include <strings.h>
>  
> +struct ref_mod_time {
> +	struct cgit_repo *repo;
> +	time_t mod_time;
> +};
> +
>  static time_t read_agefile(char *path)
>  {
>  	time_t result;
> @@ -74,11 +79,32 @@ end:
>  	return (r->mtime != 0);
>  }
>  
> +static int get_ref_mod_time(const char *refname, const unsigned char *sha1,
> +			    int flags, void *cb_data)
> +{
> +	struct ref_mod_time *rmt = (struct ref_mod_time*) cb_data;
> +	char *defbranch_old;
> +
> +	defbranch_old = rmt->repo->defbranch;
> +	rmt->repo->defbranch = (char*) refname;
> +	get_repo_modtime(rmt->repo, &rmt->mod_time);

By doing this for each ref, don't we end up with repo->modtime pointing
at which ever branch is processed last?  There doesn't seem to be an
attempt to update it below.

Also this doesn't change the behaviour to what people normally want - by
sticking with get_repo_modtime you're still just stat'ing the ref file
(assuming it's not packed).  For repositories that spend a long time
idle, a GC or other operation may well change the mtime of the file
without any actual change to the repository.

> +	rmt->repo->defbranch = defbranch_old;
> +	return 0;
> +}
> +
>  static void print_modtime(struct cgit_repo *repo)
>  {
> -	time_t t;
> -	if (get_repo_modtime(repo, &t))
> -		cgit_print_age(t, -1, NULL);
> +	struct ref_mod_time rmt = { .repo = repo };
> +	struct ref_mod_time last_mod_ref = {0};
> +
> +	/* Get the time of the most recent update to the repo */
> +	setenv("GIT_DIR", repo->path, 1);
> +	for_each_branch_ref(get_ref_mod_time, &rmt);

So we unconditionally examine ever ref here?  When a user's configured
an age file that will return the same value every time -
get_repo_modtime will never actually inspect the branch.

Also, by putting the change only in print_modtime this is going to
become inconsistent with the sort order when get_repo_modtime is used.

If we want to do a change like this, then it should be contained in
get_repo_modtime (probably with a new helper function, but it shouldn't
need to touch any other functions here).  We probably also want it
controlled by a config variable to let users with a lot of repositories
and refs avoid paying the cost of examining all of them.

> +	if (rmt.mod_time > last_mod_ref.mod_time)
> +		last_mod_ref = rmt;
> +
> +	if (last_mod_ref.mod_time)
> +		cgit_print_age(last_mod_ref.mod_time, -1, NULL);
>  }
>  
>  static int is_match(struct cgit_repo *repo)
> -- 
> 1.8.4.rc1