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

[andy.doan at linaro.org (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);