[PATCH 2/3] ui-repolist: Restructure internal logic to be more extensible
John Keeping
john at keeping.me.uk
Sun Feb 28 14:02:42 CET 2016
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
More information about the CGit
mailing list