From: Mark Jamsek <mark@jamsek.com>
Subject: Re: gotwebd: turn gotweb_get_time_str into gotweb_render_age
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Sun, 22 Jan 2023 18:37:45 +1100

On 23-01-21 07:46PM, Omar Polo wrote:
> gotweb_get_time_str was once used to allocate string to be then
> printed via fcgi_printf.  We've moved everything through template so
> (except for a single case) there's no need to dynamically allocate the
> string for later use.
> 
> Diff below then turns gotweb_get_time_str into gotweb_render_age by
> directly outputting the string.  The only usage left was in
> got_operations.c to get the repository last modification date that was
> changed to return the time_t instead.
> 
> ok?

Reads good and looks the same in manual testing, ok

> -----------------------------------------------
> commit 88a82656f65fee52fe135baccb76624ae8e1e2e4 (x)
> from: Omar Polo <op@omarpolo.com>
> date: Sat Jan 21 18:37:49 2023 UTC
>  
>  gotwebd: turn gotweb_get_time_str into gotweb_render_age
>  
>  M  gotwebd/got_operations.c  |   3+   7-
>  M  gotwebd/gotweb.c          |  30+  33-
>  M  gotwebd/gotwebd.h         |   4+   4-
>  M  gotwebd/pages.tmpl        |   9+  22-
> 
> 4 files changed, 46 insertions(+), 66 deletions(-)
> 
> diff 6cdf29f9353bcea609dcbab90131634430ac5a18 88a82656f65fee52fe135baccb76624ae8e1e2e4
> commit - 6cdf29f9353bcea609dcbab90131634430ac5a18
> commit + 88a82656f65fee52fe135baccb76624ae8e1e2e4
> blob - 50bc0de73e23d3f59b30e2eecb24a96fbc5ce5bf
> blob + 78b5a2623d4338f679a254321a0a0647a5ff85fd
> --- gotwebd/got_operations.c
> +++ gotwebd/got_operations.c
> @@ -126,8 +126,7 @@ got_get_repo_age(char **repo_age, struct request *c,
>  }
>  
>  const struct got_error *
> -got_get_repo_age(char **repo_age, struct request *c,
> -    const char *refname, int ref_tm)
> +got_get_repo_age(time_t *repo_age, struct request *c, const char *refname)
>  {
>  	const struct got_error *error = NULL;
>  	struct server *srv = c->srv;
> @@ -138,7 +137,6 @@ got_get_repo_age(char **repo_age, struct request *c,
>  	struct got_reflist_entry *re;
>  	time_t committer_time = 0, cmp_time = 0;
>  
> -	*repo_age = NULL;
>  	TAILQ_INIT(&refs);
>  
>  	if (srv->show_repo_age == 0)
> @@ -178,10 +176,8 @@ got_get_repo_age(char **repo_age, struct request *c,
>  			break;
>  	}
>  
> -	if (cmp_time != 0) {
> -		committer_time = cmp_time;
> -		error = gotweb_get_time_str(repo_age, committer_time, ref_tm);
> -	}
> +	if (cmp_time != 0)
> +		*repo_age = cmp_time;
>  done:
>  	got_ref_list_free(&refs);
>  	return error;
> blob - 47ec70cd7f1ff4721930aa83409682012b4e6d60
> blob + de3c351cbe1ced66e0a1e55b0df7afae4fe2b894
> --- gotwebd/gotweb.c
> +++ gotwebd/gotweb.c
> @@ -761,7 +761,6 @@ gotweb_free_repo_dir(struct repo_dir *repo_dir)
>  		free(repo_dir->owner);
>  		free(repo_dir->description);
>  		free(repo_dir->url);
> -		free(repo_dir->age);
>  		free(repo_dir->path);
>  	}
>  	free(repo_dir);
> @@ -1337,7 +1336,7 @@ done:
>  	error = got_get_repo_owner(&repo_dir->owner, c);
>  	if (error)
>  		goto err;
> -	error = got_get_repo_age(&repo_dir->age, c, NULL, TM_DIFF);
> +	error = got_get_repo_age(&repo_dir->age, c, NULL);
>  	if (error)
>  		goto err;
>  	error = gotweb_get_clone_url(&repo_dir->url, srv, repo_dir->path,
> @@ -1367,7 +1366,6 @@ gotweb_init_repo_dir(struct repo_dir **repo_dir, const
>  	(*repo_dir)->owner = NULL;
>  	(*repo_dir)->description = NULL;
>  	(*repo_dir)->url = NULL;
> -	(*repo_dir)->age = NULL;
>  	(*repo_dir)->path = NULL;
>  
>  	return NULL;
> @@ -1465,9 +1463,10 @@ const struct got_error *
>  	return error;
>  }
>  
> -const struct got_error *
> -gotweb_get_time_str(char **repo_age, time_t committer_time, int ref_tm)
> +int
> +gotweb_render_age(struct template *tp, time_t committer_time, int ref_tm)
>  {
> +	struct request *c = tp->tp_arg;
>  	struct tm tm;
>  	long long diff_time;
>  	const char *years = "years ago", *months = "months ago";
> @@ -1478,69 +1477,67 @@ gotweb_get_time_str(char **repo_age, time_t committer_
>  	char datebuf[64];
>  	size_t r;
>  
> -	*repo_age = NULL;
> -
>  	switch (ref_tm) {
>  	case TM_DIFF:
>  		diff_time = time(NULL) - committer_time;
>  		if (diff_time > 60 * 60 * 24 * 365 * 2) {
> -			if (asprintf(repo_age, "%lld %s",
> +			if (fcgi_printf(c, "%lld %s",
>  			    (diff_time / 60 / 60 / 24 / 365), years) == -1)
> -				return got_error_from_errno("asprintf");
> +				return -1;
>  		} else if (diff_time > 60 * 60 * 24 * (365 / 12) * 2) {
> -			if (asprintf(repo_age, "%lld %s",
> +			if (fcgi_printf(c, "%lld %s",
>  			    (diff_time / 60 / 60 / 24 / (365 / 12)),
>  			    months) == -1)
> -				return got_error_from_errno("asprintf");
> +				return -1;
>  		} else if (diff_time > 60 * 60 * 24 * 7 * 2) {
> -			if (asprintf(repo_age, "%lld %s",
> +			if (fcgi_printf(c, "%lld %s",
>  			    (diff_time / 60 / 60 / 24 / 7), weeks) == -1)
> -				return got_error_from_errno("asprintf");
> +				return -1;
>  		} else if (diff_time > 60 * 60 * 24 * 2) {
> -			if (asprintf(repo_age, "%lld %s",
> +			if (fcgi_printf(c, "%lld %s",
>  			    (diff_time / 60 / 60 / 24), days) == -1)
> -				return got_error_from_errno("asprintf");
> +				return -1;
>  		} else if (diff_time > 60 * 60 * 2) {
> -			if (asprintf(repo_age, "%lld %s",
> +			if (fcgi_printf(c, "%lld %s",
>  			    (diff_time / 60 / 60), hours) == -1)
> -				return got_error_from_errno("asprintf");
> +				return -1;
>  		} else if (diff_time > 60 * 2) {
> -			if (asprintf(repo_age, "%lld %s", (diff_time / 60),
> +			if (fcgi_printf(c, "%lld %s", (diff_time / 60),
>  			    minutes) == -1)
> -				return got_error_from_errno("asprintf");
> +				return -1;
>  		} else if (diff_time > 2) {
> -			if (asprintf(repo_age, "%lld %s", diff_time,
> +			if (fcgi_printf(c, "%lld %s", diff_time,
>  			    seconds) == -1)
> -				return got_error_from_errno("asprintf");
> +				return -1;
>  		} else {
> -			if (asprintf(repo_age, "%s", now) == -1)
> -				return got_error_from_errno("asprintf");
> +			if (fcgi_puts(tp, now) == -1)
> +				return -1;
>  		}
>  		break;
>  	case TM_LONG:
>  		if (gmtime_r(&committer_time, &tm) == NULL)
> -			return got_error_from_errno("gmtime_r");
> +			return -1;
>  
>  		s = asctime_r(&tm, datebuf);
>  		if (s == NULL)
> -			return got_error_from_errno("asctime_r");
> +			return -1;
>  
> -		if (asprintf(repo_age, "%s UTC", datebuf) == -1)
> -			return got_error_from_errno("asprintf");
> +		if (fcgi_puts(tp, datebuf) == -1 ||
> +		    fcgi_puts(tp, " UTC") == -1)
> +			return -1;
>  		break;
>  	case TM_RFC822:
>  		if (gmtime_r(&committer_time, &tm) == NULL)
> -			return got_error_from_errno("gmtime_r");
> +			return -1;
>  
>  		r = strftime(datebuf, sizeof(datebuf),
>  		    "%a, %d %b %Y %H:%M:%S GMT", &tm);
>  		if (r == 0)
> -			return got_error(GOT_ERR_NO_SPACE);
> +			return -1;
>  
> -		*repo_age = strdup(datebuf);
> -		if (*repo_age == NULL)
> -			return got_error_from_errno("asprintf");
> +		if (fcgi_puts(tp, datebuf) == -1)
> +			return -1;
>  		break;
>  	}
> -	return NULL;
> +	return 0;
>  }
> blob - 3b5d3050768b060fb3d989730d25bac381291c95
> blob + b84fe9526fa6e65fe446b6bdf8b85d2b63e73233
> --- gotwebd/gotwebd.h
> +++ gotwebd/gotwebd.h
> @@ -155,7 +155,7 @@ struct repo_dir {
>  	char			*owner;
>  	char			*description;
>  	char			*url;
> -	char			*age;
> +	time_t			 age;
>  	char			*path;
>  };
>  
> @@ -451,7 +451,7 @@ const struct got_error *gotweb_get_time_str(char **, t
>  /* gotweb.c */
>  void gotweb_get_navs(struct request *, struct gotweb_url *, int *,
>      struct gotweb_url *, int *);
> -const struct got_error *gotweb_get_time_str(char **, time_t, int);
> +int gotweb_render_age(struct template *, time_t, int);
>  const struct got_error *gotweb_init_transport(struct transport **);
>  const char *gotweb_action_name(int);
>  int gotweb_render_url(struct request *, struct gotweb_url *);
> @@ -500,8 +500,8 @@ const struct got_error *got_get_repo_age(char **, stru
>  /* got_operations.c */
>  const struct got_error *got_gotweb_flushfile(FILE *, int);
>  const struct got_error *got_get_repo_owner(char **, struct request *);
> -const struct got_error *got_get_repo_age(char **, struct request *,
> -    const char *, int);
> +const struct got_error *got_get_repo_age(time_t *, struct request *,
> +    const char *);
>  const struct got_error *got_get_repo_commits(struct request *, int);
>  const struct got_error *got_get_repo_tags(struct request *, int);
>  const struct got_error *got_get_repo_heads(struct request *);
> blob - 6325fdc6ed674e01b958b4cd26fb051e5c7f6a4c
> blob + bd93fb21ec16695dbf29784ea4a8d2de3efdecac
> --- gotwebd/pages.tmpl
> +++ gotwebd/pages.tmpl
> @@ -49,21 +49,6 @@ static int
>  static inline int rss_tag_item(struct template *, struct repo_tag *);
>  static inline int rss_author(struct template *, char *);
>  
> -static int
> -gotweb_render_age(struct template *tp, time_t time, int ref_tm)
> -{
> -	const struct got_error *err;
> -	char *age;
> -	int r;
> -
> -	err = gotweb_get_time_str(&age, time, ref_tm);
> -	if (err)
> -		return 0;
> -	r = tp->tp_puts(tp, age);
> -	free(age);
> -	return r;
> -}
> -
>  !}
>  
>  {{ define gotweb_render_header(struct template *tp) }}
> @@ -221,7 +206,7 @@ gotweb_render_age(struct template *tp, time_t time, in
>    {{ end }}
>    {{ if srv->show_repo_age }}
>      <div class="index_project_age">
> -      {{ repo_dir->age }}
> +      {{ render gotweb_render_age(tp, repo_dir->age, TM_DIFF) }}
>      </div>
>    {{ end }}
>    <div class="navs_wrapper">
> @@ -798,7 +783,7 @@ gotweb_render_age(struct template *tp, time_t time, in
>  	struct request		*c = tp->tp_arg;
>  	struct querystring	*qs = c->t->qs;
>  	const char		*refname;
> -	char			*age = NULL;
> +	time_t			 age;
>  	struct gotweb_url	 url = {
>  		.action = SUMMARY,
>  		.index_page = -1,
> @@ -808,7 +793,7 @@ gotweb_render_age(struct template *tp, time_t time, in
>  
>  	refname = got_ref_get_name(re->ref);
>  
> -	err = got_get_repo_age(&age, c, refname, TM_DIFF);
> +	err = got_get_repo_age(&age, c, refname);
>  	if (err) {
>  		log_warnx("%s: %s", __func__, err->msg);
>  		return -1;
> @@ -820,7 +805,9 @@ gotweb_render_age(struct template *tp, time_t time, in
>  	url.headref = refname;
>  !}
>  <div class="branches_wrapper">
> -  <div class="branches_age">{{ age }}</div>
> +  <div class="branches_age">
> +    {{ render gotweb_render_age(tp, age, TM_DIFF) }}
> +  </div>
>    <div class="branches_space">&nbsp;</div>
>    <div class="branch">
>      <a href="{{ render gotweb_render_url(c, &url) }}">{{ refname }}</a>
> @@ -838,8 +825,6 @@ gotweb_render_age(struct template *tp, time_t time, in
>    </div>
>    <div class="dotted_line"></div>
>  </div>
> -{{ finally }}
> -{! free(age); !}
>  {{ end }}
>  
>  {{ define gotweb_render_summary(struct template *tp,
> @@ -860,7 +845,9 @@ gotweb_render_age(struct template *tp, time_t time, in
>    {{ end }}
>    {{ if srv->show_repo_age }}
>      <div id="last_change_title">Last Change:</div>
> -    <div id="last_change">{{ t->repo_dir->age }}</div>
> +    <div id="last_change">
> +      {{ render gotweb_render_age(tp, t->repo_dir->age, TM_DIFF) }}
> +    </div>
>    {{ end }}
>    {{ if srv->show_repo_cloneurl }}
>      <div id="cloneurl_title">Clone URL:</div>
> 

-- 
Mark Jamsek <fnc.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68