On Mon, Dec 07, 2015 at 02:28:09PM -0500, Peter Colberg wrote: > --- This is missing your sign-off. See [1] for what this means. It would also be useful to include some of the rationale from your cover letter in the patch itself, particularly the motivation for the change.
I would normally expect this to be split into two patches; one to extract the new "is_visible()" function and the other to add the new feature. This makes reviewers' lives easier. The patch itself looks good; I would probably have extracted the for loop into a "any_repos_visible()" function, which would avoid polluting the entire cgit_print_repolist() with two new variables that are only used in the first few lines, but the patch in its current form is also OK. [1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?h=v4.3#n409 > ui-repolist.c | 32 +++++++++++++++++++++++++------- > 1 file changed, 25 insertions(+), 7 deletions(-) > > diff --git a/ui-repolist.c b/ui-repolist.c > index a2e9e07..f86c6b7 100644 > --- a/ui-repolist.c > +++ b/ui-repolist.c > @@ -106,6 +106,15 @@ static int is_in_url(struct cgit_repo *repo) > return 0; > } > > +static int is_visible(struct cgit_repo *repo) > +{ > + if (repo->hide || repo->ignore) > + return 0; > + if (!(is_match(repo) && is_in_url(repo))) > + return 0; > + return 1; > +} > + > static void print_sort_header(const char *title, const char *sort) > { > char *currenturl = cgit_currenturl(); > @@ -252,10 +261,23 @@ static int sort_repolist(char *field) > > void cgit_print_repolist(void) > { > - int i, columns = 3, hits = 0, header = 0; > + int i, columns = 3, found = 0, hits = 0, header = 0; > char *last_section = NULL; > char *section; > int sorted = 0; > + struct cgit_repo *repo; > + > + for (i = 0; i < cgit_repolist.count; i++) { > + repo = &cgit_repolist.repos[i]; > + if (is_visible(repo)) { > + found = 1; > + break; > + } > + } > + if (!found) { > + cgit_print_error_page(404, "Not found", "No repositories > found"); > + return; > + } > > if (ctx.cfg.enable_index_links) > ++columns; > @@ -278,9 +300,7 @@ void cgit_print_repolist(void) > html("<table summary='repository list' class='list nowrap'>"); > for (i = 0; i < cgit_repolist.count; i++) { > ctx.repo = &cgit_repolist.repos[i]; > - if (ctx.repo->hide || ctx.repo->ignore) > - continue; > - if (!(is_match(ctx.repo) && is_in_url(ctx.repo))) > + if (!is_visible(ctx.repo)) > continue; > hits++; > if (hits <= ctx.qry.ofs) > @@ -340,9 +360,7 @@ void cgit_print_repolist(void) > html("</tr>\n"); > } > html("</table>"); > - if (!hits) > - cgit_print_error("No repositories found"); > - else if (hits > ctx.cfg.max_repo_count) > + if (hits > ctx.cfg.max_repo_count) > print_pager(hits, ctx.cfg.max_repo_count, ctx.qry.search, > ctx.qry.sort); > cgit_print_docend(); > } > -- > 2.6.2 > > _______________________________________________ > CGit mailing list > [email protected] > http://lists.zx2c4.com/mailman/listinfo/cgit _______________________________________________ CGit mailing list [email protected] http://lists.zx2c4.com/mailman/listinfo/cgit
