[PATCH] ui-repolist: provide hyperlinks on section names

[PATCH] ui-repolist: provide hyperlinks on section names

[andy.doan]

This makes it easier to traverse into a section of git repositories.

Signed-off-by: Andy Doan <andy.doan at linaro.org>
---
 ui-repolist.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ui-repolist.c b/ui-repolist.c
index 30915df..f6b6b47 100644
--- a/ui-repolist.c
+++ b/ui-repolist.c
@@ -322,7 +322,7 @@ void cgit_print_repolist(void)
 		     strcmp(section, last_section)))) {
 			htmlf("<tr class='nohover'><td colspan='%d' class='reposection'>",
 			      columns);
-			html_txt(section);
+			htmlf("<a href='%s'>%s</a>", section, section);
 			html("</td></tr>");
 			last_section = section;
 		}
--
2.7.4

[PATCH] ui-repolist: Allow sections to be collapsible

[andy.doan]

The index page can be difficult to navigate for really large git
servers. This change allows a configuration like:

 section-collapse=people
 section-collapse=tests

And an index page would only display the "people" and "tests" section
headers entries (not their repos) with a hyperlink that can be used to
drill down into each section.

Signed-off-by: Andy Doan <andy.doan at linaro.org>
---
 cgit.c        | 33 +++++++++++++++++++++++++++++----
 cgit.h        | 11 +++++++++--
 cgitrc.5.txt  |  5 +++++
 scan-tree.c   |  6 +++---
 shared.c      |  5 ++++-
 ui-repolist.c | 29 ++++++++++++++++-------------
 6 files changed, 66 insertions(+), 23 deletions(-)

diff --git a/cgit.c b/cgit.c
index 9427c4a..477c920 100644
--- a/cgit.c
+++ b/cgit.c
@@ -19,6 +19,12 @@

 const char *cgit_version = CGIT_VERSION;

+struct cgit_section_list {
+	struct cgit_section section;
+	struct cgit_section_list *next;
+};
+static struct cgit_section_list *sections = NULL;
+
 static void add_mimetype(const char *name, const char *value)
 {
 	struct string_list_item *item;
@@ -77,7 +83,7 @@ static void repo_config(struct cgit_repo *repo, const char *name, const char *va
 		item = string_list_append(&repo->submodules, xstrdup(name + 12));
 		item->util = xstrdup(value);
 	} else if (!strcmp(name, "section"))
-		repo->section = xstrdup(value);
+		repo->section = get_or_create_section(value);
 	else if (!strcmp(name, "readme") && value != NULL) {
 		if (repo->readme.items == ctx.cfg.readme.items)
 			memset(&repo->readme, 0, sizeof(repo->readme));
@@ -107,7 +113,7 @@ static void repo_config(struct cgit_repo *repo, const char *name, const char *va
 static void config_cb(const char *name, const char *value)
 {
 	if (!strcmp(name, "section") || !strcmp(name, "repo.group"))
-		ctx.cfg.section = xstrdup(value);
+		ctx.cfg.section = get_or_create_section(value);
 	else if (!strcmp(name, "repo.url"))
 		ctx.repo = cgit_add_repo(value);
 	else if (ctx.repo && !strcmp(name, "repo.path"))
@@ -242,6 +248,8 @@ static void config_cb(const char *name, const char *value)
 		ctx.cfg.section_from_path = atoi(value);
 	else if (!strcmp(name, "repository-sort"))
 		ctx.cfg.repository_sort = xstrdup(value);
+	else if (!strcmp(name, "section-collapse"))
+		get_or_create_section(value)->collapse = 1;
 	else if (!strcmp(name, "section-sort"))
 		ctx.cfg.section_sort = atoi(value);
 	else if (!strcmp(name, "source-filter"))
@@ -385,7 +393,7 @@ static void prepare_context(void)
 	ctx.cfg.root_desc = "a fast webinterface for the git dscm";
 	ctx.cfg.scan_hidden_path = 0;
 	ctx.cfg.script_name = CGIT_SCRIPT_NAME;
-	ctx.cfg.section = "";
+	ctx.cfg.section = NULL;
 	ctx.cfg.repository_sort = "name";
 	ctx.cfg.section_sort = 1;
 	ctx.cfg.summary_branches = 10;
@@ -794,7 +802,7 @@ static void print_repo(FILE *f, struct cgit_repo *repo)
 	if (repo->module_link)
 		fprintf(f, "repo.module-link=%s\n", repo->module_link);
 	if (repo->section)
-		fprintf(f, "repo.section=%s\n", repo->section);
+		fprintf(f, "repo.section=%s\n", repo->section->name);
 	if (repo->homepage)
 		fprintf(f, "repo.homepage=%s\n", repo->homepage);
 	if (repo->clone_url)
@@ -1026,6 +1034,23 @@ static int calc_ttl(void)
 	return ctx.cfg.cache_repo_ttl;
 }

+struct cgit_section* get_or_create_section(const char *section)
+{
+	struct cgit_section_list *ptr = sections;
+	while(ptr) {
+		if (!strcmp(section, ptr->section.name))
+			return &ptr->section;
+		ptr = ptr->next;
+	}
+	/* Not found insert into head of list */
+	ptr = malloc(sizeof(struct cgit_section_list));
+	ptr->section.name = xstrdup(section);
+	ptr->section.collapse = 0;
+	ptr->next = sections;
+	sections = ptr;
+	return &ptr->section;
+}
+
 int main(int argc, const char **argv)
 {
 	const char *path;
diff --git a/cgit.h b/cgit.h
index 325432b..f334bb3 100644
--- a/cgit.h
+++ b/cgit.h
@@ -75,6 +75,11 @@ struct cgit_exec_filter {
 	int pid;
 };

+struct cgit_section {
+	unsigned int collapse : 1;
+	char *name;
+};
+
 struct cgit_repo {
 	char *url;
 	char *name;
@@ -85,7 +90,7 @@ struct cgit_repo {
 	char *defbranch;
 	char *module_link;
 	struct string_list readme;
-	char *section;
+	struct cgit_section *section;
 	char *clone_url;
 	char *logo;
 	char *logo_link;
@@ -208,7 +213,7 @@ struct cgit_config {
 	char *root_desc;
 	char *root_readme;
 	char *script_name;
-	char *section;
+	struct cgit_section *section;
 	char *repository_sort;
 	char *virtual_root;	/* Always ends with '/'. */
 	char *strict_export;
@@ -322,6 +327,8 @@ extern struct cgit_repolist cgit_repolist;
 extern struct cgit_context ctx;
 extern const struct cgit_snapshot_format cgit_snapshot_formats[];

+extern struct cgit_section* get_or_create_section(const char *section);
+
 extern char *cgit_default_repo_desc;
 extern struct cgit_repo *cgit_add_repo(const char *url);
 extern struct cgit_repo *cgit_get_repoinfo(const char *url);
diff --git a/cgitrc.5.txt b/cgitrc.5.txt
index 9fcf445..2762657 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -404,6 +404,11 @@ section::
 	after this option will inherit the current section name. Default value:
 	none.

+section-collapse::
+	Name of a section to "collapse" and not display on the index page.
+	Multiple config entries can be specified and each one will be
+	collapsed.
+
 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
diff --git a/scan-tree.c b/scan-tree.c
index 1cb4e5d..fa21fc4 100644
--- a/scan-tree.c
+++ b/scan-tree.c
@@ -164,10 +164,10 @@ static void add_repo(const char *base, struct strbuf *path, repo_config_fn fn)
 		}
 		if (slash && !n) {
 			*slash = '\0';
-			repo->section = xstrdup(rel.buf);
+			repo->section = get_or_create_section(rel.buf);
 			*slash = '/';
-			if (starts_with(repo->name, repo->section)) {
-				repo->name += strlen(repo->section);
+			if (starts_with(repo->name, repo->section->name)) {
+				repo->name += strlen(repo->section->name);
 				if (*repo->name == '/')
 					repo->name++;
 			}
diff --git a/shared.c b/shared.c
index a63633b..8348581 100644
--- a/shared.c
+++ b/shared.c
@@ -444,13 +444,16 @@ typedef struct {

 void cgit_prepare_repo_env(struct cgit_repo * repo)
 {
+	char *section = NULL;
+	if (repo->section)
+		section = repo->section->name;
 	cgit_env_var env_vars[] = {
 		{ .name = "CGIT_REPO_URL", .value = repo->url },
 		{ .name = "CGIT_REPO_NAME", .value = repo->name },
 		{ .name = "CGIT_REPO_PATH", .value = repo->path },
 		{ .name = "CGIT_REPO_OWNER", .value = repo->owner },
 		{ .name = "CGIT_REPO_DEFBRANCH", .value = repo->defbranch },
-		{ .name = "CGIT_REPO_SECTION", .value = repo->section },
+		{ .name = "CGIT_REPO_SECTION", .value = section },
 		{ .name = "CGIT_REPO_CLONE_URL", .value = repo->clone_url }
 	};
 	int env_var_count = ARRAY_SIZE(env_vars);
diff --git a/ui-repolist.c b/ui-repolist.c
index f6b6b47..81f5603 100644
--- a/ui-repolist.c
+++ b/ui-repolist.c
@@ -188,10 +188,16 @@ static int sort_section(const void *a, const void *b)
 {
 	const struct cgit_repo *r1 = a;
 	const struct cgit_repo *r2 = b;
+	const char *s1 = "";
+	const char *s2 = "";
 	int result;
 	time_t t;

-	result = cmp(r1->section, r2->section);
+	if (r1->section)
+		s1 = r1->section->name;
+	if (r2->section)
+		s2 = r2->section->name;
+	result = cmp(s1, s2);
 	if (!result) {
 		if (!strcmp(ctx.cfg.repository_sort, "age")) {
 			// get_repo_modtime caches the value in r->mtime, so we don't
@@ -273,8 +279,8 @@ static int sort_repolist(char *field)
 void cgit_print_repolist(void)
 {
 	int i, columns = 3, hits = 0, header = 0;
-	char *last_section = NULL;
-	char *section;
+	struct cgit_section *last_section = NULL;
+	struct cgit_section *section;
 	int sorted = 0;

 	if (!any_repos_visible()) {
@@ -294,12 +300,10 @@ void cgit_print_repolist(void)

 	if (ctx.cfg.index_header)
 		html_include(ctx.cfg.index_header);
-
 	if (ctx.qry.sort)
 		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];
@@ -313,19 +317,18 @@ void cgit_print_repolist(void)
 		if (!header++)
 			print_header();
 		section = ctx.repo->section;
-		if (section && !strcmp(section, ""))
+		if (section && !strcmp(section->name, ""))
 			section = NULL;
-		if (!sorted &&
-		    ((last_section == NULL && section != NULL) ||
-		    (last_section != NULL && section == NULL) ||
-		    (last_section != NULL && section != NULL &&
-		     strcmp(section, last_section)))) {
+		if (!sorted && section && last_section != section ) {
 			htmlf("<tr class='nohover'><td colspan='%d' class='reposection'>",
 			      columns);
-			htmlf("<a href='%s'>%s</a>", section, section);
+			htmlf("<a href='%s'>%s</a>", section->name, section->name);
 			html("</td></tr>");
-			last_section = section;
 		}
+		last_section = section;
+		if (section && section->collapse)
+			continue;
+
 		htmlf("<tr><td class='%s'>",
 		      !sorted && section ? "sublevel-repo" : "toplevel-repo");
 		cgit_summary_link(ctx.repo->name, ctx.repo->name, NULL, NULL);
--
2.7.4

[PATCH] ui-repolist: Allow sections to be collapsible

[john]

On Tue, Aug 09, 2016 at 04:53:07PM -0500, Andy Doan wrote:
> The index page can be difficult to navigate for really large git
> servers. This change allows a configuration like:
> 
>  section-collapse=people
>  section-collapse=tests
> 
> And an index page would only display the "people" and "tests" section
> headers entries (not their repos) with a hyperlink that can be used to
> drill down into each section.
> 
> Signed-off-by: Andy Doan <andy.doan at linaro.org>
> ---
>  cgit.c        | 33 +++++++++++++++++++++++++++++----
>  cgit.h        | 11 +++++++++--
>  cgitrc.5.txt  |  5 +++++
>  scan-tree.c   |  6 +++---
>  shared.c      |  5 ++++-
>  ui-repolist.c | 29 ++++++++++++++++-------------
>  6 files changed, 66 insertions(+), 23 deletions(-)
> 
> diff --git a/cgit.c b/cgit.c
> index 9427c4a..477c920 100644
> --- a/cgit.c
> +++ b/cgit.c
> @@ -19,6 +19,12 @@
> 
>  const char *cgit_version = CGIT_VERSION;
> 
> +struct cgit_section_list {
> +	struct cgit_section section;
> +	struct cgit_section_list *next;
> +};
> +static struct cgit_section_list *sections = NULL;

Should this list of sections live in ctx somewhere?

I also think it would be simpler to just add:

	struct cgit_section *next;

into the struct cgit_section and avoid this second level of wrapping.

> +
>  static void add_mimetype(const char *name, const char *value)
>  {
>  	struct string_list_item *item;
> @@ -77,7 +83,7 @@ static void repo_config(struct cgit_repo *repo, const char *name, const char *va
>  		item = string_list_append(&repo->submodules, xstrdup(name + 12));
>  		item->util = xstrdup(value);
>  	} else if (!strcmp(name, "section"))
> -		repo->section = xstrdup(value);
> +		repo->section = get_or_create_section(value);
>  	else if (!strcmp(name, "readme") && value != NULL) {
>  		if (repo->readme.items == ctx.cfg.readme.items)
>  			memset(&repo->readme, 0, sizeof(repo->readme));
> @@ -107,7 +113,7 @@ static void repo_config(struct cgit_repo *repo, const char *name, const char *va
>  static void config_cb(const char *name, const char *value)
>  {
>  	if (!strcmp(name, "section") || !strcmp(name, "repo.group"))
> -		ctx.cfg.section = xstrdup(value);
> +		ctx.cfg.section = get_or_create_section(value);
>  	else if (!strcmp(name, "repo.url"))
>  		ctx.repo = cgit_add_repo(value);
>  	else if (ctx.repo && !strcmp(name, "repo.path"))
> @@ -242,6 +248,8 @@ static void config_cb(const char *name, const char *value)
>  		ctx.cfg.section_from_path = atoi(value);
>  	else if (!strcmp(name, "repository-sort"))
>  		ctx.cfg.repository_sort = xstrdup(value);
> +	else if (!strcmp(name, "section-collapse"))
> +		get_or_create_section(value)->collapse = 1;
>  	else if (!strcmp(name, "section-sort"))
>  		ctx.cfg.section_sort = atoi(value);
>  	else if (!strcmp(name, "source-filter"))
> @@ -385,7 +393,7 @@ static void prepare_context(void)
>  	ctx.cfg.root_desc = "a fast webinterface for the git dscm";
>  	ctx.cfg.scan_hidden_path = 0;
>  	ctx.cfg.script_name = CGIT_SCRIPT_NAME;
> -	ctx.cfg.section = "";
> +	ctx.cfg.section = NULL;
>  	ctx.cfg.repository_sort = "name";
>  	ctx.cfg.section_sort = 1;
>  	ctx.cfg.summary_branches = 10;
> @@ -794,7 +802,7 @@ static void print_repo(FILE *f, struct cgit_repo *repo)
>  	if (repo->module_link)
>  		fprintf(f, "repo.module-link=%s\n", repo->module_link);
>  	if (repo->section)
> -		fprintf(f, "repo.section=%s\n", repo->section);
> +		fprintf(f, "repo.section=%s\n", repo->section->name);
>  	if (repo->homepage)
>  		fprintf(f, "repo.homepage=%s\n", repo->homepage);
>  	if (repo->clone_url)
> @@ -1026,6 +1034,23 @@ static int calc_ttl(void)
>  	return ctx.cfg.cache_repo_ttl;
>  }
> 
> +struct cgit_section* get_or_create_section(const char *section)
> +{
> +	struct cgit_section_list *ptr = sections;
> +	while(ptr) {
> +		if (!strcmp(section, ptr->section.name))
> +			return &ptr->section;
> +		ptr = ptr->next;
> +	}
> +	/* Not found insert into head of list */

Neat.  We optimize the common case of finding the same section twice in
a row.

> +	ptr = malloc(sizeof(struct cgit_section_list));

This should be xmalloc.  And it might be better to use sizeof(*ptr).

> +	ptr->section.name = xstrdup(section);
> +	ptr->section.collapse = 0;
> +	ptr->next = sections;
> +	sections = ptr;
> +	return &ptr->section;
> +}
> +
>  int main(int argc, const char **argv)
>  {
>  	const char *path;
> diff --git a/cgit.h b/cgit.h
> index 325432b..f334bb3 100644
> --- a/cgit.h
> +++ b/cgit.h
> @@ -75,6 +75,11 @@ struct cgit_exec_filter {
>  	int pid;
>  };
> 
> +struct cgit_section {
> +	unsigned int collapse : 1;
> +	char *name;
> +};
> +
>  struct cgit_repo {
>  	char *url;
>  	char *name;
> @@ -85,7 +90,7 @@ struct cgit_repo {
>  	char *defbranch;
>  	char *module_link;
>  	struct string_list readme;
> -	char *section;
> +	struct cgit_section *section;
>  	char *clone_url;
>  	char *logo;
>  	char *logo_link;
> @@ -208,7 +213,7 @@ struct cgit_config {
>  	char *root_desc;
>  	char *root_readme;
>  	char *script_name;
> -	char *section;
> +	struct cgit_section *section;
>  	char *repository_sort;
>  	char *virtual_root;	/* Always ends with '/'. */
>  	char *strict_export;
> @@ -322,6 +327,8 @@ extern struct cgit_repolist cgit_repolist;
>  extern struct cgit_context ctx;
>  extern const struct cgit_snapshot_format cgit_snapshot_formats[];
> 
> +extern struct cgit_section* get_or_create_section(const char *section);
> +
>  extern char *cgit_default_repo_desc;
>  extern struct cgit_repo *cgit_add_repo(const char *url);
>  extern struct cgit_repo *cgit_get_repoinfo(const char *url);
> diff --git a/cgitrc.5.txt b/cgitrc.5.txt
> index 9fcf445..2762657 100644
> --- a/cgitrc.5.txt
> +++ b/cgitrc.5.txt
> @@ -404,6 +404,11 @@ section::
>  	after this option will inherit the current section name. Default value:
>  	none.
> 
> +section-collapse::
> +	Name of a section to "collapse" and not display on the index page.
> +	Multiple config entries can be specified and each one will be
> +	collapsed.
> +
>  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
> diff --git a/scan-tree.c b/scan-tree.c
> index 1cb4e5d..fa21fc4 100644
> --- a/scan-tree.c
> +++ b/scan-tree.c
> @@ -164,10 +164,10 @@ static void add_repo(const char *base, struct strbuf *path, repo_config_fn fn)
>  		}
>  		if (slash && !n) {
>  			*slash = '\0';
> -			repo->section = xstrdup(rel.buf);
> +			repo->section = get_or_create_section(rel.buf);
>  			*slash = '/';
> -			if (starts_with(repo->name, repo->section)) {
> -				repo->name += strlen(repo->section);
> +			if (starts_with(repo->name, repo->section->name)) {
> +				repo->name += strlen(repo->section->name);
>  				if (*repo->name == '/')
>  					repo->name++;
>  			}
> diff --git a/shared.c b/shared.c
> index a63633b..8348581 100644
> --- a/shared.c
> +++ b/shared.c
> @@ -444,13 +444,16 @@ typedef struct {
> 
>  void cgit_prepare_repo_env(struct cgit_repo * repo)
>  {
> +	char *section = NULL;
> +	if (repo->section)
> +		section = repo->section->name;
>  	cgit_env_var env_vars[] = {
>  		{ .name = "CGIT_REPO_URL", .value = repo->url },
>  		{ .name = "CGIT_REPO_NAME", .value = repo->name },
>  		{ .name = "CGIT_REPO_PATH", .value = repo->path },
>  		{ .name = "CGIT_REPO_OWNER", .value = repo->owner },
>  		{ .name = "CGIT_REPO_DEFBRANCH", .value = repo->defbranch },
> -		{ .name = "CGIT_REPO_SECTION", .value = repo->section },
> +		{ .name = "CGIT_REPO_SECTION", .value = section },
>  		{ .name = "CGIT_REPO_CLONE_URL", .value = repo->clone_url }
>  	};
>  	int env_var_count = ARRAY_SIZE(env_vars);
> diff --git a/ui-repolist.c b/ui-repolist.c
> index f6b6b47..81f5603 100644
> --- a/ui-repolist.c
> +++ b/ui-repolist.c
> @@ -188,10 +188,16 @@ static int sort_section(const void *a, const void *b)
>  {
>  	const struct cgit_repo *r1 = a;
>  	const struct cgit_repo *r2 = b;
> +	const char *s1 = "";
> +	const char *s2 = "";
>  	int result;
>  	time_t t;
> 
> -	result = cmp(r1->section, r2->section);
> +	if (r1->section)
> +		s1 = r1->section->name;
> +	if (r2->section)
> +		s2 = r2->section->name;
> +	result = cmp(s1, s2);
>  	if (!result) {
>  		if (!strcmp(ctx.cfg.repository_sort, "age")) {
>  			// get_repo_modtime caches the value in r->mtime, so we don't
> @@ -273,8 +279,8 @@ static int sort_repolist(char *field)
>  void cgit_print_repolist(void)
>  {
>  	int i, columns = 3, hits = 0, header = 0;
> -	char *last_section = NULL;
> -	char *section;
> +	struct cgit_section *last_section = NULL;
> +	struct cgit_section *section;
>  	int sorted = 0;
> 
>  	if (!any_repos_visible()) {
> @@ -294,12 +300,10 @@ void cgit_print_repolist(void)
> 
>  	if (ctx.cfg.index_header)
>  		html_include(ctx.cfg.index_header);
> -
>  	if (ctx.qry.sort)
>  		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];
> @@ -313,19 +317,18 @@ void cgit_print_repolist(void)
>  		if (!header++)
>  			print_header();
>  		section = ctx.repo->section;
> -		if (section && !strcmp(section, ""))
> +		if (section && !strcmp(section->name, ""))
>  			section = NULL;
> -		if (!sorted &&
> -		    ((last_section == NULL && section != NULL) ||
> -		    (last_section != NULL && section == NULL) ||
> -		    (last_section != NULL && section != NULL &&
> -		     strcmp(section, last_section)))) {
> +		if (!sorted && section && last_section != section ) {

Shouldn't this just be:

	if (!sorted && last_section != section)

?  Otherwise we're missing the case where section is NULL and
last_section is non-null.

>  			htmlf("<tr class='nohover'><td colspan='%d' class='reposection'>",
>  			      columns);
> -			htmlf("<a href='%s'>%s</a>", section, section);
> +			htmlf("<a href='%s'>%s</a>", section->name, section->name);
>  			html("</td></tr>");
> -			last_section = section;
>  		}
> +		last_section = section;
> +		if (section && section->collapse)
> +			continue;
> +
>  		htmlf("<tr><td class='%s'>",
>  		      !sorted && section ? "sublevel-repo" : "toplevel-repo");
>  		cgit_summary_link(ctx.repo->name, ctx.repo->name, NULL, NULL);
> --
> 2.7.4

[PATCH] ui-repolist: Allow sections to be collapsible

[andy.doan]

On 08/25/2016 04:13 PM, John Keeping wrote:
> On Tue, Aug 09, 2016 at 04:53:07PM -0500, Andy Doan wrote:
>> The index page can be difficult to navigate for really large git
>> servers. This change allows a configuration like:
>>
>>  section-collapse=people
>>  section-collapse=tests
>>
>> And an index page would only display the "people" and "tests" section
>> headers entries (not their repos) with a hyperlink that can be used to
>> drill down into each section.
>>
>> Signed-off-by: Andy Doan <andy.doan at linaro.org>
>> ---
>>  cgit.c        | 33 +++++++++++++++++++++++++++++----
>>  cgit.h        | 11 +++++++++--
>>  cgitrc.5.txt  |  5 +++++
>>  scan-tree.c   |  6 +++---
>>  shared.c      |  5 ++++-
>>  ui-repolist.c | 29 ++++++++++++++++-------------
>>  6 files changed, 66 insertions(+), 23 deletions(-)
>>
>> diff --git a/cgit.c b/cgit.c
>> index 9427c4a..477c920 100644
>> --- a/cgit.c
>> +++ b/cgit.c
>> @@ -19,6 +19,12 @@
>>
>>  const char *cgit_version = CGIT_VERSION;
>>
>> +struct cgit_section_list {
>> +	struct cgit_section section;
>> +	struct cgit_section_list *next;
>> +};
>> +static struct cgit_section_list *sections = NULL;
>
> Should this list of sections live in ctx somewhere?

Its only needed from this one file, so I was trying to keep it static 
there. I'd be happy to move it into the ctx if that would make things 
more consistent with the rest of your project. Let me know which you prefer.

> I also think it would be simpler to just add:
>
> 	struct cgit_section *next;
>
> into the struct cgit_section and avoid this second level of wrapping.

agreed. will address this in v2.

>> diff --git a/ui-repolist.c b/ui-repolist.c
>> @@ -313,19 +317,18 @@ void cgit_print_repolist(void)
>>  		if (!header++)
>>  			print_header();
>>  		section = ctx.repo->section;
>> -		if (section && !strcmp(section, ""))
>> +		if (section && !strcmp(section->name, ""))
>>  			section = NULL;
>> -		if (!sorted &&
>> -		    ((last_section == NULL && section != NULL) ||
>> -		    (last_section != NULL && section == NULL) ||
>> -		    (last_section != NULL && section != NULL &&
>> -		     strcmp(section, last_section)))) {
>> +		if (!sorted && section && last_section != section ) {
>
> Shouldn't this just be:
>
> 	if (!sorted && last_section != section)
>
> ?  Otherwise we're missing the case where section is NULL and
> last_section is non-null.

I might be missing something subtle that doesn't match my use case. 
However, to enter this block you *must* have a non-null pointer to 
section since its trying to print a the section's name. I think in the 
event section became null and last_section wasn't you'd be okay because 
this block would get skipped, and then the repo would get displayed as a 
"toplevel-repo" and not appear in the previous section.

>>  			htmlf("<tr class='nohover'><td colspan='%d' class='reposection'>",
>>  			      columns);
>> -			htmlf("<a href='%s'>%s</a>", section, section);
>> +			htmlf("<a href='%s'>%s</a>", section->name, section->name);
>>  			html("</td></tr>");
>> -			last_section = section;
>>  		}
>> +		last_section = section;
>> +		if (section && section->collapse)
>> +			continue;
>> +
>>  		htmlf("<tr><td class='%s'>",
>>  		      !sorted && section ? "sublevel-repo" : "toplevel-repo");
>>  		cgit_summary_link(ctx.repo->name, ctx.repo->name, NULL, NULL);

[PATCH] ui-repolist: Allow sections to be collapsible

[john]

On Thu, Aug 25, 2016 at 10:20:58PM -0500, Andy Doan wrote:
> On 08/25/2016 04:13 PM, John Keeping wrote:
> > On Tue, Aug 09, 2016 at 04:53:07PM -0500, Andy Doan wrote:
> >> The index page can be difficult to navigate for really large git
> >> servers. This change allows a configuration like:
> >>
> >>  section-collapse=people
> >>  section-collapse=tests
> >>
> >> And an index page would only display the "people" and "tests" section
> >> headers entries (not their repos) with a hyperlink that can be used to
> >> drill down into each section.
> >>
> >> Signed-off-by: Andy Doan <andy.doan at linaro.org>
> >> ---
> >>  cgit.c        | 33 +++++++++++++++++++++++++++++----
> >>  cgit.h        | 11 +++++++++--
> >>  cgitrc.5.txt  |  5 +++++
> >>  scan-tree.c   |  6 +++---
> >>  shared.c      |  5 ++++-
> >>  ui-repolist.c | 29 ++++++++++++++++-------------
> >>  6 files changed, 66 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/cgit.c b/cgit.c
> >> index 9427c4a..477c920 100644
> >> --- a/cgit.c
> >> +++ b/cgit.c
> >> @@ -19,6 +19,12 @@
> >>
> >>  const char *cgit_version = CGIT_VERSION;
> >>
> >> +struct cgit_section_list {
> >> +	struct cgit_section section;
> >> +	struct cgit_section_list *next;
> >> +};
> >> +static struct cgit_section_list *sections = NULL;
> >
> > Should this list of sections live in ctx somewhere?
> 
> Its only needed from this one file, so I was trying to keep it static 
> there. I'd be happy to move it into the ctx if that would make things 
> more consistent with the rest of your project. Let me know which you prefer.

The rest of our global state is stored in ctx, so I think this belongs
there as well.

> > I also think it would be simpler to just add:
> >
> > 	struct cgit_section *next;
> >
> > into the struct cgit_section and avoid this second level of wrapping.
> 
> agreed. will address this in v2.
> 
> >> diff --git a/ui-repolist.c b/ui-repolist.c
> >> @@ -313,19 +317,18 @@ void cgit_print_repolist(void)
> >>  		if (!header++)
> >>  			print_header();
> >>  		section = ctx.repo->section;
> >> -		if (section && !strcmp(section, ""))
> >> +		if (section && !strcmp(section->name, ""))
> >>  			section = NULL;
> >> -		if (!sorted &&
> >> -		    ((last_section == NULL && section != NULL) ||
> >> -		    (last_section != NULL && section == NULL) ||
> >> -		    (last_section != NULL && section != NULL &&
> >> -		     strcmp(section, last_section)))) {
> >> +		if (!sorted && section && last_section != section ) {
> >
> > Shouldn't this just be:
> >
> > 	if (!sorted && last_section != section)
> >
> > ?  Otherwise we're missing the case where section is NULL and
> > last_section is non-null.
> 
> I might be missing something subtle that doesn't match my use case. 
> However, to enter this block you *must* have a non-null pointer to 
> section since its trying to print a the section's name. I think in the 
> event section became null and last_section wasn't you'd be okay because 
> this block would get skipped, and then the repo would get displayed as a 
> "toplevel-repo" and not appear in the previous section.

I was comparing against the condition that's been replaced here, which
does deal with this case, but you're right that the body isn't valid if
we hit that.  And in fact we won't ever hit that because if "!sorted"
then we have ordered the repositories by section and those with no
section always come first.

So the new code is correct, but it might deserve a comment in the commit
message to point out why the old condition is wrong.

> >>  			htmlf("<tr class='nohover'><td colspan='%d' class='reposection'>",
> >>  			      columns);
> >> -			htmlf("<a href='%s'>%s</a>", section, section);
> >> +			htmlf("<a href='%s'>%s</a>", section->name, section->name);
> >>  			html("</td></tr>");
> >> -			last_section = section;
> >>  		}
> >> +		last_section = section;
> >> +		if (section && section->collapse)
> >> +			continue;
> >> +
> >>  		htmlf("<tr><td class='%s'>",
> >>  		      !sorted && section ? "sublevel-repo" : "toplevel-repo");
> >>  		cgit_summary_link(ctx.repo->name, ctx.repo->name, NULL, NULL);
>