Restyle the sources and fix the audit's findings

1 file changed, 204 insertions, 159 deletions

diff --git a/source/ui-repolist.c b/source/ui-repolist.c
index e2c4abf..3bdae37 100644
--- a/source/ui-repolist.c
+++ b/source/ui-repolist.c
@@ -1,113 +1,137 @@
-/* ui-repolist.c: functions for generating the repolist page
- *
- * Copyright (C) 2006-2014 cgit Development Team <cgit@lists.zx2c4.com>
- *
- * Licensed under GNU General Public License v2
- *   (see LICENSE.txt for full license text)
+/*
+ * The repository index, which is the page a cgit site opens on, and the site
+ * readme the about page falls back to when a request names no repository.
+ * Every repository cgitrc registered is a candidate row, cut down to those
+ * matching the search terms and the url prefix the request carried, ordered
+ * either by the column the reader asked for or by section, and split into
+ * pages of max-repo-count rows.
  */
 
 #include "cgit.h"
-#include "ui-repolist.h"
+#include "filter.h"
 #include "html.h"
+#include "shared.h"
+#include "ui-repolist.h"
 #include "ui-shared.h"
 
+// A section heading spans the whole table, so this has to stay in step with
+// the headings print_header_row emits. Name, description and idle are always
+// there, owner and links are added when cgitrc asks for them.
+#define BASE_COLUMNS 3
+
+struct sort_column {
+	const char *name;
+	int (*cmp)(const void *a, const void *b);
+};
+
 static time_t read_agefile(const char *path)
 {
 	time_t result;
 	size_t size;
 	char *buf = NULL;
-	struct strbuf date_buf = STRBUF_INIT;
+	struct strbuf date = STRBUF_INIT;
 
 	if (cgit_read_first_line(path, &buf, &size)) {
 		free(buf);
 		return 0;
 	}
 
-	if (parse_date(buf, &date_buf) == 0)
-		result = strtoul(date_buf.buf, NULL, 10);
+	// parse_date writes the timestamp followed by its timezone offset, so
+	// only the seconds at the front are wanted here.
+	if (parse_date(buf, &date) == 0)
+		result = strtoul(date.buf, NULL, 10);
 	else
 		result = 0;
 	free(buf);
-	strbuf_release(&date_buf);
+	strbuf_release(&date);
 	return result;
 }
 
+/*
+ * The returned name can point into head, which the caller has to keep alive
+ * for as long as it uses it.
+ */
+static const char *tip_branch(const struct cgit_repo *repo, struct strbuf *head)
+{
+	struct strbuf path = STRBUF_INIT;
+	const char *branch = repo->defbranch;
+
+	if (branch)
+		return branch;
+
+	strbuf_addf(&path, "%s/HEAD", repo->path);
+	if (strbuf_read_file(head, path.buf, 0) > 0) {
+		strbuf_rtrim(head);
+		if (!skip_prefix(head->buf, "ref: refs/heads/", &branch))
+			branch = NULL;
+		// HEAD belongs to the repository, so a crafted target such as
+		// "ref: refs/heads/../../.." must not let the caller's stat
+		// walk outside it. Git forbids ".." in a ref name anyway.
+		if (branch && strstr(branch, ".."))
+			branch = NULL;
+	}
+	strbuf_release(&path);
+
+	return branch ? branch : "master";
+}
+
 static int get_repo_modtime(const struct cgit_repo *repo, time_t *mtime)
 {
 	struct strbuf path = STRBUF_INIT;
 	struct strbuf head = STRBUF_INIT;
-	struct stat s;
-	struct cgit_repo *r = (struct cgit_repo *)repo;
+	struct stat st;
 	const char *branch;
+	// The comparators below are handed const repositories, but the answer
+	// is kept in the repository itself so one page stats it only once.
+	struct cgit_repo *writable = (struct cgit_repo *)repo;
 
 	if (repo->mtime != -1) {
 		*mtime = repo->mtime;
 		return 1;
 	}
 	strbuf_addf(&path, "%s/%s", repo->path, ctx.cfg.agefile);
-	if (stat(path.buf, &s) == 0) {
+	if (stat(path.buf, &st) == 0) {
 		*mtime = read_agefile(path.buf);
 		if (*mtime) {
-			r->mtime = *mtime;
+			writable->mtime = *mtime;
 			goto end;
 		}
 	}
 
-	/* Stat the tip of the default branch. Prefer a configured defbranch,
-	 * otherwise read HEAD so a repo on "main" (or any branch name) is
-	 * handled, not only "master".
-	 */
-	branch = repo->defbranch;
-	if (!branch) {
-		strbuf_reset(&path);
-		strbuf_addf(&path, "%s/HEAD", repo->path);
-		if (strbuf_read_file(&head, path.buf, 0) > 0) {
-			strbuf_rtrim(&head);
-			if (!skip_prefix(head.buf, "ref: refs/heads/", &branch))
-				branch = NULL;
-			/* HEAD is repo-controlled, so a crafted target such as
-			 * "ref: refs/heads/../../.." must not let the stat()
-			 * below walk outside the repository. Git forbids ".."
-			 * in ref names anyway. */
-			if (branch && strstr(branch, ".."))
-				branch = NULL;
-		}
-		if (!branch)
-			branch = "master";
-	}
-
+	branch = tip_branch(repo, &head);
 	strbuf_reset(&path);
 	strbuf_addf(&path, "%s/refs/heads/%s", repo->path, branch);
-	if (stat(path.buf, &s) == 0) {
-		*mtime = s.st_mtime;
-		r->mtime = *mtime;
+	if (stat(path.buf, &st) == 0) {
+		*mtime = st.st_mtime;
+		writable->mtime = *mtime;
 		goto end;
 	}
 
 	strbuf_reset(&path);
-	strbuf_addf(&path, "%s/%s", repo->path, "packed-refs");
-	if (stat(path.buf, &s) == 0) {
-		*mtime = s.st_mtime;
-		r->mtime = *mtime;
+	strbuf_addf(&path, "%s/packed-refs", repo->path);
+	if (stat(path.buf, &st) == 0) {
+		*mtime = st.st_mtime;
+		writable->mtime = *mtime;
 		goto end;
 	}
 
 	*mtime = 0;
-	r->mtime = *mtime;
+	writable->mtime = *mtime;
 end:
 	strbuf_release(&path);
 	strbuf_release(&head);
-	return (r->mtime != 0);
+	return (writable->mtime != 0);
 }
 
 static void print_modtime(struct cgit_repo *repo)
 {
-	time_t t;
-	if (get_repo_modtime(repo, &t))
-		cgit_print_age(t, 0, -1);
+	time_t mtime;
+
+	if (get_repo_modtime(repo, &mtime))
+		cgit_print_age(mtime, 0, -1);
 }
 
-static int is_match(struct cgit_repo *repo)
+static int matches_search(struct cgit_repo *repo)
 {
 	if (!ctx.qry.search)
 		return 1;
@@ -122,7 +146,7 @@ static int is_match(struct cgit_repo *repo)
 	return 0;
 }
 
-static int is_in_url(struct cgit_repo *repo)
+static int matches_url(struct cgit_repo *repo)
 {
 	if (!ctx.qry.url)
 		return 1;
@@ -135,7 +159,7 @@ static int is_visible(struct cgit_repo *repo)
 {
 	if (repo->hide || repo->ignore)
 		return 0;
-	if (!(is_match(repo) && is_in_url(repo)))
+	if (!(matches_search(repo) && matches_url(repo)))
 		return 0;
 	return 1;
 }
@@ -151,14 +175,14 @@ static int any_repos_visible(void)
 	return 0;
 }
 
-// The index url is the same for every heading and every row, so the caller
-// works it out once rather than building and freeing one per cell.
-static void print_sort_header(const char *title, const char *sort,
-			      const char *currenturl)
+// currenturl is passed in because it is the same for every heading and every
+// row, and working it out here would mean an allocation and a free per cell.
+static void print_column_header(const char *title, const char *column,
+				const char *currenturl)
 {
-	htmlf("<th class='left col-%s'><a href='", sort);
+	htmlf("<th class='left col-%s'><a href='", column);
 	html_attr(currenturl);
-	htmlf("?s=%s", sort);
+	htmlf("?s=%s", column);
 	if (ctx.qry.search) {
 		html("&amp;q=");
 		html_url_arg(ctx.qry.search);
@@ -166,26 +190,83 @@ static void print_sort_header(const char *title, const char *sort,
 	htmlf("'>%s</a></th>", title);
 }
 
-static void print_header(const char *currenturl)
+static void print_header_row(const char *currenturl)
 {
 	html("<tr class='nohover'>");
-	print_sort_header("Name", "name", currenturl);
-	print_sort_header("Description", "desc", currenturl);
+	print_column_header("Name", "name", currenturl);
+	print_column_header("Description", "desc", currenturl);
 	if (ctx.cfg.enable_index_owner)
-		print_sort_header("Owner", "owner", currenturl);
-	print_sort_header("Idle", "idle", currenturl);
+		print_column_header("Owner", "owner", currenturl);
+	print_column_header("Idle", "idle", currenturl);
 	if (ctx.cfg.enable_index_links)
 		html("<th class='left col-links'>Links</th>");
 	html("</tr>\n");
 }
 
+static int section_changed(const char *section, const char *last)
+{
+	if (!section && !last)
+		return 0;
+	if (!section || !last)
+		return 1;
+	return strcmp(section, last) != 0;
+}
 
-static void print_pager(int items, int pagelen, char *search, char *sort)
+static void print_section_row(const char *section, int columns)
+{
+	htmlf("<tr class='nohover-highlight'><td colspan='%d' class='reposection'>",
+	      columns);
+	html_txt(section);
+	html("</td></tr>");
+}
+
+static void print_repo_row(const char *currenturl, int sublevel)
+{
+	char *repourl;
+
+	htmlf("<tr><td class='col-name %s'>",
+	      sublevel ? "sublevel-repo" : "toplevel-repo");
+	cgit_summary_link(ctx.repo->name, NULL, NULL, NULL);
+	html("</td><td class='col-desc'>");
+	repourl = cgit_repourl(ctx.repo->url);
+	html_link_open(repourl, NULL, NULL);
+	free(repourl);
+	if (html_ntxt(ctx.repo->desc, ctx.cfg.max_repodesc_len) < 0)
+		html("...");
+	html_link_close();
+	html("</td>");
+	if (ctx.cfg.enable_index_owner) {
+		html("<td class='col-owner'>");
+		html("<a href='");
+		html_attr(currenturl);
+		html("?q=");
+		html_url_arg(ctx.repo->owner);
+		html("'>");
+		html_txt(ctx.repo->owner);
+		html("</a>");
+		html("</td>");
+	}
+	html("<td class='col-idle'>");
+	print_modtime(ctx.repo);
+	html("</td>");
+	if (ctx.cfg.enable_index_links) {
+		html("<td class='col-links'>");
+		cgit_summary_link("summary", NULL, "button", NULL);
+		cgit_log_link("log", NULL, "button", NULL, NULL, NULL,
+			      0, NULL, NULL, ctx.qry.showmsg, 0);
+		cgit_tree_link("tree", NULL, "button", NULL, NULL, NULL);
+		html("</td>");
+	}
+	html("</tr>\n");
+}
+
+static void print_pager(int total, int pagelen, char *search, char *sort)
 {
 	int i, ofs;
 	char *class = NULL;
+
 	html("<ul class='pager'>");
-	for (i = 0, ofs = 0; ofs < items; i++, ofs = i * pagelen) {
+	for (i = 0, ofs = 0; ofs < total; i++, ofs = i * pagelen) {
 		class = (ctx.qry.ofs == ofs) ? "current" : NULL;
 		html("<li>");
 		cgit_index_link(cgit_fmt("[%d]", i + 1), cgit_fmt("Page %d", i + 1),
@@ -195,7 +276,7 @@ static void print_pager(int items, int pagelen, char *search, char *sort)
 	html("</ul>");
 }
 
-static int cmp(const char *s1, const char *s2)
+static int cmp_str(const char *s1, const char *s2)
 {
 	if (s1 && s2) {
 		if (ctx.cfg.case_sensitive_sort)
@@ -210,44 +291,31 @@ static int cmp(const char *s1, const char *s2)
 	return 0;
 }
 
-static int sort_name(const void *a, const void *b)
+static int cmp_name(const void *a, const void *b)
 {
 	const struct cgit_repo *r1 = a;
 	const struct cgit_repo *r2 = b;
 
-	return cmp(r1->name, r2->name);
+	return cmp_str(r1->name, r2->name);
 }
 
-static int sort_desc(const void *a, const void *b)
+static int cmp_desc(const void *a, const void *b)
 {
 	const struct cgit_repo *r1 = a;
 	const struct cgit_repo *r2 = b;
 
-	return cmp(r1->desc, r2->desc);
+	return cmp_str(r1->desc, r2->desc);
 }
 
-static int sort_owner(const void *a, const void *b)
+static int cmp_owner(const void *a, const void *b)
 {
 	const struct cgit_repo *r1 = a;
 	const struct cgit_repo *r2 = b;
 
-	return cmp(r1->owner, r2->owner);
+	return cmp_str(r1->owner, r2->owner);
 }
 
-/* Resolve every repository's modification time up front. get_repo_modtime
- * caches into the repo it is given, but qsort moves those structs around while
- * it sorts, so a comparator that fills the cache loses most of what it stored
- * and stats the same repository again and again. */
-static void resolve_modtimes(void)
-{
-	time_t t;
-	int i;
-
-	for (i = 0; i < cgit_repolist.count; i++)
-		get_repo_modtime(&cgit_repolist.repos[i], &t);
-}
-
-static int sort_idle(const void *a, const void *b)
+static int cmp_idle(const void *a, const void *b)
 {
 	const struct cgit_repo *r1 = a;
 	const struct cgit_repo *r2 = b;
@@ -256,8 +324,9 @@ static int sort_idle(const void *a, const void *b)
 	t1 = t2 = 0;
 	get_repo_modtime(r1, &t1);
 	get_repo_modtime(r2, &t2);
-	/* Return the sign only; a truncated 64-bit time_t difference could
-	 * flip and make the comparator inconsistent. */
+	// Only the sign is returned, because a 64-bit difference truncated
+	// into an int could come back with the wrong sign and leave the
+	// ordering inconsistent.
 	if (t2 > t1)
 		return 1;
 	if (t2 < t1)
@@ -265,61 +334,70 @@ static int sort_idle(const void *a, const void *b)
 	return 0;
 }
 
-static int sort_section(const void *a, const void *b)
+static int cmp_section(const void *a, const void *b)
 {
 	const struct cgit_repo *r1 = a;
 	const struct cgit_repo *r2 = b;
 	int result;
 
-	result = cmp(r1->section, r2->section);
+	result = cmp_str(r1->section, r2->section);
 	if (!result) {
 		if (!strcmp(ctx.cfg.repository_sort, "age"))
-			result = sort_idle(r1, r2);
+			result = cmp_idle(r1, r2);
 		if (!result)
-			result = cmp(r1->name, r2->name);
+			result = cmp_str(r1->name, r2->name);
 	}
 	return result;
 }
 
-struct sortcolumn {
-	const char *name;
-	int (*fn)(const void *a, const void *b);
-};
+/*
+ * get_repo_modtime caches into the repository it is handed, but qsort moves
+ * those structs around as it works, so a comparator left to fill the cache
+ * loses most of what it stored and stats the same repository over and over.
+ */
+static void resolve_modtimes(void)
+{
+	time_t t;
+	int i;
+
+	for (i = 0; i < cgit_repolist.count; i++)
+		get_repo_modtime(&cgit_repolist.repos[i], &t);
+}
 
-static const struct sortcolumn sortcolumn[] = {
-	{"section", sort_section},
-	{"name", sort_name},
-	{"desc", sort_desc},
-	{"owner", sort_owner},
-	{"idle", sort_idle},
+static const struct sort_column sort_columns[] = {
+	{"section", cmp_section},
+	{"name", cmp_name},
+	{"desc", cmp_desc},
+	{"owner", cmp_owner},
+	{"idle", cmp_idle},
 	{NULL, NULL}
 };
 
 static int sort_repolist(char *field)
 {
-	const struct sortcolumn *column;
+	const struct sort_column *column;
 
-	for (column = &sortcolumn[0]; column->name; column++) {
+	for (column = &sort_columns[0]; column->name; column++) {
 		if (strcmp(field, column->name))
 			continue;
-		if (column->fn == sort_idle || column->fn == sort_section)
+		if (column->cmp == cmp_idle || column->cmp == cmp_section)
 			resolve_modtimes();
 		qsort(cgit_repolist.repos, cgit_repolist.count,
-			sizeof(struct cgit_repo), column->fn);
+			sizeof(struct cgit_repo), column->cmp);
 		return 1;
 	}
 	return 0;
 }
 
-
 void cgit_print_repolist(void)
 {
-	int i, columns = 3, hits = 0, header = 0;
 	char *last_section = NULL;
-	char *section;
-	char *repourl;
 	char *currenturl;
-	int sorted = 0;
+	int columns = BASE_COLUMNS;
+	int column_sorted = 0;
+	int hits = 0;
+	int shown = 0;
+	int i;
 
 	if (!any_repos_visible()) {
 		cgit_print_error_page(404, "Not found", "No repositories found");
@@ -337,71 +415,38 @@ void cgit_print_repolist(void)
 	cgit_print_pageheader();
 
 	if (ctx.qry.sort)
-		sorted = sort_repolist(ctx.qry.sort);
+		column_sorted = sort_repolist(ctx.qry.sort);
 	else if (ctx.cfg.section_sort)
 		sort_repolist("section");
 
 	currenturl = cgit_currenturl();
 	html("<table class='list nowrap repolist'>");
 	for (i = 0; i < cgit_repolist.count; i++) {
+		char *section;
+
 		ctx.repo = &cgit_repolist.repos[i];
 		if (!is_visible(ctx.repo))
 			continue;
 		hits++;
+		// Rows outside the page are stepped over rather than broken
+		// out of, because the pager below is sized from the total.
 		if (hits <= ctx.qry.ofs)
 			continue;
-		if (hits > ctx.qry.ofs + ctx.cfg.max_repo_count)
+		// Written as a subtraction because max-repo-count of zero
+		// means unlimited and is held as INT_MAX, which any positive
+		// offset would overflow if it were added to instead.
+		if (hits - ctx.qry.ofs > ctx.cfg.max_repo_count)
 			continue;
-		if (!header++)
-			print_header(currenturl);
+		if (!shown++)
+			print_header_row(currenturl);
 		section = ctx.repo->section;
 		if (section && !strcmp(section, ""))
 			section = NULL;
-		if (!sorted &&
-		    ((last_section == NULL && section != NULL) ||
-		    (last_section != NULL && section == NULL) ||
-		    (last_section != NULL && section != NULL &&
-		     strcmp(section, last_section)))) {
-			htmlf("<tr class='nohover-highlight'><td colspan='%d' class='reposection'>",
-			      columns);
-			html_txt(section);
-			html("</td></tr>");
+		if (!column_sorted && section_changed(section, last_section)) {
+			print_section_row(section, columns);
 			last_section = section;
 		}
-		htmlf("<tr><td class='col-name %s'>",
-		      !sorted && section ? "sublevel-repo" : "toplevel-repo");
-		cgit_summary_link(ctx.repo->name, NULL, NULL, NULL);
-		html("</td><td class='col-desc'>");
-		repourl = cgit_repourl(ctx.repo->url);
-		html_link_open(repourl, NULL, NULL);
-		free(repourl);
-		if (html_ntxt(ctx.repo->desc, ctx.cfg.max_repodesc_len) < 0)
-			html("...");
-		html_link_close();
-		html("</td>");
-		if (ctx.cfg.enable_index_owner) {
-			html("<td class='col-owner'>");
-			html("<a href='");
-			html_attr(currenturl);
-			html("?q=");
-			html_url_arg(ctx.repo->owner);
-			html("'>");
-			html_txt(ctx.repo->owner);
-			html("</a>");
-			html("</td>");
-		}
-		html("<td class='col-idle'>");
-		print_modtime(ctx.repo);
-		html("</td>");
-		if (ctx.cfg.enable_index_links) {
-			html("<td class='col-links'>");
-			cgit_summary_link("summary", NULL, "button", NULL);
-			cgit_log_link("log", NULL, "button", NULL, NULL, NULL,
-				      0, NULL, NULL, ctx.qry.showmsg, 0);
-			cgit_tree_link("tree", NULL, "button", NULL, NULL, NULL);
-			html("</td>");
-		}
-		html("</tr>\n");
+		print_repo_row(currenturl, !column_sorted && section);
 	}
 	html("</table>");
 	if (hits > ctx.cfg.max_repo_count)