[PATCH] ui-repolist: return HTTP 404 if no repositories found

[PATCH] ui-repolist: return HTTP 404 if no repositories found

[peter]

Dear cgit maintainers,

With the following patch, cgit returns HTTP 404 Not found when querying
a non-existent repository. This is useful to signal to search engines
that a repository no longer exists. Further, webservers such as nginx
permit logging requests to different files depending on the HTTP code.

cgit_print_repolist() does an early return with an error page if no
repository is found. I added a helper function is_visible() to avoid
repeating the repo conditions (and risking them getting out of sync).

Regards,
Peter

Peter Colberg (1):
  ui-repolist: return HTTP 404 if no repositories found

 ui-repolist.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

-- 
2.6.2

[PATCH] ui-repolist: return HTTP 404 if no repositories found

[peter]

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

[PATCH] ui-repolist: return HTTP 404 if no repositories found

[john]

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
> CGit at lists.zx2c4.com
> http://lists.zx2c4.com/mailman/listinfo/cgit

[PATCH v2 0/2] return HTTP 404 if no repositories found

[peter]

On Mon, Dec 07, 2015 at 08:28:45PM +0000, John Keeping 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.

John, thank you for the helpful comments!

Regards,
Peter

Peter Colberg (2):
  ui-repolist: extract repo visibility criteria to separate function
  ui-repolist: return HTTP 404 if no repositories found

 ui-repolist.c | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

-- 
2.6.2

[PATCH v2 1/2] ui-repolist: extract repo visibility criteria to separate function

[peter]

Signed-off-by: Peter Colberg <peter at colberg.org>
---
 ui-repolist.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/ui-repolist.c b/ui-repolist.c
index a2e9e07..f3f5353 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();
@@ -278,9 +287,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)
-- 
2.6.2

[PATCH v2 2/2] ui-repolist: return HTTP 404 if no repositories found

[peter]

Return HTTP status code 404 Not found when querying a non-existent
repository, which signals to search engines that a repository no
longer exists. Further, some webservers such as nginx permit
logging requests to different files depending on the HTTP code.

Signed-off-by: Peter Colberg <peter at colberg.org>
---
 ui-repolist.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/ui-repolist.c b/ui-repolist.c
index f3f5353..95abb8d 100644
--- a/ui-repolist.c
+++ b/ui-repolist.c
@@ -115,6 +115,20 @@ static int is_visible(struct cgit_repo *repo)
 	return 1;
 }
 
+static int any_repos_visible()
+{
+	struct cgit_repo *repo;
+	int i;
+
+	for (i = 0; i < cgit_repolist.count; i++) {
+		repo = &cgit_repolist.repos[i];
+		if (is_visible(repo)) {
+			return 1;
+		}
+	}
+	return 0;
+}
+
 static void print_sort_header(const char *title, const char *sort)
 {
 	char *currenturl = cgit_currenturl();
@@ -266,6 +280,11 @@ void cgit_print_repolist(void)
 	char *section;
 	int sorted = 0;
 
+	if (!any_repos_visible()) {
+		cgit_print_error_page(404, "Not found", "No repositories found");
+		return;
+	}
+
 	if (ctx.cfg.enable_index_links)
 		++columns;
 	if (ctx.cfg.enable_index_owner)
@@ -347,9 +366,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

[PATCH v2 1/2] ui-repolist: extract repo visibility criteria to separate function

[Jason]

Merged this. Thanks for the helper function.

[PATCH v2 2/2] ui-repolist: return HTTP 404 if no repositories found

[Jason]

Thanks Peter. I merged this with some changes:

http://git.zx2c4.com/cgit/commit/?id=9abe4a26a92b91170cb9c5dab830b40fb1e0327f

Note that in C you need to specify (void) in the argument list if it
doesn't take any arguments. Otherwise it takes a variable number of
arguments.

[PATCH v2 2/2] ui-repolist: return HTTP 404 if no repositories found

[peter]

On Wed, Jan 13, 2016 at 05:21:21PM +0100, Jason A. Donenfeld wrote:
> Thanks Peter. I merged this with some changes:
>
> http://git.zx2c4.com/cgit/commit/?id=9abe4a26a92b91170cb9c5dab830b40fb1e0327f

Thanks for merging, Jason.

> Note that in C you need to specify (void) in the argument list if it
> doesn't take any arguments. Otherwise it takes a variable number of
> arguments.

I knew about (void) in C and still made this mistake.

The following commits add compiler warning flags to prevent this mistake
from happening ever again and fix various declarations without (void).

Regards,
Peter

Peter Colberg (2):
  Compile with -Wstrict-prototypes -Wmissing-prototypes
  Fix missing prototype declarations

 cgit.mk       |  5 +++++
 ui-diff.h     |  2 +-
 ui-refs.h     |  2 +-
 ui-repolist.h |  4 ++--
 ui-shared.h   | 12 ++++++------
 ui-ssdiff.h   |  8 ++++----
 ui-summary.h  |  2 +-
 7 files changed, 20 insertions(+), 15 deletions(-)

-- 
2.7.0.rc3

[PATCH 1/2] Compile with -Wstrict-prototypes -Wmissing-prototypes

[peter]

Signed-off-by: Peter Colberg <peter at colberg.org>
---
 cgit.mk | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/cgit.mk b/cgit.mk
index 1b50307..f8fc6b3 100644
--- a/cgit.mk
+++ b/cgit.mk
@@ -21,6 +21,11 @@ CGIT_CFLAGS += -DCGIT_CONFIG='"$(CGIT_CONFIG)"'
 CGIT_CFLAGS += -DCGIT_SCRIPT_NAME='"$(CGIT_SCRIPT_NAME)"'
 CGIT_CFLAGS += -DCGIT_CACHE_ROOT='"$(CACHE_ROOT)"'
 
+# Warn if function is declared or defined without argument types
+CGIT_CFLAGS += -Wstrict-prototypes
+# Warn if function is defined without previous declaration in header
+CGIT_CFLAGS += -Wmissing-prototypes
+
 ifdef NO_C99_FORMAT
 	CFLAGS += -DNO_C99_FORMAT
 endif
-- 
2.7.0.rc3

[PATCH 2/2] Fix missing prototype declarations

[peter]

Signed-off-by: Peter Colberg <peter at colberg.org>
---
 ui-diff.h     |  2 +-
 ui-refs.h     |  2 +-
 ui-repolist.h |  4 ++--
 ui-shared.h   | 12 ++++++------
 ui-ssdiff.h   |  8 ++++----
 ui-summary.h  |  2 +-
 6 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/ui-diff.h b/ui-diff.h
index 04f9029..382e8c5 100644
--- a/ui-diff.h
+++ b/ui-diff.h
@@ -1,7 +1,7 @@
 #ifndef UI_DIFF_H
 #define UI_DIFF_H
 
-extern void cgit_print_diff_ctrls();
+extern void cgit_print_diff_ctrls(void);
 
 extern void cgit_print_diff(const char *new_hex, const char *old_hex,
 			    const char *prefix, int show_ctrls, int raw);
diff --git a/ui-refs.h b/ui-refs.h
index b35c04a..1d4a54a 100644
--- a/ui-refs.h
+++ b/ui-refs.h
@@ -3,6 +3,6 @@
 
 extern void cgit_print_branches(int maxcount);
 extern void cgit_print_tags(int maxcount);
-extern void cgit_print_refs();
+extern void cgit_print_refs(void);
 
 #endif /* UI_REFS_H */
diff --git a/ui-repolist.h b/ui-repolist.h
index 5b1e542..1b6b322 100644
--- a/ui-repolist.h
+++ b/ui-repolist.h
@@ -1,7 +1,7 @@
 #ifndef UI_REPOLIST_H
 #define UI_REPOLIST_H
 
-extern void cgit_print_repolist();
-extern void cgit_print_site_readme();
+extern void cgit_print_repolist(void);
+extern void cgit_print_site_readme(void);
 
 #endif /* UI_REPOLIST_H */
diff --git a/ui-shared.h b/ui-shared.h
index 474e0c5..de08e1b 100644
--- a/ui-shared.h
+++ b/ui-shared.h
@@ -1,11 +1,11 @@
 #ifndef UI_SHARED_H
 #define UI_SHARED_H
 
-extern const char *cgit_httpscheme();
-extern char *cgit_hosturl();
-extern const char *cgit_rooturl();
-extern char *cgit_currenturl();
-extern const char *cgit_loginurl();
+extern const char *cgit_httpscheme(void);
+extern char *cgit_hosturl(void);
+extern const char *cgit_rooturl(void);
+extern char *cgit_currenturl(void);
+extern const char *cgit_loginurl(void);
 extern char *cgit_repourl(const char *reponame);
 extern char *cgit_fileurl(const char *reponame, const char *pagename,
 			  const char *filename, const char *query);
@@ -66,7 +66,7 @@ extern void cgit_print_age(time_t t, time_t max_relative, const char *format);
 extern void cgit_print_http_headers(void);
 extern void cgit_redirect(const char *url, bool permanent);
 extern void cgit_print_docstart(void);
-extern void cgit_print_docend();
+extern void cgit_print_docend(void);
 __attribute__((format (printf,3,4)))
 extern void cgit_print_error_page(int code, const char *msg, const char *fmt, ...);
 extern void cgit_print_pageheader(void);
diff --git a/ui-ssdiff.h b/ui-ssdiff.h
index 88627e2..11f2714 100644
--- a/ui-ssdiff.h
+++ b/ui-ssdiff.h
@@ -13,13 +13,13 @@
 #endif
 #define MAX_SSDIFF_SIZE ((MAX_SSDIFF_M) * (MAX_SSDIFF_N))
 
-extern void cgit_ssdiff_print_deferred_lines();
+extern void cgit_ssdiff_print_deferred_lines(void);
 
 extern void cgit_ssdiff_line_cb(char *line, int len);
 
-extern void cgit_ssdiff_header_begin();
-extern void cgit_ssdiff_header_end();
+extern void cgit_ssdiff_header_begin(void);
+extern void cgit_ssdiff_header_end(void);
 
-extern void cgit_ssdiff_footer();
+extern void cgit_ssdiff_footer(void);
 
 #endif /* UI_SSDIFF_H */
diff --git a/ui-summary.h b/ui-summary.h
index c01f560..0896650 100644
--- a/ui-summary.h
+++ b/ui-summary.h
@@ -1,7 +1,7 @@
 #ifndef UI_SUMMARY_H
 #define UI_SUMMARY_H
 
-extern void cgit_print_summary();
+extern void cgit_print_summary(void);
 extern void cgit_print_repo_readme(char *path);
 
 #endif /* UI_SUMMARY_H */
-- 
2.7.0.rc3

[PATCH 1/2] Compile with -Wstrict-prototypes -Wmissing-prototypes

[john]

On Wed, Jan 13, 2016 at 05:25:06PM -0500, Peter Colberg wrote:
> Signed-off-by: Peter Colberg <peter at colberg.org>
> ---

I don't think we want to do this in the generic makefile.  Not all
compilers will support these options so we can't blindly add them to
CFLAGS.  Especially not CGIT_CFLAGS which is designed to be options to
compile the program; this is more along the lines of user customization
which would go in CFLAGS (which is expected to be overridden in the
environment or on the make command line) before being captured into
CGIT_CFLAGS.

You can update CFLAGS in cgit.conf or git/config.mak for site-specific
customization.  Maybe Ferry can set that up on his Jenkins instance.

>  cgit.mk | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/cgit.mk b/cgit.mk
> index 1b50307..f8fc6b3 100644
> --- a/cgit.mk
> +++ b/cgit.mk
> @@ -21,6 +21,11 @@ CGIT_CFLAGS += -DCGIT_CONFIG='"$(CGIT_CONFIG)"'
>  CGIT_CFLAGS += -DCGIT_SCRIPT_NAME='"$(CGIT_SCRIPT_NAME)"'
>  CGIT_CFLAGS += -DCGIT_CACHE_ROOT='"$(CACHE_ROOT)"'
>  
> +# Warn if function is declared or defined without argument types
> +CGIT_CFLAGS += -Wstrict-prototypes
> +# Warn if function is defined without previous declaration in header
> +CGIT_CFLAGS += -Wmissing-prototypes
> +
>  ifdef NO_C99_FORMAT
>  	CFLAGS += -DNO_C99_FORMAT
>  endif

[PATCH 1/2] Compile with -Wstrict-prototypes -Wmissing-prototypes

[Jason]

John - what compilers do you have in mind that don't accept this flag?
I rather like it.

[PATCH 2/2] Fix missing prototype declarations

[Jason]

Merged! Thanks.

[PATCH 1/2] Compile with -Wstrict-prototypes -Wmissing-prototypes

[mailings]

I work on olsrd and that makefile has compiler flag probing.
We could possibly 'borrow' that.

(the makefile is 'history' so don't puke)

On 14/01/16 14:01, Jason A. Donenfeld wrote:
> John - what compilers do you have in mind that don't accept this flag?
> I rather like it.
>

-- 
Ferry Huberts

[PATCH 1/2] Compile with -Wstrict-prototypes -Wmissing-prototypes

[john]

On Thu, Jan 14, 2016 at 02:01:32PM +0100, Jason A. Donenfeld wrote:
> John - what compilers do you have in mind that don't accept this flag?
> I rather like it.

SunStudio certainly doesn't support these diagnostics.

I'm following what git.git does in this regard.  If you check
git/Makefile you'll see that it sets -Wall but nothing else.  Since our
makefile is derived from that I'd prefer to follow the same principle.

The important thing is that we maintain the CFLAGS/CGIT_CFLAGS split for
compiler tuning vs. "functional" flags; warnings are in the former
category and should be in CFLAGS but we need to think carefully about
where to add those and allow the user to override them.  I think they
would have to go in cgit.mk between "include Makefile" and
"-include $(CGIT_PREFIX)cgit.conf" which allows users to override them
in cgit.conf but not in git/config.mak.

I would much rather that we add a wrapper akin to Junio's maintainer
"Make" for git.git [1] that compiles with these stricter flags rather
than modifying the default makefile and causing pain for those on less
common systems.  It only takes a couple of people to have this turned on
for us to catch these issues reasonably quickly.

[1] https://github.com/git/git/blob/todo/Make

[PATCH 1/2] Compile with -Wstrict-prototypes -Wmissing-prototypes

[Jason]

Okay, that's compelling enough.