Hello,

On 2023/11/27 13:00:35 +0530, Dhruvin Gandhi <contact@dhruvin.dev> wrote:
> Gitweb supports these per-repository configuration files [1]:
> 
> [x] description
> [ ] category
> [x] cloneurl
> [x] gitweb.owner (or gotweb.owner)
> 
> Note: x means the feature is present in gotwebd.
> 
> This diff adds support for labeling repos with a category. Currently, category is shown
> wherever the description is shown.

Thanks for working on this, I think it's a nice feature to be able to
"enrich" the listing with the categories, and something I've missed in
gotwebd.

> Rationale behind using a category column in index:
> 
> gotwebd list pages have pagination implemented. When introducting categories
> web can employ below approaches:
> 
> 1. Paginate first, and then group <per-page> entries by category:
> 	This works in practice, but each page has varying number of categories
> 	at various indices. To me, it looks a bit out of place.

agreed, it would be confusing as well.

> 2. Categorize first, and then paginate <all> entries starting from
> alphabetically sorted categories:
> 	This brings up all repos under "archived" category to the first page. I
> 	believe this is undesirable.

IMHO this is the "best" one.  "archived" category will show up at the
top no matter what, unless we also allow to somehow change the ordering
of the categories, which may be a bit awkward.

> 3. No grouping, just label repositories with category metadata:
> 	This is quite simple to implement. We can, in a later commit, allow filtering 
> 	repositories by category.

anyway I think going with this could be viable, at least initially.

> These ideas are subject to my understanding of how the user inteface should look like
> and I'm open to change them.

From my understanding 2) is what cgit is doing too, see for example
<https://git.causal.agency/> (I don't know a gitweb instance with
categories enabled), and it's the one I personally prefer too.  Assuming
there is no pagination, the result is quite cool.  With pagination, we
might "break" the categories across pages, but I don't see other ways
around.  I think 1) would be just annoying and confusing.

I understand the concern about having "archived" in the first page: with
pagination turned on it means that we show on the first page the less
interesting repositories.  I'm not sure how cgit or gitweb are addressing
this problem either, given that both support pagination.

This makes me start to question the usefullness of paging the
repositories, or at least having it enabled by default.  (I'm biased
though since I've always ran my instance without pagination)

> [1] https://git-scm.com/docs/gitweb#_per_repository_gitweb_configuration
> 
> PS: This is my first contribution to gameoftrees. Let me know if I got anything
> wrong while sending a diff to mailing list.

Thanks for contributing! :)

This is indeed the right mailing list, and the patch looks correct to
me.  I've annotated one nit below.

> blob - 207d9f20026688bdc8849f0125e73b94a285504e
> blob + dfed531e40bc3b2b0d3b5103b2e9b4c19943d972
> --- gotwebd/parse.y
> +++ gotwebd/parse.y
> @@ -386,6 +386,9 @@ serveropts1	: REPOS_PATH STRING {
>  		| SHOW_REPO_DESCRIPTION boolean {
>  			new_srv->show_repo_description = $2;
>  		}
> +		| SHOW_REPO_CATEGORY boolean {

nit: when using a new token, it needs to be defined in the %token list
too.  Our yacc only shows a warning

% touch parse.y
% make
[...]
/home/op/w/got/gotwebd/parse.y: the symbol SHOW_REPO_CATEGORY is undefined

but IIRC other implementations will (rightfully) error upon unknown
tokens.

> +			new_srv->show_repo_category = $2;
> +		}
>  		| SHOW_REPO_CLONEURL boolean {
>  			new_srv->show_repo_cloneurl = $2;
>  		}
> @@ -470,6 +473,7 @@ lookup(char *s)
>  		{ "respect_exportok",		RESPECT_EXPORTOK },
>  		{ "server",			SERVER },
>  		{ "show_repo_age",		SHOW_REPO_AGE },
> +		{ "show_repo_category",		SHOW_REPO_CATEGORY },
>  		{ "show_repo_cloneurl",		SHOW_REPO_CLONEURL },
>  		{ "show_repo_description",	SHOW_REPO_DESCRIPTION },
>  		{ "show_repo_owner",		SHOW_REPO_OWNER },
> @@ -904,6 +908,7 @@ conf_new_server(const char *name)
>  	srv->show_repo_owner = D_SHOWROWNER;
>  	srv->show_repo_age = D_SHOWAGE;
>  	srv->show_repo_description = D_SHOWDESC;
> +	srv->show_repo_category = D_SHOWCAT;
>  	srv->show_repo_cloneurl = D_SHOWURL;
>  	srv->respect_exportok = D_RESPECTEXPORTOK;
>