[PATCH 0/3] ui-repolist: Support additional formatting for sections

[PATCH 0/3] ui-repolist: Support additional formatting for sections

[tim.nordell]

On our internal server we have over 2,000 repositories, primarily due to
mirroring external repositories locally.  We patched cgit to make navigation
a little easier by doing the following:

 - Make sections clickable (via a section filter)
 - Collapse repository listing to just sections when the repository listing
   is over 1 page worth of repositories

The patchset here contains this modification and allows it to be enabled
(or disabled) via configuration.

You can see an example of this UI over here: http://git.logicpd.com

(Note: That is running an older patched version not containing the changes
here presently but has the same presentation.)

Tim Nordell (3):
  ui-repolist: Add section filter
  ui-repolist: Restructure internal logic to be more extensible
  ui-repolist: Support a trimmed view when several sections are present

 cgit.c                      |   2 +
 cgit.h                      |   3 +-
 cgitrc.5.txt                |  23 +++-
 filter.c                    |   1 +
 filters/section-example.lua |  33 ++++++
 ui-repolist.c               | 271 +++++++++++++++++++++++++++++++++-----------
 6 files changed, 260 insertions(+), 73 deletions(-)
 create mode 100644 filters/section-example.lua

-- 
2.4.9

[PATCH 1/3] ui-repolist: Add section filter

[tim.nordell]

This allows custom links to be added into the section headers by
configuring a filter to be applied in the repository list.

Signed-off-by: Tim Nordell <tim.nordell at logicpd.com>

 create mode 100644 filters/section-example.lua

diff --git a/cgit.c b/cgit.c
index 87ba811..7dac332 100644
--- a/cgit.c
+++ b/cgit.c
@@ -234,6 +234,8 @@ static void config_cb(const char *name, const char *value)
 		ctx.cfg.email_filter = cgit_new_filter(value, EMAIL);
 	else if (!strcmp(name, "owner-filter"))
 		ctx.cfg.owner_filter = cgit_new_filter(value, OWNER);
+	else if (!strcmp(name, "section-filter"))
+		ctx.cfg.section_filter = cgit_new_filter(value, SECTION);
 	else if (!strcmp(name, "auth-filter"))
 		ctx.cfg.auth_filter = cgit_new_filter(value, AUTH);
 	else if (!strcmp(name, "embedded"))
diff --git a/cgit.h b/cgit.h
index b2b2ae9..f0ac36b 100644
--- a/cgit.h
+++ b/cgit.h
@@ -55,7 +55,7 @@ typedef enum {
 } diff_type;
 
 typedef enum {
-	ABOUT, COMMIT, SOURCE, EMAIL, AUTH, OWNER
+	ABOUT, COMMIT, SOURCE, EMAIL, AUTH, OWNER, SECTION
 } filter_type;
 
 struct cgit_filter {
@@ -272,6 +272,7 @@ struct cgit_config {
 	struct cgit_filter *source_filter;
 	struct cgit_filter *email_filter;
 	struct cgit_filter *owner_filter;
+	struct cgit_filter *section_filter;
 	struct cgit_filter *auth_filter;
 };
 
diff --git a/cgitrc.5.txt b/cgitrc.5.txt
index 3bca0ab..113b7a0 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -404,6 +404,13 @@ section::
 	after this option will inherit the current section name. Default value:
 	none.
 
+section-filter::
+	Specifies a command which will be invoked to format section headings.
+	The command will get the section on its STDIN, and the STDOUT from the
+	command will be included verbatim as the section heading.
+	Default value: none.
+	See also: "FILTER API".
+
 section-sort::
 	Flag which, when set to "1", will sort the sections on the repository
 	listing by name. Set this flag to "0" if the order in the cgitrc file should
@@ -695,6 +702,11 @@ owner filter::
 	standard input and the filter is expected to write to standard
 	output.  The output is included in the Owner column.
 
+section filter::
+	This filter is given no arguments.  The section text is avilable on
+	standard input and the filter is expected to write to standard
+	output.  The output is included in the section heading.
+
 source filter::
 	This filter is given a single parameter: the filename of the source
 	file to filter. The filter can use the filename to determine (for
diff --git a/filter.c b/filter.c
index 949c931..952336f 100644
--- a/filter.c
+++ b/filter.c
@@ -436,6 +436,7 @@ struct cgit_filter *cgit_new_filter(const char *cmd, filter_type filtertype)
 			argument_count = 1;
 			break;
 
+		case SECTION:
 		case COMMIT:
 		default:
 			argument_count = 0;
diff --git a/filters/section-example.lua b/filters/section-example.lua
new file mode 100644
index 0000000..76076b2
--- /dev/null
+++ b/filters/section-example.lua
@@ -0,0 +1,33 @@
+-- This script is an example of an section-filter.  It replaces the
+-- section title with several links for each subdirectory represented
+-- within the section title.  (It's intended to be used where the section
+-- title is also the same as the path to the repository, similar to
+-- the output from the "section-from-path" option.)  This script may
+-- be used with the section-filter setting in cgitrc with the `lua:`
+-- prefix.
+
+function gen_link(name, path)
+end
+
+function filter_open()
+	buffer = ""
+end
+
+function filter_close()
+	path = "/"
+	cnt = 0
+	for i in string.gmatch(buffer, "[^/]+") do
+		if cnt > 0 then
+			html("/")
+		end
+		path = path .. i .. "/"
+		html(string.format("<a href=\"%s%s\" class=reposection>%s</a>",
+			os.getenv("SCRIPT_NAME"), path, i))
+		cnt = cnt + 1
+	end
+	return 0
+end
+
+function filter_write(str)
+	buffer = buffer .. str
+end
diff --git a/ui-repolist.c b/ui-repolist.c
index 30915df..7af77e0 100644
--- a/ui-repolist.c
+++ b/ui-repolist.c
@@ -322,7 +322,11 @@ void cgit_print_repolist(void)
 		     strcmp(section, last_section)))) {
 			htmlf("<tr class='nohover'><td colspan='%d' class='reposection'>",
 			      columns);
+			if (ctx.cfg.section_filter)
+				cgit_open_filter(ctx.cfg.section_filter);
 			html_txt(section);
+			if (ctx.cfg.section_filter)
+				cgit_close_filter(ctx.cfg.section_filter);
 			html("</td></tr>");
 			last_section = section;
 		}
-- 
2.4.9

[PATCH 1/3] ui-repolist: Add section filter

[john]

On Fri, Feb 26, 2016 at 02:58:57PM -0600, Tim Nordell wrote:
> This allows custom links to be added into the section headers by
> configuring a filter to be applied in the repository list.
> 
> Signed-off-by: Tim Nordell <tim.nordell at logicpd.com>
> 
>  create mode 100644 filters/section-example.lua
> 
> diff --git a/cgit.c b/cgit.c
> index 87ba811..7dac332 100644
> --- a/cgit.c
> +++ b/cgit.c
> @@ -234,6 +234,8 @@ static void config_cb(const char *name, const char *value)
>  		ctx.cfg.email_filter = cgit_new_filter(value, EMAIL);
>  	else if (!strcmp(name, "owner-filter"))
>  		ctx.cfg.owner_filter = cgit_new_filter(value, OWNER);
> +	else if (!strcmp(name, "section-filter"))
> +		ctx.cfg.section_filter = cgit_new_filter(value, SECTION);
>  	else if (!strcmp(name, "auth-filter"))
>  		ctx.cfg.auth_filter = cgit_new_filter(value, AUTH);
>  	else if (!strcmp(name, "embedded"))
> diff --git a/cgit.h b/cgit.h
> index b2b2ae9..f0ac36b 100644
> --- a/cgit.h
> +++ b/cgit.h
> @@ -55,7 +55,7 @@ typedef enum {
>  } diff_type;
>  
>  typedef enum {
> -	ABOUT, COMMIT, SOURCE, EMAIL, AUTH, OWNER
> +	ABOUT, COMMIT, SOURCE, EMAIL, AUTH, OWNER, SECTION
>  } filter_type;
>  
>  struct cgit_filter {
> @@ -272,6 +272,7 @@ struct cgit_config {
>  	struct cgit_filter *source_filter;
>  	struct cgit_filter *email_filter;
>  	struct cgit_filter *owner_filter;
> +	struct cgit_filter *section_filter;
>  	struct cgit_filter *auth_filter;
>  };
>  
> diff --git a/cgitrc.5.txt b/cgitrc.5.txt
> index 3bca0ab..113b7a0 100644
> --- a/cgitrc.5.txt
> +++ b/cgitrc.5.txt
> @@ -404,6 +404,13 @@ section::
>  	after this option will inherit the current section name. Default value:
>  	none.
>  
> +section-filter::
> +	Specifies a command which will be invoked to format section headings.
> +	The command will get the section on its STDIN, and the STDOUT from the
> +	command will be included verbatim as the section heading.
> +	Default value: none.
> +	See also: "FILTER API".
> +
>  section-sort::
>  	Flag which, when set to "1", will sort the sections on the repository
>  	listing by name. Set this flag to "0" if the order in the cgitrc file should
> @@ -695,6 +702,11 @@ owner filter::
>  	standard input and the filter is expected to write to standard
>  	output.  The output is included in the Owner column.
>  
> +section filter::
> +	This filter is given no arguments.  The section text is avilable on
> +	standard input and the filter is expected to write to standard
> +	output.  The output is included in the section heading.
> +
>  source filter::
>  	This filter is given a single parameter: the filename of the source
>  	file to filter. The filter can use the filename to determine (for
> diff --git a/filter.c b/filter.c
> index 949c931..952336f 100644
> --- a/filter.c
> +++ b/filter.c
> @@ -436,6 +436,7 @@ struct cgit_filter *cgit_new_filter(const char *cmd, filter_type filtertype)
>  			argument_count = 1;
>  			break;
>  
> +		case SECTION:
>  		case COMMIT:
>  		default:
>  			argument_count = 0;
> diff --git a/filters/section-example.lua b/filters/section-example.lua
> new file mode 100644
> index 0000000..76076b2
> --- /dev/null
> +++ b/filters/section-example.lua
> @@ -0,0 +1,33 @@
> +-- This script is an example of an section-filter.  It replaces the
> +-- section title with several links for each subdirectory represented
> +-- within the section title.  (It's intended to be used where the section
> +-- title is also the same as the path to the repository, similar to
> +-- the output from the "section-from-path" option.)  This script may
> +-- be used with the section-filter setting in cgitrc with the `lua:`
> +-- prefix.
> +
> +function gen_link(name, path)
> +end
> +
> +function filter_open()
> +	buffer = ""
> +end
> +
> +function filter_close()
> +	path = "/"
> +	cnt = 0
> +	for i in string.gmatch(buffer, "[^/]+") do
> +		if cnt > 0 then
> +			html("/")
> +		end
> +		path = path .. i .. "/"
> +		html(string.format("<a href=\"%s%s\" class=reposection>%s</a>",
> +			os.getenv("SCRIPT_NAME"), path, i))
> +		cnt = cnt + 1
> +	end
> +	return 0
> +end
> +
> +function filter_write(str)
> +	buffer = buffer .. str
> +end
> diff --git a/ui-repolist.c b/ui-repolist.c
> index 30915df..7af77e0 100644
> --- a/ui-repolist.c
> +++ b/ui-repolist.c
> @@ -322,7 +322,11 @@ void cgit_print_repolist(void)
>  		     strcmp(section, last_section)))) {
>  			htmlf("<tr class='nohover'><td colspan='%d' class='reposection'>",
>  			      columns);
> +			if (ctx.cfg.section_filter)

No need for the if here, cgit_{open,close}_filter already do the right
thing if given a null filter.

> +				cgit_open_filter(ctx.cfg.section_filter);
>  			html_txt(section);
> +			if (ctx.cfg.section_filter)
> +				cgit_close_filter(ctx.cfg.section_filter);
>  			html("</td></tr>");
>  			last_section = section;
>  		}
> -- 
> 2.4.9
> 
> _______________________________________________
> CGit mailing list
> CGit at lists.zx2c4.com
> http://lists.zx2c4.com/mailman/listinfo/cgit

[PATCH 2/3] ui-repolist: Restructure internal logic to be more extensible

[tim.nordell]

The internal logic has been restructured so that there is a "walking"
routine that filters the repo list based on the visible criteria, and
subsequently calls a given callback for each repo found.  Additionally,
split out generating a table line for a given repo, and a table line
for a given section.  This makes this more loosely coupled and allows
reuse of more logic for changes to the way the repo list is displayed.
This patch does not make any functional changes to the system and is
a reorganization of the existing logic only.

Signed-off-by: Tim Nordell <tim.nordell at logicpd.com>

diff --git a/ui-repolist.c b/ui-repolist.c
index 7af77e0..6b06a1a 100644
--- a/ui-repolist.c
+++ b/ui-repolist.c
@@ -269,13 +269,135 @@ static int sort_repolist(char *field)
 	return 0;
 }
 
+struct repolist_ctx
+{
+	/* From outer contex passed to interior */
+	int columns;
+	int sorted;
+
+	/* Used in interior context; should be reset in repolist_walk_visible() */
+	int hits;
+	const char *last_section;
+};
+
+static void html_section(struct cgit_repo *repo, int columns)
+{
+	htmlf("<tr class='nohover'><td colspan='%d' class='reposection'>",
+		columns);
+	if (ctx.cfg.section_filter)
+		cgit_open_filter(ctx.cfg.section_filter);
+	html_txt(repo->section);
+	if (ctx.cfg.section_filter)
+		cgit_close_filter(ctx.cfg.section_filter);
+	html("</td></tr>");
+}
+
+static void html_repository(struct cgit_repo *repo, int sorted)
+{
+	bool is_toplevel;
+
+	ctx.repo = repo;
+
+	is_toplevel = (!sorted && NULL != repo->section && repo->section[0] != '\0');
+	htmlf("<tr><td class='%s'>", is_toplevel ? "sublevel-repo" : "toplevel-repo");
+	cgit_summary_link(repo->name, repo->name, NULL, NULL);
+	html("</td><td>");
+	html_link_open(cgit_repourl(repo->url), NULL, NULL);
+	html_ntxt(ctx.cfg.max_repodesc_len, repo->desc);
+	html_link_close();
+	html("</td><td>");
+	if (ctx.cfg.enable_index_owner) {
+		if (repo->owner_filter) {
+			cgit_open_filter(repo->owner_filter);
+			html_txt(repo->owner);
+			cgit_close_filter(repo->owner_filter);
+		} else {
+			html("<a href='");
+			html_attr(cgit_currenturl());
+			html("?q=");
+			html_url_arg(repo->owner);
+			html("'>");
+			html_txt(repo->owner);
+			html("</a>");
+		}
+		html("</td><td>");
+	}
+	print_modtime(repo);
+	html("</td>");
+	if (ctx.cfg.enable_index_links) {
+		html("<td>");
+		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 inline bool should_emit_section(struct cgit_repo *repo, struct repolist_ctx *c)
+{
+	/* If we're sorted, we will not have a new section emitted. */
+	if (c->sorted)
+		return false;
+
+	/* We need a valid repo section for the rest of the checks */
+	if(NULL == repo->section)
+		return false;
+
+	/* If the section title is blank (e.g. top-level), we never emit
+	 * a section heading. */
+	if('\0' == repo->section[0])
+		return false;
+
+	/* Finally, compare the last section name to the current.  If they're
+	 * the same, do not emit a section area. */
+	if(NULL != c->last_section && !strcmp(repo->section, c->last_section))
+		return false;
+
+	c->last_section = repo->section;
+	return true;
+}
+
+static int generate_repolist(struct cgit_repo *repo, struct repolist_ctx *c)
+{
+	c->hits++;
+	if (c->hits <= ctx.qry.ofs)
+		return 0;
+	if (c->hits > ctx.qry.ofs + ctx.cfg.max_repo_count)
+		return 0;
+
+	if(should_emit_section(repo, c))
+		html_section(repo, c->columns);
+	html_repository(repo, c->sorted);
+
+	return 0;
+}
+
+typedef int (*repolist_walk_callback_t)(struct cgit_repo *repo, struct repolist_ctx *c);
+static int repolist_walk_visible(repolist_walk_callback_t callback, struct repolist_ctx *c)
+{
+	struct cgit_repo *repo;
+	int i;
+
+	c->hits = 0;
+	c->last_section = NULL;
+
+	for (i = 0; i < cgit_repolist.count; i++) {
+		repo = &cgit_repolist.repos[i];
+		if (!is_visible(repo))
+			continue;
+		if(NULL != callback)
+			callback(repo, c);
+	}
+	return 0;
+}
 
 void cgit_print_repolist(void)
 {
-	int i, columns = 3, hits = 0, header = 0;
-	char *last_section = NULL;
-	char *section;
-	int sorted = 0;
+	struct repolist_ctx repolist_ctx = {0};
+
+	repolist_ctx.columns = 3;
 
 	if (!any_repos_visible()) {
 		cgit_print_error_page(404, "Not found", "No repositories found");
@@ -283,9 +405,9 @@ void cgit_print_repolist(void)
 	}
 
 	if (ctx.cfg.enable_index_links)
-		++columns;
+		++repolist_ctx.columns;
 	if (ctx.cfg.enable_index_owner)
-		++columns;
+		++repolist_ctx.columns;
 
 	ctx.page.title = ctx.cfg.root_title;
 	cgit_print_http_headers();
@@ -296,79 +418,19 @@ void cgit_print_repolist(void)
 		html_include(ctx.cfg.index_header);
 
 	if (ctx.qry.sort)
-		sorted = sort_repolist(ctx.qry.sort);
+		repolist_ctx.sorted = sort_repolist(ctx.qry.sort);
 	else if (ctx.cfg.section_sort)
 		sort_repolist("section");
 
 	html("<table summary='repository list' class='list nowrap'>");
-	for (i = 0; i < cgit_repolist.count; i++) {
-		ctx.repo = &cgit_repolist.repos[i];
-		if (!is_visible(ctx.repo))
-			continue;
-		hits++;
-		if (hits <= ctx.qry.ofs)
-			continue;
-		if (hits > ctx.qry.ofs + ctx.cfg.max_repo_count)
-			continue;
-		if (!header++)
-			print_header();
-		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'><td colspan='%d' class='reposection'>",
-			      columns);
-			if (ctx.cfg.section_filter)
-				cgit_open_filter(ctx.cfg.section_filter);
-			html_txt(section);
-			if (ctx.cfg.section_filter)
-				cgit_close_filter(ctx.cfg.section_filter);
-			html("</td></tr>");
-			last_section = section;
-		}
-		htmlf("<tr><td class='%s'>",
-		      !sorted && section ? "sublevel-repo" : "toplevel-repo");
-		cgit_summary_link(ctx.repo->name, ctx.repo->name, NULL, NULL);
-		html("</td><td>");
-		html_link_open(cgit_repourl(ctx.repo->url), NULL, NULL);
-		html_ntxt(ctx.cfg.max_repodesc_len, ctx.repo->desc);
-		html_link_close();
-		html("</td><td>");
-		if (ctx.cfg.enable_index_owner) {
-			if (ctx.repo->owner_filter) {
-				cgit_open_filter(ctx.repo->owner_filter);
-				html_txt(ctx.repo->owner);
-				cgit_close_filter(ctx.repo->owner_filter);
-			} else {
-				html("<a href='");
-				html_attr(cgit_currenturl());
-				html("?q=");
-				html_url_arg(ctx.repo->owner);
-				html("'>");
-				html_txt(ctx.repo->owner);
-				html("</a>");
-			}
-			html("</td><td>");
-		}
-		print_modtime(ctx.repo);
-		html("</td>");
-		if (ctx.cfg.enable_index_links) {
-			html("<td>");
-			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_header();
+
+	/* Count visible repositories */
+	repolist_walk_visible(generate_repolist, &repolist_ctx);
+
 	html("</table>");
-	if (hits > ctx.cfg.max_repo_count)
-		print_pager(hits, ctx.cfg.max_repo_count, ctx.qry.search, ctx.qry.sort);
+	if (repolist_ctx.hits > ctx.cfg.max_repo_count)
+		print_pager(repolist_ctx.hits, ctx.cfg.max_repo_count, ctx.qry.search, ctx.qry.sort);
 	cgit_print_docend();
 }
 
-- 
2.4.9

[PATCH 2/3] ui-repolist: Restructure internal logic to be more extensible

[john]

On Fri, Feb 26, 2016 at 02:58:58PM -0600, Tim Nordell wrote:
> The internal logic has been restructured so that there is a "walking"
> routine that filters the repo list based on the visible criteria, and
> subsequently calls a given callback for each repo found.  Additionally,
> split out generating a table line for a given repo, and a table line
> for a given section.  This makes this more loosely coupled and allows
> reuse of more logic for changes to the way the repo list is displayed.
> This patch does not make any functional changes to the system and is
> a reorganization of the existing logic only.

There's a lot going on in this patch that makes it very hard to review.
It would be easier if it were broken down into:

	extract struct repolist_ctx
	extract the new generate_repolist() function
	add repolist_walk_visible()

It would also be helpful to mention in the commit message why
repolist_walk_visible() is needed: in a following patch we will be
adding a variant that prints only section headers.

There are also various style problems here, but they're similar to
earlier patches and the Linux kernel's scripts/checkpatch.pl script
catches them.

> Signed-off-by: Tim Nordell <tim.nordell at logicpd.com>
> 
> diff --git a/ui-repolist.c b/ui-repolist.c
> index 7af77e0..6b06a1a 100644
> --- a/ui-repolist.c
> +++ b/ui-repolist.c
> @@ -269,13 +269,135 @@ static int sort_repolist(char *field)
>  	return 0;
>  }
>  
> +struct repolist_ctx
> +{
> +	/* From outer contex passed to interior */
> +	int columns;
> +	int sorted;
> +
> +	/* Used in interior context; should be reset in repolist_walk_visible() */
> +	int hits;
> +	const char *last_section;
> +};
> +
> +static void html_section(struct cgit_repo *repo, int columns)
> +{
> +	htmlf("<tr class='nohover'><td colspan='%d' class='reposection'>",
> +		columns);
> +	if (ctx.cfg.section_filter)
> +		cgit_open_filter(ctx.cfg.section_filter);
> +	html_txt(repo->section);
> +	if (ctx.cfg.section_filter)
> +		cgit_close_filter(ctx.cfg.section_filter);
> +	html("</td></tr>");
> +}
> +
> +static void html_repository(struct cgit_repo *repo, int sorted)
> +{
> +	bool is_toplevel;
> +
> +	ctx.repo = repo;
> +
> +	is_toplevel = (!sorted && NULL != repo->section && repo->section[0] != '\0');
> +	htmlf("<tr><td class='%s'>", is_toplevel ? "sublevel-repo" : "toplevel-repo");
> +	cgit_summary_link(repo->name, repo->name, NULL, NULL);
> +	html("</td><td>");
> +	html_link_open(cgit_repourl(repo->url), NULL, NULL);
> +	html_ntxt(ctx.cfg.max_repodesc_len, repo->desc);
> +	html_link_close();
> +	html("</td><td>");
> +	if (ctx.cfg.enable_index_owner) {
> +		if (repo->owner_filter) {
> +			cgit_open_filter(repo->owner_filter);
> +			html_txt(repo->owner);
> +			cgit_close_filter(repo->owner_filter);
> +		} else {
> +			html("<a href='");
> +			html_attr(cgit_currenturl());
> +			html("?q=");
> +			html_url_arg(repo->owner);
> +			html("'>");
> +			html_txt(repo->owner);
> +			html("</a>");
> +		}
> +		html("</td><td>");
> +	}
> +	print_modtime(repo);
> +	html("</td>");
> +	if (ctx.cfg.enable_index_links) {
> +		html("<td>");
> +		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 inline bool should_emit_section(struct cgit_repo *repo, struct repolist_ctx *c)
> +{
> +	/* If we're sorted, we will not have a new section emitted. */
> +	if (c->sorted)
> +		return false;
> +
> +	/* We need a valid repo section for the rest of the checks */
> +	if(NULL == repo->section)
> +		return false;
> +
> +	/* If the section title is blank (e.g. top-level), we never emit
> +	 * a section heading. */
> +	if('\0' == repo->section[0])
> +		return false;
> +
> +	/* Finally, compare the last section name to the current.  If they're
> +	 * the same, do not emit a section area. */
> +	if(NULL != c->last_section && !strcmp(repo->section, c->last_section))
> +		return false;
> +
> +	c->last_section = repo->section;
> +	return true;
> +}
> +
> +static int generate_repolist(struct cgit_repo *repo, struct repolist_ctx *c)
> +{
> +	c->hits++;
> +	if (c->hits <= ctx.qry.ofs)
> +		return 0;
> +	if (c->hits > ctx.qry.ofs + ctx.cfg.max_repo_count)
> +		return 0;
> +
> +	if(should_emit_section(repo, c))
> +		html_section(repo, c->columns);
> +	html_repository(repo, c->sorted);
> +
> +	return 0;
> +}
> +
> +typedef int (*repolist_walk_callback_t)(struct cgit_repo *repo, struct repolist_ctx *c);
> +static int repolist_walk_visible(repolist_walk_callback_t callback, struct repolist_ctx *c)
> +{
> +	struct cgit_repo *repo;
> +	int i;
> +
> +	c->hits = 0;
> +	c->last_section = NULL;
> +
> +	for (i = 0; i < cgit_repolist.count; i++) {
> +		repo = &cgit_repolist.repos[i];
> +		if (!is_visible(repo))
> +			continue;
> +		if(NULL != callback)
> +			callback(repo, c);
> +	}
> +	return 0;
> +}
>  
>  void cgit_print_repolist(void)
>  {
> -	int i, columns = 3, hits = 0, header = 0;
> -	char *last_section = NULL;
> -	char *section;
> -	int sorted = 0;
> +	struct repolist_ctx repolist_ctx = {0};
> +
> +	repolist_ctx.columns = 3;
>  
>  	if (!any_repos_visible()) {
>  		cgit_print_error_page(404, "Not found", "No repositories found");
> @@ -283,9 +405,9 @@ void cgit_print_repolist(void)
>  	}
>  
>  	if (ctx.cfg.enable_index_links)
> -		++columns;
> +		++repolist_ctx.columns;
>  	if (ctx.cfg.enable_index_owner)
> -		++columns;
> +		++repolist_ctx.columns;
>  
>  	ctx.page.title = ctx.cfg.root_title;
>  	cgit_print_http_headers();
> @@ -296,79 +418,19 @@ void cgit_print_repolist(void)
>  		html_include(ctx.cfg.index_header);
>  
>  	if (ctx.qry.sort)
> -		sorted = sort_repolist(ctx.qry.sort);
> +		repolist_ctx.sorted = sort_repolist(ctx.qry.sort);
>  	else if (ctx.cfg.section_sort)
>  		sort_repolist("section");
>  
>  	html("<table summary='repository list' class='list nowrap'>");
> -	for (i = 0; i < cgit_repolist.count; i++) {
> -		ctx.repo = &cgit_repolist.repos[i];
> -		if (!is_visible(ctx.repo))
> -			continue;
> -		hits++;
> -		if (hits <= ctx.qry.ofs)
> -			continue;
> -		if (hits > ctx.qry.ofs + ctx.cfg.max_repo_count)
> -			continue;
> -		if (!header++)
> -			print_header();
> -		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'><td colspan='%d' class='reposection'>",
> -			      columns);
> -			if (ctx.cfg.section_filter)
> -				cgit_open_filter(ctx.cfg.section_filter);
> -			html_txt(section);
> -			if (ctx.cfg.section_filter)
> -				cgit_close_filter(ctx.cfg.section_filter);
> -			html("</td></tr>");
> -			last_section = section;
> -		}
> -		htmlf("<tr><td class='%s'>",
> -		      !sorted && section ? "sublevel-repo" : "toplevel-repo");
> -		cgit_summary_link(ctx.repo->name, ctx.repo->name, NULL, NULL);
> -		html("</td><td>");
> -		html_link_open(cgit_repourl(ctx.repo->url), NULL, NULL);
> -		html_ntxt(ctx.cfg.max_repodesc_len, ctx.repo->desc);
> -		html_link_close();
> -		html("</td><td>");
> -		if (ctx.cfg.enable_index_owner) {
> -			if (ctx.repo->owner_filter) {
> -				cgit_open_filter(ctx.repo->owner_filter);
> -				html_txt(ctx.repo->owner);
> -				cgit_close_filter(ctx.repo->owner_filter);
> -			} else {
> -				html("<a href='");
> -				html_attr(cgit_currenturl());
> -				html("?q=");
> -				html_url_arg(ctx.repo->owner);
> -				html("'>");
> -				html_txt(ctx.repo->owner);
> -				html("</a>");
> -			}
> -			html("</td><td>");
> -		}
> -		print_modtime(ctx.repo);
> -		html("</td>");
> -		if (ctx.cfg.enable_index_links) {
> -			html("<td>");
> -			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_header();
> +
> +	/* Count visible repositories */
> +	repolist_walk_visible(generate_repolist, &repolist_ctx);
> +
>  	html("</table>");
> -	if (hits > ctx.cfg.max_repo_count)
> -		print_pager(hits, ctx.cfg.max_repo_count, ctx.qry.search, ctx.qry.sort);
> +	if (repolist_ctx.hits > ctx.cfg.max_repo_count)
> +		print_pager(repolist_ctx.hits, ctx.cfg.max_repo_count, ctx.qry.search, ctx.qry.sort);
>  	cgit_print_docend();
>  }
>  
> -- 
> 2.4.9
> 
> _______________________________________________
> CGit mailing list
> CGit at lists.zx2c4.com
> http://lists.zx2c4.com/mailman/listinfo/cgit

[PATCH 2/3] ui-repolist: Restructure internal logic to be more extensible

[tim.nordell]

On 02/28/16 07:02, John Keeping wrote:
> On Fri, Feb 26, 2016 at 02:58:58PM -0600, Tim Nordell wrote:
>> The internal logic has been restructured so that there is a "walking"
>> routine that filters the repo list based on the visible criteria, and
>> subsequently calls a given callback for each repo found.  Additionally,
>> split out generating a table line for a given repo, and a table line
>> for a given section.  This makes this more loosely coupled and allows
>> reuse of more logic for changes to the way the repo list is displayed.
>> This patch does not make any functional changes to the system and is
>> a reorganization of the existing logic only.
>
> There's a lot going on in this patch that makes it very hard to review.
> It would be easier if it were broken down into:
>
> 	extract struct repolist_ctx
> 	extract the new generate_repolist() function
> 	add repolist_walk_visible()
>

Thanks for the initial glance and suggestion.  I'll break it apart along 
these lines.

> It would also be helpful to mention in the commit message why
> repolist_walk_visible() is needed: in a following patch we will be
> adding a variant that prints only section headers.

Noted.

> There are also various style problems here, but they're similar to
> earlier patches and the Linux kernel's scripts/checkpatch.pl script
> catches them.

Yeah, I didn't realize that CGIT was following the kernel's style guide. 
  I looked in CGIT's folder, but I didn't expect it to be following 
documentation inside the submodule.  I'll grab the kernel's style 
checker and vet the changes against that.

- Tim

[PATCH 3/3] ui-repolist: Support a trimmed view when several sections are present

[tim.nordell]

Trim the view to just a section list if there are multiple
sections present in the output view.  This is only really useful
if one also has a script placing links on each section heading to
trim the view by path.

Signed-off-by: Tim Nordell <tim.nordell at logicpd.com>

diff --git a/cgitrc.5.txt b/cgitrc.5.txt
index 113b7a0..2658e89 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -413,9 +413,14 @@ section-filter::
 
 section-sort::
 	Flag which, when set to "1", will sort the sections on the repository
-	listing by name. Set this flag to "0" if the order in the cgitrc file should
-	be preserved. Default value: "1". See also: section,
-	case-sensitive-sort, repository-sort.
+	listing by name. When set to "2", this will collapse the listing of
+	repositories (when over the size max-repo-count) into just the list
+	of sections. This is only really useful if section filters are also
+	enabled, if the filter makes links into the URL that the section
+	comprises, and if all section headings are paths. Set this flag to
+	"0" if the order in the cgitrc file should be preserved.
+	Default value: "1". See also: section, case-sensitive-sort,
+	repository-sort, section-filter
 
 section-from-path::
 	A number which, if defined prior to scan-path, specifies how many
diff --git a/ui-repolist.c b/ui-repolist.c
index 6b06a1a..9045203 100644
--- a/ui-repolist.c
+++ b/ui-repolist.c
@@ -278,6 +278,8 @@ struct repolist_ctx
 	/* Used in interior context; should be reset in repolist_walk_visible() */
 	int hits;
 	const char *last_section;
+	int section_cnt;
+	int section_nested_cnt;
 };
 
 static void html_section(struct cgit_repo *repo, int columns)
@@ -374,6 +376,57 @@ static int generate_repolist(struct cgit_repo *repo, struct repolist_ctx *c)
 	return 0;
 }
 
+static int generate_sectionlist(struct cgit_repo *repo, struct repolist_ctx *c)
+{
+	bool is_toplevel;
+
+	is_toplevel = (NULL == repo->section || repo->section[0] == '\0');
+
+	if(!should_emit_section(repo, c) && !is_toplevel)
+		return 0;
+
+	c->hits++;
+
+	if (c->hits <= ctx.qry.ofs)
+		return 0;
+	if (c->hits > ctx.qry.ofs + ctx.cfg.max_repo_count)
+		return 0;
+
+	if(is_toplevel)
+		html_repository(repo, c->sorted);
+	else
+		html_section(repo, c->columns);
+
+	return 0;
+}
+
+static int count_sections(struct cgit_repo *repo, struct repolist_ctx *c)
+{
+	const char *last_section;
+
+	last_section = c->last_section;
+	if(should_emit_section(repo, c))
+	{
+		c->section_cnt++;
+
+		/* Determine if one section is nested within the other.  This
+		 * is only accurate if this is a sorted list.
+		 */
+		if(NULL != last_section && NULL != repo->section)
+		{
+			int spn = strspn(last_section, repo->section);
+			if(last_section[spn] == '\0')
+			{
+				c->section_nested_cnt++;
+			}
+		}
+	}
+
+	c->hits++;
+
+	return 0;
+}
+
 typedef int (*repolist_walk_callback_t)(struct cgit_repo *repo, struct repolist_ctx *c);
 static int repolist_walk_visible(repolist_walk_callback_t callback, struct repolist_ctx *c)
 {
@@ -382,6 +435,8 @@ static int repolist_walk_visible(repolist_walk_callback_t callback, struct repol
 
 	c->hits = 0;
 	c->last_section = NULL;
+	c->section_cnt = 0;
+	c->section_nested_cnt = 0;
 
 	for (i = 0; i < cgit_repolist.count; i++) {
 		repo = &cgit_repolist.repos[i];
@@ -395,6 +450,7 @@ static int repolist_walk_visible(repolist_walk_callback_t callback, struct repol
 
 void cgit_print_repolist(void)
 {
+	bool section_pages = false;
 	struct repolist_ctx repolist_ctx = {0};
 
 	repolist_ctx.columns = 3;
@@ -420,13 +476,24 @@ void cgit_print_repolist(void)
 	if (ctx.qry.sort)
 		repolist_ctx.sorted = sort_repolist(ctx.qry.sort);
 	else if (ctx.cfg.section_sort)
+	{
 		sort_repolist("section");
+		section_pages = (2 == ctx.cfg.section_sort);
+	}
 
 	html("<table summary='repository list' class='list nowrap'>");
 	print_header();
 
-	/* Count visible repositories */
-	repolist_walk_visible(generate_repolist, &repolist_ctx);
+	if(section_pages)
+		repolist_walk_visible(count_sections, &repolist_ctx);
+
+	if(section_pages && repolist_ctx.hits > ctx.cfg.max_repo_count &&
+		repolist_ctx.section_cnt - repolist_ctx.section_nested_cnt > 1)
+	{
+		repolist_walk_visible(generate_sectionlist, &repolist_ctx);
+	} else {
+		repolist_walk_visible(generate_repolist, &repolist_ctx);
+	}
 
 	html("</table>");
 	if (repolist_ctx.hits > ctx.cfg.max_repo_count)
-- 
2.4.9

[PATCH v2 0/8] ui-repolist: Support additional formatting for sections

[tim.nordell]

On our internal server we have over 2,000 repositories, primarily due to
mirroring external repositories locally.  We patched cgit to make navigation
a little easier by doing the following:

 - Make sections clickable (via a section filter)
 - Collapse repository listing to just sections when the repository listing
   is over 1 page worth of repositories

The patchset here contains this modification and allows it to be enabled
(or disabled) via configuration.

You can see an example of this UI over here: http://git.logicpd.com

(Note: That is running an older patched version not containing the changes
here presently but has the same presentation.)

Changes since v1:
 - Split apart patches into smaller discrete patches
 - Removed NULL check around open and close filter
 - Misc. style issues
 - Made parameter orders for repolist_ctx vs cgit_repo more consistent
   across routines

Tim Nordell (8):
  ui-repolist: Add section filter
  ui-repolist: Remove conditional around print_header()
  ui-repolist: Create context structure for cgit_print_repolist()
  ui-repolist: Move HTML generation into helper functions
  ui-repolist: Factor out logic for emitting a new section heading
  ui-repolist: Remove assignment of section to repolist_ctx
  ui-repolist: Restructure internal logic to be more extensible
  ui-repolist: Support a trimmed view when several sections are present

 cgit.c                      |   2 +
 cgit.h                      |   3 +-
 cgitrc.5.txt                |  12 ++
 filter.c                    |   1 +
 filters/section-example.lua |  33 ++++++
 ui-repolist.c               | 268 ++++++++++++++++++++++++++++++++------------
 6 files changed, 247 insertions(+), 72 deletions(-)
 create mode 100644 filters/section-example.lua

-- 
2.4.9

[PATCH v2 1/8] ui-repolist: Add section filter

[tim.nordell]

This allows custom links to be added into the section headers by
configuring a filter to be applied in the repository list.

Signed-off-by: Tim Nordell <tim.nordell at logicpd.com>

 create mode 100644 filters/section-example.lua

diff --git a/cgit.c b/cgit.c
index fc482be..3ef01cd 100644
--- a/cgit.c
+++ b/cgit.c
@@ -210,6 +210,8 @@ static void config_cb(const char *name, const char *value)
 		ctx.cfg.email_filter = cgit_new_filter(value, EMAIL);
 	else if (!strcmp(name, "owner-filter"))
 		ctx.cfg.owner_filter = cgit_new_filter(value, OWNER);
+	else if (!strcmp(name, "section-filter"))
+		ctx.cfg.section_filter = cgit_new_filter(value, SECTION);
 	else if (!strcmp(name, "auth-filter"))
 		ctx.cfg.auth_filter = cgit_new_filter(value, AUTH);
 	else if (!strcmp(name, "embedded"))
diff --git a/cgit.h b/cgit.h
index 325432b..74ef01c 100644
--- a/cgit.h
+++ b/cgit.h
@@ -55,7 +55,7 @@ typedef enum {
 } diff_type;
 
 typedef enum {
-	ABOUT, COMMIT, SOURCE, EMAIL, AUTH, OWNER
+	ABOUT, COMMIT, SOURCE, EMAIL, AUTH, OWNER, SECTION
 } filter_type;
 
 struct cgit_filter {
@@ -266,6 +266,7 @@ struct cgit_config {
 	struct cgit_filter *source_filter;
 	struct cgit_filter *email_filter;
 	struct cgit_filter *owner_filter;
+	struct cgit_filter *section_filter;
 	struct cgit_filter *auth_filter;
 };
 
diff --git a/cgitrc.5.txt b/cgitrc.5.txt
index 94901bd..de5ceab 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -404,6 +404,13 @@ section::
 	after this option will inherit the current section name. Default value:
 	none.
 
+section-filter::
+	Specifies a command which will be invoked to format section headings.
+	The command will get the section on its STDIN, and the STDOUT from the
+	command will be included verbatim as the section heading.
+	Default value: none.
+	See also: "FILTER API".
+
 section-sort::
 	Flag which, when set to "1", will sort the sections on the repository
 	listing by name. Set this flag to "0" if the order in the cgitrc file should
@@ -687,6 +694,11 @@ owner filter::
 	standard input and the filter is expected to write to standard
 	output.  The output is included in the Owner column.
 
+section filter::
+	This filter is given no arguments.  The section text is avilable on
+	standard input and the filter is expected to write to standard
+	output.  The output is included in the section heading.
+
 source filter::
 	This filter is given a single parameter: the filename of the source
 	file to filter. The filter can use the filename to determine (for
diff --git a/filter.c b/filter.c
index 949c931..952336f 100644
--- a/filter.c
+++ b/filter.c
@@ -436,6 +436,7 @@ struct cgit_filter *cgit_new_filter(const char *cmd, filter_type filtertype)
 			argument_count = 1;
 			break;
 
+		case SECTION:
 		case COMMIT:
 		default:
 			argument_count = 0;
diff --git a/filters/section-example.lua b/filters/section-example.lua
new file mode 100644
index 0000000..76076b2
--- /dev/null
+++ b/filters/section-example.lua
@@ -0,0 +1,33 @@
+-- This script is an example of an section-filter.  It replaces the
+-- section title with several links for each subdirectory represented
+-- within the section title.  (It's intended to be used where the section
+-- title is also the same as the path to the repository, similar to
+-- the output from the "section-from-path" option.)  This script may
+-- be used with the section-filter setting in cgitrc with the `lua:`
+-- prefix.
+
+function gen_link(name, path)
+end
+
+function filter_open()
+	buffer = ""
+end
+
+function filter_close()
+	path = "/"
+	cnt = 0
+	for i in string.gmatch(buffer, "[^/]+") do
+		if cnt > 0 then
+			html("/")
+		end
+		path = path .. i .. "/"
+		html(string.format("<a href=\"%s%s\" class=reposection>%s</a>",
+			os.getenv("SCRIPT_NAME"), path, i))
+		cnt = cnt + 1
+	end
+	return 0
+end
+
+function filter_write(str)
+	buffer = buffer .. str
+end
diff --git a/ui-repolist.c b/ui-repolist.c
index 30915df..771b330 100644
--- a/ui-repolist.c
+++ b/ui-repolist.c
@@ -322,7 +322,9 @@ void cgit_print_repolist(void)
 		     strcmp(section, last_section)))) {
 			htmlf("<tr class='nohover'><td colspan='%d' class='reposection'>",
 			      columns);
+			cgit_open_filter(ctx.cfg.section_filter);
 			html_txt(section);
+			cgit_close_filter(ctx.cfg.section_filter);
 			html("</td></tr>");
 			last_section = section;
 		}
-- 
2.4.9

[PATCH v2 2/8] ui-repolist: Remove conditional around print_header()

[tim.nordell]

The code has since morphed from the original use case of the
conditional for deciding whether or not to print the header.
This is guaranteed to always pass due to the earlier check
in the function "any_repos_visible()" which emits a 404
page.

Signed-off-by: Tim Nordell <tim.nordell at logicpd.com>

diff --git a/ui-repolist.c b/ui-repolist.c
index 771b330..c240af3 100644
--- a/ui-repolist.c
+++ b/ui-repolist.c
@@ -272,7 +272,7 @@ static int sort_repolist(char *field)
 
 void cgit_print_repolist(void)
 {
-	int i, columns = 3, hits = 0, header = 0;
+	int i, columns = 3, hits = 0;
 	char *last_section = NULL;
 	char *section;
 	int sorted = 0;
@@ -301,6 +301,7 @@ void cgit_print_repolist(void)
 		sort_repolist("section");
 
 	html("<table summary='repository list' class='list nowrap'>");
+	print_header();
 	for (i = 0; i < cgit_repolist.count; i++) {
 		ctx.repo = &cgit_repolist.repos[i];
 		if (!is_visible(ctx.repo))
@@ -310,8 +311,6 @@ void cgit_print_repolist(void)
 			continue;
 		if (hits > ctx.qry.ofs + ctx.cfg.max_repo_count)
 			continue;
-		if (!header++)
-			print_header();
 		section = ctx.repo->section;
 		if (section && !strcmp(section, ""))
 			section = NULL;
-- 
2.4.9

[PATCH v2 3/8] ui-repolist: Create context structure for cgit_print_repolist()

[tim.nordell]

This is in preparation of moving the code that iterates over the
repository list into a walk pattern.  This walk pattern will permit
other views on the repository list more easily, such as a section
only list.

Signed-off-by: Tim Nordell <tim.nordell at logicpd.com>

diff --git a/ui-repolist.c b/ui-repolist.c
index c240af3..6b751d2 100644
--- a/ui-repolist.c
+++ b/ui-repolist.c
@@ -269,13 +269,24 @@ static int sort_repolist(char *field)
 	return 0;
 }
 
+struct repolist_ctx {
+	int columns;
+	int sorted;
+	int hits;
+	const char *section;
+	const char *last_section;
+};
 
 void cgit_print_repolist(void)
 {
-	int i, columns = 3, hits = 0;
-	char *last_section = NULL;
-	char *section;
-	int sorted = 0;
+	struct repolist_ctx repolist_ctx;
+	struct repolist_ctx *c = &repolist_ctx;
+	int i;
+
+	repolist_ctx.columns      = 3;
+	repolist_ctx.hits         = 0;
+	repolist_ctx.last_section = NULL;
+	repolist_ctx.sorted       = 0;
 
 	if (!any_repos_visible()) {
 		cgit_print_error_page(404, "Not found", "No repositories found");
@@ -283,9 +294,9 @@ void cgit_print_repolist(void)
 	}
 
 	if (ctx.cfg.enable_index_links)
-		++columns;
+		++repolist_ctx.columns;
 	if (ctx.cfg.enable_index_owner)
-		++columns;
+		++repolist_ctx.columns;
 
 	ctx.page.title = ctx.cfg.root_title;
 	cgit_print_http_headers();
@@ -296,7 +307,7 @@ void cgit_print_repolist(void)
 		html_include(ctx.cfg.index_header);
 
 	if (ctx.qry.sort)
-		sorted = sort_repolist(ctx.qry.sort);
+		repolist_ctx.sorted = sort_repolist(ctx.qry.sort);
 	else if (ctx.cfg.section_sort)
 		sort_repolist("section");
 
@@ -306,29 +317,29 @@ void cgit_print_repolist(void)
 		ctx.repo = &cgit_repolist.repos[i];
 		if (!is_visible(ctx.repo))
 			continue;
-		hits++;
-		if (hits <= ctx.qry.ofs)
+		c->hits++;
+		if (c->hits <= ctx.qry.ofs)
 			continue;
-		if (hits > ctx.qry.ofs + ctx.cfg.max_repo_count)
+		if (c->hits > ctx.qry.ofs + ctx.cfg.max_repo_count)
 			continue;
-		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)))) {
+		c->section = ctx.repo->section;
+		if (c->section && !strcmp(c->section, ""))
+			c->section = NULL;
+		if (!c->sorted &&
+		    ((c->last_section == NULL && c->section != NULL) ||
+		    (repolist_ctx.last_section != NULL && c->section == NULL) ||
+		    (repolist_ctx.last_section != NULL && c->section != NULL &&
+		     strcmp(c->section, c->last_section)))) {
 			htmlf("<tr class='nohover'><td colspan='%d' class='reposection'>",
-			      columns);
+			      c->columns);
 			cgit_open_filter(ctx.cfg.section_filter);
-			html_txt(section);
+			html_txt(c->section);
 			cgit_close_filter(ctx.cfg.section_filter);
 			html("</td></tr>");
-			last_section = section;
+			c->last_section = c->section;
 		}
 		htmlf("<tr><td class='%s'>",
-		      !sorted && section ? "sublevel-repo" : "toplevel-repo");
+		      !c->sorted && c->section ? "sublevel-repo" : "toplevel-repo");
 		cgit_summary_link(ctx.repo->name, ctx.repo->name, NULL, NULL);
 		html("</td><td>");
 		html_link_open(cgit_repourl(ctx.repo->url), NULL, NULL);
@@ -364,8 +375,8 @@ void cgit_print_repolist(void)
 		html("</tr>\n");
 	}
 	html("</table>");
-	if (hits > ctx.cfg.max_repo_count)
-		print_pager(hits, ctx.cfg.max_repo_count, ctx.qry.search, ctx.qry.sort);
+	if (repolist_ctx.hits > ctx.cfg.max_repo_count)
+		print_pager(repolist_ctx.hits, ctx.cfg.max_repo_count, ctx.qry.search, ctx.qry.sort);
 	cgit_print_docend();
 }
 
-- 
2.4.9

[PATCH v2 4/8] ui-repolist: Move HTML generation into helper functions

[tim.nordell]

Move the code that generates the repository and section snippets
of html into their own helper functions.  This is for code reuse
in a later patch in this series.

This also moves some dependencies from the context out into
the caller instead of in the section of code moved.

Signed-off-by: Tim Nordell <tim.nordell at logicpd.com>

diff --git a/ui-repolist.c b/ui-repolist.c
index 6b751d2..38a42c7 100644
--- a/ui-repolist.c
+++ b/ui-repolist.c
@@ -277,6 +277,58 @@ struct repolist_ctx {
 	const char *last_section;
 };
 
+static void html_section(struct cgit_repo *repo, int columns)
+{
+	htmlf("<tr class='nohover'><td colspan='%d' class='reposection'>",
+		columns);
+	cgit_open_filter(ctx.cfg.section_filter);
+	html_txt(repo->section);
+	cgit_close_filter(ctx.cfg.section_filter);
+	html("</td></tr>");
+}
+
+static void html_repository(struct cgit_repo *repo, bool sorted)
+{
+	bool is_toplevel;
+
+	is_toplevel = (NULL != repo->section && '\0' != repo->section[0]);
+	htmlf("<tr><td class='%s'>",
+		(!sorted && is_toplevel) ? "sublevel-repo" : "toplevel-repo");
+	cgit_summary_link(repo->name, repo->name, NULL, NULL);
+	html("</td><td>");
+	html_link_open(cgit_repourl(repo->url), NULL, NULL);
+	html_ntxt(ctx.cfg.max_repodesc_len, repo->desc);
+	html_link_close();
+	html("</td><td>");
+	if (ctx.cfg.enable_index_owner) {
+		if (repo->owner_filter) {
+			cgit_open_filter(repo->owner_filter);
+			html_txt(repo->owner);
+			cgit_close_filter(repo->owner_filter);
+		} else {
+			html("<a href='");
+			html_attr(cgit_currenturl());
+			html("?q=");
+			html_url_arg(repo->owner);
+			html("'>");
+			html_txt(repo->owner);
+			html("</a>");
+		}
+		html("</td><td>");
+	}
+	print_modtime(repo);
+	html("</td>");
+	if (ctx.cfg.enable_index_links) {
+		html("<td>");
+		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");
+}
+
 void cgit_print_repolist(void)
 {
 	struct repolist_ctx repolist_ctx;
@@ -330,49 +382,10 @@ void cgit_print_repolist(void)
 		    (repolist_ctx.last_section != NULL && c->section == NULL) ||
 		    (repolist_ctx.last_section != NULL && c->section != NULL &&
 		     strcmp(c->section, c->last_section)))) {
-			htmlf("<tr class='nohover'><td colspan='%d' class='reposection'>",
-			      c->columns);
-			cgit_open_filter(ctx.cfg.section_filter);
-			html_txt(c->section);
-			cgit_close_filter(ctx.cfg.section_filter);
-			html("</td></tr>");
+			html_section(ctx.repo, c->columns);
 			c->last_section = c->section;
 		}
-		htmlf("<tr><td class='%s'>",
-		      !c->sorted && c->section ? "sublevel-repo" : "toplevel-repo");
-		cgit_summary_link(ctx.repo->name, ctx.repo->name, NULL, NULL);
-		html("</td><td>");
-		html_link_open(cgit_repourl(ctx.repo->url), NULL, NULL);
-		html_ntxt(ctx.cfg.max_repodesc_len, ctx.repo->desc);
-		html_link_close();
-		html("</td><td>");
-		if (ctx.cfg.enable_index_owner) {
-			if (ctx.repo->owner_filter) {
-				cgit_open_filter(ctx.repo->owner_filter);
-				html_txt(ctx.repo->owner);
-				cgit_close_filter(ctx.repo->owner_filter);
-			} else {
-				html("<a href='");
-				html_attr(cgit_currenturl());
-				html("?q=");
-				html_url_arg(ctx.repo->owner);
-				html("'>");
-				html_txt(ctx.repo->owner);
-				html("</a>");
-			}
-			html("</td><td>");
-		}
-		print_modtime(ctx.repo);
-		html("</td>");
-		if (ctx.cfg.enable_index_links) {
-			html("<td>");
-			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");
+		html_repository(ctx.repo, repolist_ctx.sorted);
 	}
 	html("</table>");
 	if (repolist_ctx.hits > ctx.cfg.max_repo_count)
-- 
2.4.9

[PATCH v2 5/8] ui-repolist: Factor out logic for emitting a new section heading

[tim.nordell]

For a later commit in this series, this logic is common in a couple
of places.  This simplifies the logic a bit instead of having several
nested checks.  Additionally, this patch removes the need for using
"!strcmp(c->section, "")" as this is equivalent to comparing
"'\0' == repo->section[0]".

Signed-off-by: Tim Nordell <tim.nordell at logicpd.com>

diff --git a/ui-repolist.c b/ui-repolist.c
index 38a42c7..df2dc92 100644
--- a/ui-repolist.c
+++ b/ui-repolist.c
@@ -329,6 +329,30 @@ static void html_repository(struct cgit_repo *repo, bool sorted)
 	html("</tr>\n");
 }
 
+static inline bool should_emit_section(struct repolist_ctx *c, struct cgit_repo *repo)
+{
+	/* If we're sorted, we will not have a new section emitted. */
+	if (c->sorted)
+		return false;
+
+	/* We need a valid repo section for the rest of the checks */
+	if (NULL == repo->section)
+		return false;
+
+	/* If the section title is blank (e.g. top-level), we never emit
+	 * a section heading. */
+	if ('\0' == repo->section[0])
+		return false;
+
+	/* Finally, compare the last section name to the current.  If they're
+	 * the same, do not emit a section area. */
+	if (NULL != c->last_section && !strcmp(repo->section, c->last_section))
+		return false;
+
+	c->last_section = repo->section;
+	return true;
+}
+
 void cgit_print_repolist(void)
 {
 	struct repolist_ctx repolist_ctx;
@@ -375,16 +399,8 @@ void cgit_print_repolist(void)
 		if (c->hits > ctx.qry.ofs + ctx.cfg.max_repo_count)
 			continue;
 		c->section = ctx.repo->section;
-		if (c->section && !strcmp(c->section, ""))
-			c->section = NULL;
-		if (!c->sorted &&
-		    ((c->last_section == NULL && c->section != NULL) ||
-		    (repolist_ctx.last_section != NULL && c->section == NULL) ||
-		    (repolist_ctx.last_section != NULL && c->section != NULL &&
-		     strcmp(c->section, c->last_section)))) {
+		if (should_emit_section(&repolist_ctx, ctx.repo))
 			html_section(ctx.repo, c->columns);
-			c->last_section = c->section;
-		}
 		html_repository(ctx.repo, repolist_ctx.sorted);
 	}
 	html("</table>");
-- 
2.4.9

[PATCH v2 6/8] ui-repolist: Remove assignment of section to repolist_ctx

[tim.nordell]

There is now only one user of this, emitting a html_section, and this
gets the repository passed in (which also contains a link to the
section heading).

Signed-off-by: Tim Nordell <tim.nordell at logicpd.com>

diff --git a/ui-repolist.c b/ui-repolist.c
index df2dc92..fab589e 100644
--- a/ui-repolist.c
+++ b/ui-repolist.c
@@ -273,7 +273,6 @@ struct repolist_ctx {
 	int columns;
 	int sorted;
 	int hits;
-	const char *section;
 	const char *last_section;
 };
 
@@ -398,7 +397,6 @@ void cgit_print_repolist(void)
 			continue;
 		if (c->hits > ctx.qry.ofs + ctx.cfg.max_repo_count)
 			continue;
-		c->section = ctx.repo->section;
 		if (should_emit_section(&repolist_ctx, ctx.repo))
 			html_section(ctx.repo, c->columns);
 		html_repository(ctx.repo, repolist_ctx.sorted);
-- 
2.4.9

[PATCH v2 7/8] ui-repolist: Restructure internal logic to be more extensible

[tim.nordell]

The internal logic has been restructured so that there is a "walking"
routine that filters the repo list based on the visible criteria, and
subsequently calls a given callback for each repo found.  Additionally,
split out generating a table line for a given repo, and a table line
for a given section.  This makes this more loosely coupled and allows
reuse of more logic for changes to the way the repo list is displayed.

This is for a future patch in this series.

Signed-off-by: Tim Nordell <tim.nordell at logicpd.com>

diff --git a/ui-repolist.c b/ui-repolist.c
index fab589e..93655d4 100644
--- a/ui-repolist.c
+++ b/ui-repolist.c
@@ -270,8 +270,12 @@ static int sort_repolist(char *field)
 }
 
 struct repolist_ctx {
+	/* From outer contex passed to interior */
 	int columns;
 	int sorted;
+
+	/* Used in interior context
+	 * (Should be reset in repolist_walk_visible()) */
 	int hits;
 	const char *last_section;
 };
@@ -290,6 +294,10 @@ static void html_repository(struct cgit_repo *repo, bool sorted)
 {
 	bool is_toplevel;
 
+	/* Note: cgit_summary_link() ultimately calls repolink(), which is
+	 * dependent on ctx.repo pointing to the repo we're working on */
+	ctx.repo = repo;
+
 	is_toplevel = (NULL != repo->section && '\0' != repo->section[0]);
 	htmlf("<tr><td class='%s'>",
 		(!sorted && is_toplevel) ? "sublevel-repo" : "toplevel-repo");
@@ -352,15 +360,45 @@ static inline bool should_emit_section(struct repolist_ctx *c, struct cgit_repo
 	return true;
 }
 
+static int generate_repolist(struct repolist_ctx *c, struct cgit_repo *repo)
+{
+	c->hits++;
+	if (c->hits <= ctx.qry.ofs)
+		return 0;
+	if (c->hits > ctx.qry.ofs + ctx.cfg.max_repo_count)
+		return 0;
+
+	if (should_emit_section(c, repo))
+		html_section(repo, c->columns);
+	html_repository(repo, c->sorted);
+
+	return 0;
+}
+
+typedef int (*repolist_walk_callback_t)(struct repolist_ctx *c, struct cgit_repo *repo);
+static int repolist_walk_visible(struct repolist_ctx *c, repolist_walk_callback_t callback)
+{
+	struct cgit_repo *repo;
+	int i;
+
+	c->hits = 0;
+	c->last_section = NULL;
+
+	for (i = 0; i < cgit_repolist.count; i++) {
+		repo = &cgit_repolist.repos[i];
+		if (!is_visible(repo))
+			continue;
+		if (NULL != callback)
+			callback(c, repo);
+	}
+	return 0;
+}
+
 void cgit_print_repolist(void)
 {
 	struct repolist_ctx repolist_ctx;
-	struct repolist_ctx *c = &repolist_ctx;
-	int i;
 
 	repolist_ctx.columns      = 3;
-	repolist_ctx.hits         = 0;
-	repolist_ctx.last_section = NULL;
 	repolist_ctx.sorted       = 0;
 
 	if (!any_repos_visible()) {
@@ -388,19 +426,7 @@ void cgit_print_repolist(void)
 
 	html("<table summary='repository list' class='list nowrap'>");
 	print_header();
-	for (i = 0; i < cgit_repolist.count; i++) {
-		ctx.repo = &cgit_repolist.repos[i];
-		if (!is_visible(ctx.repo))
-			continue;
-		c->hits++;
-		if (c->hits <= ctx.qry.ofs)
-			continue;
-		if (c->hits > ctx.qry.ofs + ctx.cfg.max_repo_count)
-			continue;
-		if (should_emit_section(&repolist_ctx, ctx.repo))
-			html_section(ctx.repo, c->columns);
-		html_repository(ctx.repo, repolist_ctx.sorted);
-	}
+	repolist_walk_visible(&repolist_ctx, generate_repolist);
 	html("</table>");
 	if (repolist_ctx.hits > ctx.cfg.max_repo_count)
 		print_pager(repolist_ctx.hits, ctx.cfg.max_repo_count, ctx.qry.search, ctx.qry.sort);
-- 
2.4.9

[PATCH v2 8/8] ui-repolist: Support a trimmed view when several sections are present

[tim.nordell]

Trim the view to just a section list if there are multiple
sections present in the output view.  This is only really useful
if one also has a script placing links on each section heading to
trim the view by path.

Signed-off-by: Tim Nordell <tim.nordell at logicpd.com>

diff --git a/ui-repolist.c b/ui-repolist.c
index 93655d4..ff70ddc 100644
--- a/ui-repolist.c
+++ b/ui-repolist.c
@@ -278,6 +278,8 @@ struct repolist_ctx {
 	 * (Should be reset in repolist_walk_visible()) */
 	int hits;
 	const char *last_section;
+	int section_cnt;
+	int section_nested_cnt;
 };
 
 static void html_section(struct cgit_repo *repo, int columns)
@@ -375,6 +377,51 @@ static int generate_repolist(struct repolist_ctx *c, struct cgit_repo *repo)
 	return 0;
 }
 
+static int generate_sectionlist(struct repolist_ctx *c, struct cgit_repo *repo)
+{
+	bool is_toplevel;
+
+	is_toplevel = (NULL == repo->section || repo->section[0] == '\0');
+
+	if (!should_emit_section(c, repo) && !is_toplevel)
+		return 0;
+
+	c->hits++;
+
+	if (c->hits <= ctx.qry.ofs)
+		return 0;
+	if (c->hits > ctx.qry.ofs + ctx.cfg.max_repo_count)
+		return 0;
+
+	if (is_toplevel)
+		html_repository(repo, c->sorted);
+	else
+		html_section(repo, c->columns);
+
+	return 0;
+}
+
+static int count_sections(struct repolist_ctx *c, struct cgit_repo *repo)
+{
+	const char *last_section;
+
+	last_section = c->last_section;
+	if (should_emit_section(c, repo)) {
+		c->section_cnt++;
+
+		/* Determine if one section is nested within the other.  This
+		 * is only accurate if this is a sorted list.
+		 */
+		if (NULL != last_section && NULL != repo->section &&
+		    last_section[strspn(last_section, repo->section)] == '\0')
+			c->section_nested_cnt++;
+	}
+
+	c->hits++;
+
+	return 0;
+}
+
 typedef int (*repolist_walk_callback_t)(struct repolist_ctx *c, struct cgit_repo *repo);
 static int repolist_walk_visible(struct repolist_ctx *c, repolist_walk_callback_t callback)
 {
@@ -383,6 +430,8 @@ static int repolist_walk_visible(struct repolist_ctx *c, repolist_walk_callback_
 
 	c->hits = 0;
 	c->last_section = NULL;
+	c->section_cnt = 0;
+	c->section_nested_cnt = 0;
 
 	for (i = 0; i < cgit_repolist.count; i++) {
 		repo = &cgit_repolist.repos[i];
@@ -396,6 +445,7 @@ static int repolist_walk_visible(struct repolist_ctx *c, repolist_walk_callback_
 
 void cgit_print_repolist(void)
 {
+	bool section_pages = false;
 	struct repolist_ctx repolist_ctx;
 
 	repolist_ctx.columns      = 3;
@@ -421,12 +471,23 @@ void cgit_print_repolist(void)
 
 	if (ctx.qry.sort)
 		repolist_ctx.sorted = sort_repolist(ctx.qry.sort);
-	else if (ctx.cfg.section_sort)
+	else if (ctx.cfg.section_sort) {
 		sort_repolist("section");
+		section_pages = (2 == ctx.cfg.section_sort);
+	}
 
 	html("<table summary='repository list' class='list nowrap'>");
 	print_header();
-	repolist_walk_visible(&repolist_ctx, generate_repolist);
+
+	if (section_pages)
+		repolist_walk_visible(&repolist_ctx, count_sections);
+
+	if (section_pages && repolist_ctx.hits > ctx.cfg.max_repo_count &&
+		repolist_ctx.section_cnt - repolist_ctx.section_nested_cnt > 1)
+		repolist_walk_visible(&repolist_ctx, generate_sectionlist);
+	else
+		repolist_walk_visible(&repolist_ctx, generate_repolist);
+
 	html("</table>");
 	if (repolist_ctx.hits > ctx.cfg.max_repo_count)
 		print_pager(repolist_ctx.hits, ctx.cfg.max_repo_count, ctx.qry.search, ctx.qry.sort);
-- 
2.4.9