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

[John Keeping]

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);
>