[PATCH 00/17] HTTP response code improvements

[PATCH 00/17] HTTP response code improvements

[john]

Following Nicolas' report[0] about CGit caching error pages I've been
looking into ways of (optionally) avoiding caching error responses.

This series doesn't achieve that but it does give us two features that
will help to do so (neither of which are the case without this series of
patches):

1) Always return an HTTP error code if something goes wrong
2) Always set "ctx.page.status" when returning an error page

Without these changes CGit generally returns a "200 OK" response if
anything goes wrong after we have parsed the URL and found the
repository.  For example: unrecognised refs, unknown paths in trees,
corrupt objects in the repository.  This change requires moving the
layout start and end code into the individual page printing functions so
that we can delay printing the HTTP headers until we have resolved all
of the request parameters.

A more controversial change in this series is that we now always
generate an HTML response when an error occurs.  There are several cases
where this means that a response body will now be returned where there
was none before.  I think this is generally better, but I could be
convinced that there are some cases where we should not do this.

[0] http://lists.zx2c4.com/pipermail/cgit/2015-March/002464.html

John Keeping (17):
  ui-shared: add cgit_print_error_page() function
  cgit: use cgit_print_error_page() where appropriate
  clone: use cgit_print_error_page() instead of html_status()
  plain: use cgit_print_error_page() instead of html_status()
  snapshot: use cgit_print_error_page() instead of html_status()
  html: remove html_status()
  ui-shared: add cgit_print_layout_{start,end}()
  about: move layout into page functions
  commit: move layout into page function
  diff: move layout to page function
  log: move layout into page function
  refs: move layout to page function
  stats: move layout into page function
  summary: move layout into page function
  tag: move layout into page function
  tree: move layout into page function
  cmd: remove "want_layout" field

 cgit.c        | 36 +++++++-----------------------------
 cmd.c         | 44 ++++++++++++++++++++++----------------------
 cmd.h         |  1 -
 html.c        |  7 -------
 html.h        |  1 -
 ui-clone.c    | 10 +++++-----
 ui-commit.c   |  8 ++++++--
 ui-diff.c     | 19 ++++++++++++++-----
 ui-log.c      |  5 ++++-
 ui-plain.c    | 10 +++++-----
 ui-refs.c     |  3 ++-
 ui-repolist.c |  5 ++++-
 ui-shared.c   | 26 ++++++++++++++++++++++++++
 ui-shared.h   |  5 +++++
 ui-snapshot.c |  2 +-
 ui-stats.c    |  8 ++++++--
 ui-summary.c  | 10 ++++++++--
 ui-tag.c      | 13 ++++++++++---
 ui-tree.c     | 13 ++++++++++---
 19 files changed, 135 insertions(+), 91 deletions(-)

-- 
2.4.0.rc0.173.gb1cefcc

[PATCH 01/17] ui-shared: add cgit_print_error_page() function

[john]

This will allow us to generate error responses with the correct HTTP
response code without needing all of the layout boilerplate.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 ui-shared.c | 14 ++++++++++++++
 ui-shared.h |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/ui-shared.c b/ui-shared.c
index ac5a287..bcde2bb 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -767,6 +767,20 @@ void cgit_print_docend(void)
 	html("</body>\n</html>\n");
 }
 
+void cgit_print_error_page(int code, const char *msg, const char *fmt, ...)
+{
+	va_list ap;
+	ctx.page.status = 404;
+	ctx.page.statusmsg = "Not found";
+	cgit_print_http_headers();
+	cgit_print_docstart();
+	cgit_print_pageheader();
+	va_start(ap, fmt);
+	cgit_vprint_error(fmt, ap);
+	va_end(ap);
+	cgit_print_docend();
+}
+
 static void add_clone_urls(void (*fn)(const char *), char *txt, char *suffix)
 {
 	struct strbuf **url_list = strbuf_split_str(txt, ' ', 0);
diff --git a/ui-shared.h b/ui-shared.h
index 1b8ecb5..6a14858 100644
--- a/ui-shared.h
+++ b/ui-shared.h
@@ -63,6 +63,8 @@ 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_print_docstart(void);
 extern void cgit_print_docend();
+__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);
 extern void cgit_print_filemode(unsigned short mode);
 extern void cgit_print_snapshot_links(const char *repo, const char *head,
-- 
2.4.0.rc0.173.gb1cefcc

[PATCH 02/17] cgit: use cgit_print_error_page() where appropriate

[john]

These are more-or-less one-to-one translations but in the final hunk we
gain an HTTP error code where we used to send "200 OK", which is an
improvement.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 cgit.c | 27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)

diff --git a/cgit.c b/cgit.c
index ae413c6..227c336 100644
--- a/cgit.c
+++ b/cgit.c
@@ -610,13 +610,8 @@ static int prepare_repo_cmd(void)
 	if (get_sha1(ctx.qry.head, sha1)) {
 		char *tmp = xstrdup(ctx.qry.head);
 		ctx.qry.head = ctx.repo->defbranch;
-		ctx.page.status = 404;
-		ctx.page.statusmsg = "Not found";
-		cgit_print_http_headers();
-		cgit_print_docstart();
-		cgit_print_pageheader();
-		cgit_print_error("Invalid branch: %s", tmp);
-		cgit_print_docend();
+		cgit_print_error_page(404, "Not found",
+				"Invalid branch: %s", tmp);
 		free(tmp);
 		return 1;
 	}
@@ -709,18 +704,13 @@ static void process_request(void)
 	cmd = cgit_get_cmd();
 	if (!cmd) {
 		ctx.page.title = "cgit error";
-		ctx.page.status = 404;
-		ctx.page.statusmsg = "Not found";
-		cgit_print_http_headers();
-		cgit_print_docstart();
-		cgit_print_pageheader();
-		cgit_print_error("Invalid request");
-		cgit_print_docend();
+		cgit_print_error_page(404, "Not found", "Invalid request");
 		return;
 	}
 
 	if (!ctx.cfg.enable_http_clone && cmd->is_clone) {
-		html_status(404, "Not found", 0);
+		ctx.page.title = "cgit error";
+		cgit_print_error_page(404, "Not found", "Invalid request");
 		return;
 	}
 
@@ -731,11 +721,8 @@ static void process_request(void)
 	ctx.qry.vpath = cmd->want_vpath ? ctx.qry.path : NULL;
 
 	if (cmd->want_repo && !ctx.repo) {
-		cgit_print_http_headers();
-		cgit_print_docstart();
-		cgit_print_pageheader();
-		cgit_print_error("No repository selected");
-		cgit_print_docend();
+		cgit_print_error_page(400, "Bad request",
+				"No repository selected");
 		return;
 	}
 
-- 
2.4.0.rc0.173.gb1cefcc

[PATCH 03/17] clone: use cgit_print_error_page() instead of html_status()

[john]

This provides a formatted error response rather than a simple HTTP
error.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 ui-clone.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/ui-clone.c b/ui-clone.c
index e35d3d3..e7a6b76 100644
--- a/ui-clone.c
+++ b/ui-clone.c
@@ -57,13 +57,13 @@ static void send_file(char *path)
 	if (stat(path, &st)) {
 		switch (errno) {
 		case ENOENT:
-			html_status(404, "Not found", 0);
+			cgit_print_error_page(404, "Not found", "Not found");
 			break;
 		case EACCES:
-			html_status(403, "Forbidden", 0);
+			cgit_print_error_page(403, "Forbidden", "Forbidden");
 			break;
 		default:
-			html_status(400, "Bad request", 0);
+			cgit_print_error_page(400, "Bad request", "Bad request");
 		}
 		return;
 	}
@@ -78,7 +78,7 @@ static void send_file(char *path)
 void cgit_clone_info(void)
 {
 	if (!ctx.qry.path || strcmp(ctx.qry.path, "refs")) {
-		html_status(400, "Bad request", 0);
+		cgit_print_error_page(400, "Bad request", "Bad request");
 		return;
 	}
 
@@ -91,7 +91,7 @@ void cgit_clone_info(void)
 void cgit_clone_objects(void)
 {
 	if (!ctx.qry.path) {
-		html_status(400, "Bad request", 0);
+		cgit_print_error_page(400, "Bad request", "Bad request");
 		return;
 	}
 
-- 
2.4.0.rc0.173.gb1cefcc

[PATCH 04/17] plain: use cgit_print_error_page() instead of html_status()

[john]

This provides a formatted error response rather than a simple HTTP
error.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 ui-plain.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/ui-plain.c b/ui-plain.c
index b787bc3..e08dad9 100644
--- a/ui-plain.c
+++ b/ui-plain.c
@@ -67,13 +67,13 @@ static int print_object(const unsigned char *sha1, const char *path)
 
 	type = sha1_object_info(sha1, &size);
 	if (type == OBJ_BAD) {
-		html_status(404, "Not found", 0);
+		cgit_print_error_page(404, "Not found", "Not found");
 		return 0;
 	}
 
 	buf = read_sha1_file(sha1, &type, &size);
 	if (!buf) {
-		html_status(404, "Not found", 0);
+		cgit_print_error_page(404, "Not found", "Not found");
 		return 0;
 	}
 	ctx.page.mimetype = NULL;
@@ -226,12 +226,12 @@ void cgit_print_plain(void)
 		rev = ctx.qry.head;
 
 	if (get_sha1(rev, sha1)) {
-		html_status(404, "Not found", 0);
+		cgit_print_error_page(404, "Not found", "Not found");
 		return;
 	}
 	commit = lookup_commit_reference(sha1);
 	if (!commit || parse_commit(commit)) {
-		html_status(404, "Not found", 0);
+		cgit_print_error_page(404, "Not found", "Not found");
 		return;
 	}
 	if (!path_items.match) {
@@ -244,7 +244,7 @@ void cgit_print_plain(void)
 		walk_tree_ctx.match_baselen = basedir_len(path_items.match);
 	read_tree_recursive(commit->tree, "", 0, 0, &paths, walk_tree, &walk_tree_ctx);
 	if (!walk_tree_ctx.match)
-		html_status(404, "Not found", 0);
+		cgit_print_error_page(404, "Not found", "Not found");
 	else if (walk_tree_ctx.match == 2)
 		print_dir_tail();
 }
-- 
2.4.0.rc0.173.gb1cefcc

[PATCH 05/17] snapshot: use cgit_print_error_page() instead of html_status()

[john]

This provides a formatted error response rather than a simple HTTP
error.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 ui-snapshot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ui-snapshot.c b/ui-snapshot.c
index cb34f4b..bf4bcd7 100644
--- a/ui-snapshot.c
+++ b/ui-snapshot.c
@@ -213,7 +213,7 @@ void cgit_print_snapshot(const char *head, const char *hex,
 	if (!hex && dwim) {
 		hex = get_ref_from_filename(ctx.repo->url, filename, f);
 		if (hex == NULL) {
-			html_status(404, "Not found", 0);
+			cgit_print_error_page(404, "Not found", "Not found");
 			return;
 		}
 		prefix = xstrdup(filename);
-- 
2.4.0.rc0.173.gb1cefcc

[PATCH 06/17] html: remove html_status()

[john]

This is now unused.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 html.c | 7 -------
 html.h | 1 -
 2 files changed, 8 deletions(-)

diff --git a/html.c b/html.c
index 155cde5..9e0d6ba 100644
--- a/html.c
+++ b/html.c
@@ -127,13 +127,6 @@ void html_vtxtf(const char *format, va_list ap)
 	strbuf_release(&buf);
 }
 
-void html_status(int code, const char *msg, int more_headers)
-{
-	htmlf("Status: %d %s\n", code, msg);
-	if (!more_headers)
-		html("\n");
-}
-
 void html_txt(const char *txt)
 {
 	const char *t = txt;
diff --git a/html.h b/html.h
index be3b311..c554763 100644
--- a/html.h
+++ b/html.h
@@ -18,7 +18,6 @@ extern void html_vtxtf(const char *format, va_list ap);
 __attribute__((format (printf,1,2)))
 extern void html_attrf(const char *format,...);
 
-extern void html_status(int code, const char *msg, int more_headers);
 extern void html_txt(const char *txt);
 extern void html_ntxt(int len, const char *txt);
 extern void html_attr(const char *txt);
-- 
2.4.0.rc0.173.gb1cefcc

[PATCH 07/17] ui-shared: add cgit_print_layout_{start,end}()

[john]

These will avoid needing to call three functions to start page layout in
subsequent patches when we move the layout setup into each individual
page.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 ui-shared.c | 12 ++++++++++++
 ui-shared.h |  3 +++
 2 files changed, 15 insertions(+)

diff --git a/ui-shared.c b/ui-shared.c
index bcde2bb..e0b3121 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -781,6 +781,18 @@ void cgit_print_error_page(int code, const char *msg, const char *fmt, ...)
 	cgit_print_docend();
 }
 
+void cgit_print_layout_start(void)
+{
+	cgit_print_http_headers();
+	cgit_print_docstart();
+	cgit_print_pageheader();
+}
+
+void cgit_print_layout_end(void)
+{
+	cgit_print_docend();
+}
+
 static void add_clone_urls(void (*fn)(const char *), char *txt, char *suffix)
 {
 	struct strbuf **url_list = strbuf_split_str(txt, ' ', 0);
diff --git a/ui-shared.h b/ui-shared.h
index 6a14858..bc77535 100644
--- a/ui-shared.h
+++ b/ui-shared.h
@@ -54,6 +54,9 @@ extern void cgit_object_link(struct object *obj);
 extern void cgit_submodule_link(const char *class, char *path,
 				const char *rev);
 
+extern void cgit_print_layout_start(void);
+extern void cgit_print_layout_end(void);
+
 __attribute__((format (printf,1,2)))
 extern void cgit_print_error(const char *fmt, ...);
 __attribute__((format (printf,1,0)))
-- 
2.4.0.rc0.173.gb1cefcc

[PATCH 08/17] about: move layout into page functions

[john]

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 cmd.c         | 2 +-
 ui-repolist.c | 5 ++++-
 ui-summary.c  | 8 ++++++--
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/cmd.c b/cmd.c
index 188cd56..0f67c74 100644
--- a/cmd.c
+++ b/cmd.c
@@ -144,7 +144,7 @@ struct cgit_cmd *cgit_get_cmd(void)
 	static struct cgit_cmd cmds[] = {
 		def_cmd(HEAD, 1, 0, 0, 1),
 		def_cmd(atom, 1, 0, 0, 0),
-		def_cmd(about, 0, 1, 0, 0),
+		def_cmd(about, 0, 0, 0, 0),
 		def_cmd(blob, 1, 0, 0, 0),
 		def_cmd(commit, 1, 1, 1, 0),
 		def_cmd(diff, 1, 1, 1, 0),
diff --git a/ui-repolist.c b/ui-repolist.c
index 2453a7f..ea38384 100644
--- a/ui-repolist.c
+++ b/ui-repolist.c
@@ -346,9 +346,12 @@ void cgit_print_repolist(void)
 
 void cgit_print_site_readme(void)
 {
+	cgit_print_layout_start();
 	if (!ctx.cfg.root_readme)
-		return;
+		goto done;
 	cgit_open_filter(ctx.cfg.about_filter, ctx.cfg.root_readme);
 	html_include(ctx.cfg.root_readme);
 	cgit_close_filter(ctx.cfg.about_filter);
+done:
+	cgit_print_layout_end();
 }
diff --git a/ui-summary.c b/ui-summary.c
index b0af073..52ed2eb 100644
--- a/ui-summary.c
+++ b/ui-summary.c
@@ -102,8 +102,9 @@ void cgit_print_repo_readme(char *path)
 	char *filename, *ref;
 	int free_filename = 0;
 
+	cgit_print_layout_start();
 	if (ctx.repo->readme.nr == 0)
-		return;
+		goto done;
 
 	filename = ctx.repo->readme.items[0].string;
 	ref = ctx.repo->readme.items[0].util;
@@ -112,7 +113,7 @@ void cgit_print_repo_readme(char *path)
 		free_filename = 1;
 		filename = append_readme_path(filename, ref, path);
 		if (!filename)
-			return;
+			goto done;
 	}
 
 	/* Print the calculated readme, either from the git repo or from the
@@ -129,4 +130,7 @@ void cgit_print_repo_readme(char *path)
 	html("</div>");
 	if (free_filename)
 		free(filename);
+
+done:
+	cgit_print_layout_end();
 }
-- 
2.4.0.rc0.173.gb1cefcc

[PATCH 09/17] commit: move layout into page function

[john]

This allows us to return a proper HTTP status code when an object is not
found by switching from cgit_print_error() to cgit_print_error_page().

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 cmd.c       | 2 +-
 ui-commit.c | 8 ++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/cmd.c b/cmd.c
index 0f67c74..73c5e40 100644
--- a/cmd.c
+++ b/cmd.c
@@ -146,7 +146,7 @@ struct cgit_cmd *cgit_get_cmd(void)
 		def_cmd(atom, 1, 0, 0, 0),
 		def_cmd(about, 0, 0, 0, 0),
 		def_cmd(blob, 1, 0, 0, 0),
-		def_cmd(commit, 1, 1, 1, 0),
+		def_cmd(commit, 1, 0, 1, 0),
 		def_cmd(diff, 1, 1, 1, 0),
 		def_cmd(info, 1, 0, 0, 1),
 		def_cmd(log, 1, 1, 1, 0),
diff --git a/ui-commit.c b/ui-commit.c
index d5a888d..2bca7a0 100644
--- a/ui-commit.c
+++ b/ui-commit.c
@@ -27,12 +27,14 @@ void cgit_print_commit(char *hex, const char *prefix)
 		hex = ctx.qry.head;
 
 	if (get_sha1(hex, sha1)) {
-		cgit_print_error("Bad object id: %s", hex);
+		cgit_print_error_page(400, "Bad request",
+				"Bad object id: %s", hex);
 		return;
 	}
 	commit = lookup_commit_reference(sha1);
 	if (!commit) {
-		cgit_print_error("Bad commit reference: %s", hex);
+		cgit_print_error_page(404, "Not found",
+				"Bad commit reference: %s", hex);
 		return;
 	}
 	info = cgit_parse_commit(commit);
@@ -41,6 +43,7 @@ void cgit_print_commit(char *hex, const char *prefix)
 
 	load_ref_decorations(DECORATE_FULL_REFS);
 
+	cgit_print_layout_start();
 	cgit_print_diff_ctrls();
 	html("<table summary='commit info' class='commit-info'>\n");
 	html("<tr><th>author</th><td>");
@@ -139,4 +142,5 @@ void cgit_print_commit(char *hex, const char *prefix)
 	}
 	strbuf_release(&notes);
 	cgit_free_commitinfo(info);
+	cgit_print_layout_end();
 }
-- 
2.4.0.rc0.173.gb1cefcc

[PATCH 10/17] diff: move layout to page function

[john]

The existing "show_ctrls" flag is used to control whether we are running
in an existing page or control the page ourselves.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 cmd.c     |  2 +-
 ui-diff.c | 19 ++++++++++++++-----
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/cmd.c b/cmd.c
index 73c5e40..f26ae0b 100644
--- a/cmd.c
+++ b/cmd.c
@@ -147,7 +147,7 @@ struct cgit_cmd *cgit_get_cmd(void)
 		def_cmd(about, 0, 0, 0, 0),
 		def_cmd(blob, 1, 0, 0, 0),
 		def_cmd(commit, 1, 0, 1, 0),
-		def_cmd(diff, 1, 1, 1, 0),
+		def_cmd(diff, 1, 0, 1, 0),
 		def_cmd(info, 1, 0, 0, 1),
 		def_cmd(log, 1, 1, 1, 0),
 		def_cmd(ls_cache, 0, 0, 0, 0),
diff --git a/ui-diff.c b/ui-diff.c
index 1cf2ce0..5066429 100644
--- a/ui-diff.c
+++ b/ui-diff.c
@@ -368,19 +368,22 @@ void cgit_print_diff(const char *new_rev, const char *old_rev,
 	if (!new_rev)
 		new_rev = ctx.qry.head;
 	if (get_sha1(new_rev, new_rev_sha1)) {
-		cgit_print_error("Bad object name: %s", new_rev);
+		cgit_print_error_page(404, "Not found",
+			"Bad object name: %s", new_rev);
 		return;
 	}
 	commit = lookup_commit_reference(new_rev_sha1);
 	if (!commit || parse_commit(commit)) {
-		cgit_print_error("Bad commit: %s", sha1_to_hex(new_rev_sha1));
+		cgit_print_error_page(404, "Not found",
+			"Bad commit: %s", sha1_to_hex(new_rev_sha1));
 		return;
 	}
 	new_tree_sha1 = commit->tree->object.sha1;
 
 	if (old_rev) {
 		if (get_sha1(old_rev, old_rev_sha1)) {
-			cgit_print_error("Bad object name: %s", old_rev);
+			cgit_print_error_page(404, "Not found",
+				"Bad object name: %s", old_rev);
 			return;
 		}
 	} else if (commit->parents && commit->parents->item) {
@@ -392,7 +395,8 @@ void cgit_print_diff(const char *new_rev, const char *old_rev,
 	if (!is_null_sha1(old_rev_sha1)) {
 		commit2 = lookup_commit_reference(old_rev_sha1);
 		if (!commit2 || parse_commit(commit2)) {
-			cgit_print_error("Bad commit: %s", sha1_to_hex(old_rev_sha1));
+			cgit_print_error_page(404, "Not found",
+				"Bad commit: %s", sha1_to_hex(old_rev_sha1));
 			return;
 		}
 		old_tree_sha1 = commit2->tree->object.sha1;
@@ -425,8 +429,10 @@ void cgit_print_diff(const char *new_rev, const char *old_rev,
 	difftype = ctx.qry.has_difftype ? ctx.qry.difftype : ctx.cfg.difftype;
 	use_ssdiff = difftype == DIFF_SSDIFF;
 
-	if (show_ctrls)
+	if (show_ctrls) {
+		cgit_print_layout_start();
 		cgit_print_diff_ctrls();
+	}
 
 	/*
 	 * Clicking on a link to a file in the diff stat should show a diff
@@ -454,4 +460,7 @@ void cgit_print_diff(const char *new_rev, const char *old_rev,
 	if (!use_ssdiff)
 		html("</td></tr>");
 	html("</table>");
+
+	if (show_ctrls)
+		cgit_print_layout_end();
 }
-- 
2.4.0.rc0.173.gb1cefcc

[PATCH 11/17] log: move layout into page function

[john]

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 cmd.c    | 2 +-
 ui-log.c | 5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/cmd.c b/cmd.c
index f26ae0b..b61c736 100644
--- a/cmd.c
+++ b/cmd.c
@@ -149,7 +149,7 @@ struct cgit_cmd *cgit_get_cmd(void)
 		def_cmd(commit, 1, 0, 1, 0),
 		def_cmd(diff, 1, 0, 1, 0),
 		def_cmd(info, 1, 0, 0, 1),
-		def_cmd(log, 1, 1, 1, 0),
+		def_cmd(log, 1, 0, 1, 0),
 		def_cmd(ls_cache, 0, 0, 0, 0),
 		def_cmd(objects, 1, 0, 0, 1),
 		def_cmd(patch, 1, 0, 1, 0),
diff --git a/ui-log.c b/ui-log.c
index 32b4c47..b2a3948 100644
--- a/ui-log.c
+++ b/ui-log.c
@@ -354,8 +354,10 @@ void cgit_print_log(const char *tip, int ofs, int cnt, char *grep, char *pattern
 	compile_grep_patterns(&rev.grep_filter);
 	prepare_revision_walk(&rev);
 
-	if (pager)
+	if (pager) {
+		cgit_print_layout_start();
 		html("<table class='list nowrap'>");
+	}
 
 	html("<tr class='nohover'>");
 	if (commit_graph)
@@ -418,6 +420,7 @@ void cgit_print_log(const char *tip, int ofs, int cnt, char *grep, char *pattern
 			html("</li>");
 		}
 		html("</ul>");
+		cgit_print_layout_end();
 	} else if ((commit = get_revision(&rev)) != NULL) {
 		htmlf("<tr class='nohover'><td colspan='%d'>", columns);
 		cgit_log_link("[...]", NULL, NULL, ctx.qry.head, NULL,
-- 
2.4.0.rc0.173.gb1cefcc

[PATCH 12/17] refs: move layout to page function

[john]

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 cmd.c     | 2 +-
 ui-refs.c | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/cmd.c b/cmd.c
index b61c736..57a8fe7 100644
--- a/cmd.c
+++ b/cmd.c
@@ -155,7 +155,7 @@ struct cgit_cmd *cgit_get_cmd(void)
 		def_cmd(patch, 1, 0, 1, 0),
 		def_cmd(plain, 1, 0, 0, 0),
 		def_cmd(rawdiff, 1, 0, 1, 0),
-		def_cmd(refs, 1, 1, 0, 0),
+		def_cmd(refs, 1, 0, 0, 0),
 		def_cmd(repolist, 0, 0, 0, 0),
 		def_cmd(snapshot, 1, 0, 0, 0),
 		def_cmd(stats, 1, 1, 1, 0),
diff --git a/ui-refs.c b/ui-refs.c
index d3d71dd..29dc860 100644
--- a/ui-refs.c
+++ b/ui-refs.c
@@ -236,7 +236,7 @@ void cgit_print_tags(int maxcount)
 
 void cgit_print_refs(void)
 {
-
+	cgit_print_layout_start();
 	html("<table class='list nowrap'>");
 
 	if (ctx.qry.path && starts_with(ctx.qry.path, "heads"))
@@ -249,4 +249,5 @@ void cgit_print_refs(void)
 		cgit_print_tags(0);
 	}
 	html("</table>");
+	cgit_print_layout_end();
 }
-- 
2.4.0.rc0.173.gb1cefcc

[PATCH 13/17] stats: move layout into page function

[john]

This also allows us to return proper HTTP error codes for invalid
requests.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 cmd.c      | 2 +-
 ui-stats.c | 8 ++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/cmd.c b/cmd.c
index 57a8fe7..5340405 100644
--- a/cmd.c
+++ b/cmd.c
@@ -158,7 +158,7 @@ struct cgit_cmd *cgit_get_cmd(void)
 		def_cmd(refs, 1, 0, 0, 0),
 		def_cmd(repolist, 0, 0, 0, 0),
 		def_cmd(snapshot, 1, 0, 0, 0),
-		def_cmd(stats, 1, 1, 1, 0),
+		def_cmd(stats, 1, 0, 1, 0),
 		def_cmd(summary, 1, 1, 0, 0),
 		def_cmd(tag, 1, 1, 0, 0),
 		def_cmd(tree, 1, 1, 1, 0),
diff --git a/ui-stats.c b/ui-stats.c
index 9cd8247..74ce0f7 100644
--- a/ui-stats.c
+++ b/ui-stats.c
@@ -372,11 +372,13 @@ void cgit_show_stats(void)
 
 	i = cgit_find_stats_period(code, &period);
 	if (!i) {
-		cgit_print_error("Unknown statistics type: %c", code[0]);
+		cgit_print_error_page(404, "Not found",
+			"Unknown statistics type: %c", code[0]);
 		return;
 	}
 	if (i > ctx.repo->max_stats) {
-		cgit_print_error("Statistics type disabled: %s", period->name);
+		cgit_print_error_page(400, "Bad request",
+			"Statistics type disabled: %s", period->name);
 		return;
 	}
 	authors = collect_stats(period);
@@ -387,6 +389,7 @@ void cgit_show_stats(void)
 	if (!top)
 		top = 10;
 
+	cgit_print_layout_start();
 	html("<div class='cgit-panel'>");
 	html("<b>stat options</b>");
 	html("<form method='get' action=''>");
@@ -421,5 +424,6 @@ void cgit_show_stats(void)
 	}
 	html("</h2>");
 	print_authors(&authors, top, period);
+	cgit_print_layout_end();
 }
 
-- 
2.4.0.rc0.173.gb1cefcc

[PATCH 14/17] summary: move layout into page function

[john]

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 cmd.c        | 2 +-
 ui-summary.c | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/cmd.c b/cmd.c
index 5340405..ef41ccf 100644
--- a/cmd.c
+++ b/cmd.c
@@ -159,7 +159,7 @@ struct cgit_cmd *cgit_get_cmd(void)
 		def_cmd(repolist, 0, 0, 0, 0),
 		def_cmd(snapshot, 1, 0, 0, 0),
 		def_cmd(stats, 1, 0, 1, 0),
-		def_cmd(summary, 1, 1, 0, 0),
+		def_cmd(summary, 1, 0, 0, 0),
 		def_cmd(tag, 1, 1, 0, 0),
 		def_cmd(tree, 1, 1, 1, 0),
 	};
diff --git a/ui-summary.c b/ui-summary.c
index 52ed2eb..bf250b2 100644
--- a/ui-summary.c
+++ b/ui-summary.c
@@ -49,6 +49,7 @@ void cgit_print_summary(void)
 	if (ctx.repo->enable_log_linecount)
 		columns++;
 
+	cgit_print_layout_start();
 	html("<table summary='repository info' class='list nowrap'>");
 	cgit_print_branches(ctx.cfg.summary_branches);
 	htmlf("<tr class='nohover'><td colspan='%d'>&nbsp;</td></tr>", columns);
@@ -61,6 +62,7 @@ void cgit_print_summary(void)
 	urls = 0;
 	cgit_add_clone_urls(print_url);
 	html("</table>");
+	cgit_print_layout_end();
 }
 
 /* The caller must free the return value. */
-- 
2.4.0.rc0.173.gb1cefcc

[PATCH 15/17] tag: move layout into page function

[john]

This also allows us to return proper HTTP error codes when something
goes wrong.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 cmd.c    |  2 +-
 ui-tag.c | 13 ++++++++++---
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/cmd.c b/cmd.c
index ef41ccf..27408f2 100644
--- a/cmd.c
+++ b/cmd.c
@@ -160,7 +160,7 @@ struct cgit_cmd *cgit_get_cmd(void)
 		def_cmd(snapshot, 1, 0, 0, 0),
 		def_cmd(stats, 1, 0, 1, 0),
 		def_cmd(summary, 1, 0, 0, 0),
-		def_cmd(tag, 1, 1, 0, 0),
+		def_cmd(tag, 1, 0, 0, 0),
 		def_cmd(tree, 1, 1, 1, 0),
 	};
 	int i;
diff --git a/ui-tag.c b/ui-tag.c
index c1d1738..0afc663 100644
--- a/ui-tag.c
+++ b/ui-tag.c
@@ -52,20 +52,24 @@ void cgit_print_tag(char *revname)
 
 	strbuf_addf(&fullref, "refs/tags/%s", revname);
 	if (get_sha1(fullref.buf, sha1)) {
-		cgit_print_error("Bad tag reference: %s", revname);
+		cgit_print_error_page(404, "Not found",
+			"Bad tag reference: %s", revname);
 		goto cleanup;
 	}
 	obj = parse_object(sha1);
 	if (!obj) {
-		cgit_print_error("Bad object id: %s", sha1_to_hex(sha1));
+		cgit_print_error_page(500, "Internal server error",
+			"Bad object id: %s", sha1_to_hex(sha1));
 		goto cleanup;
 	}
 	if (obj->type == OBJ_TAG) {
 		tag = lookup_tag(sha1);
 		if (!tag || parse_tag(tag) || !(info = cgit_parse_tag(tag))) {
-			cgit_print_error("Bad tag object: %s", revname);
+			cgit_print_error_page(500, "Internal server error",
+				"Bad tag object: %s", revname);
 			goto cleanup;
 		}
+		cgit_print_layout_start();
 		html("<table class='commit-info'>\n");
 		htmlf("<tr><td>tag name</td><td>");
 		html_txt(revname);
@@ -93,7 +97,9 @@ void cgit_print_tag(char *revname)
 			print_download_links(revname);
 		html("</table>\n");
 		print_tag_content(info->msg);
+		cgit_print_layout_end();
 	} else {
+		cgit_print_layout_start();
 		html("<table class='commit-info'>\n");
 		htmlf("<tr><td>tag name</td><td>");
 		html_txt(revname);
@@ -104,6 +110,7 @@ void cgit_print_tag(char *revname)
 		if (ctx.repo->snapshots)
 			print_download_links(revname);
 		html("</table>\n");
+		cgit_print_layout_end();
 	}
 
 cleanup:
-- 
2.4.0.rc0.173.gb1cefcc

[PATCH 16/17] tree: move layout into page function

[john]

This also allows us to return proper HTTP error codes when the requested
tree is not found and display an error message in one case (invalid path
inside valid commit) where we previously just displayed an empty page.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 cmd.c     |  2 +-
 ui-tree.c | 13 ++++++++++---
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/cmd.c b/cmd.c
index 27408f2..6a964b1 100644
--- a/cmd.c
+++ b/cmd.c
@@ -161,7 +161,7 @@ struct cgit_cmd *cgit_get_cmd(void)
 		def_cmd(stats, 1, 0, 1, 0),
 		def_cmd(summary, 1, 0, 0, 0),
 		def_cmd(tag, 1, 0, 0, 0),
-		def_cmd(tree, 1, 1, 1, 0),
+		def_cmd(tree, 1, 0, 1, 0),
 	};
 	int i;
 
diff --git a/ui-tree.c b/ui-tree.c
index bbc468e..4c1f9cb 100644
--- a/ui-tree.c
+++ b/ui-tree.c
@@ -182,6 +182,7 @@ static int ls_item(const unsigned char *sha1, struct strbuf *base,
 
 static void ls_head(void)
 {
+	cgit_print_layout_start();
 	html("<table summary='tree listing' class='list'>\n");
 	html("<tr class='nohover'>");
 	html("<th class='left'>Mode</th>");
@@ -194,6 +195,7 @@ static void ls_head(void)
 static void ls_tail(void)
 {
 	html("</table>\n");
+	cgit_print_layout_end();
 }
 
 static void ls_tree(const unsigned char *sha1, char *path, struct walk_tree_context *walk_tree_ctx)
@@ -205,7 +207,8 @@ static void ls_tree(const unsigned char *sha1, char *path, struct walk_tree_cont
 
 	tree = parse_tree_indirect(sha1);
 	if (!tree) {
-		cgit_print_error("Not a tree object: %s", sha1_to_hex(sha1));
+		cgit_print_error_page(400, "Bad request",
+			"Not a tree object: %s", sha1_to_hex(sha1));
 		return;
 	}
 
@@ -266,12 +269,14 @@ void cgit_print_tree(const char *rev, char *path)
 		rev = ctx.qry.head;
 
 	if (get_sha1(rev, sha1)) {
-		cgit_print_error("Invalid revision name: %s", rev);
+		cgit_print_error_page(404, "Not found",
+			"Invalid revision name: %s", rev);
 		return;
 	}
 	commit = lookup_commit_reference(sha1);
 	if (!commit || parse_commit(commit)) {
-		cgit_print_error("Invalid commit reference: %s", rev);
+		cgit_print_error_page(404, "Not found",
+			"Invalid commit reference: %s", rev);
 		return;
 	}
 
@@ -285,6 +290,8 @@ void cgit_print_tree(const char *rev, char *path)
 	read_tree_recursive(commit->tree, "", 0, 0, &paths, walk_tree, &walk_tree_ctx);
 	if (walk_tree_ctx.state == 1)
 		ls_tail();
+	else
+		cgit_print_error_page(404, "Not found", "Path not found");
 
 cleanup:
 	free(walk_tree_ctx.curr_rev);
-- 
2.4.0.rc0.173.gb1cefcc

[PATCH 17/17] cmd: remove "want_layout" field

[john]

No commands use this any more.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 cgit.c |  9 ---------
 cmd.c  | 44 ++++++++++++++++++++++----------------------
 cmd.h  |  1 -
 3 files changed, 22 insertions(+), 32 deletions(-)

diff --git a/cgit.c b/cgit.c
index 227c336..465b4c3 100644
--- a/cgit.c
+++ b/cgit.c
@@ -729,16 +729,7 @@ static void process_request(void)
 	if (ctx.repo && prepare_repo_cmd())
 		return;
 
-	if (cmd->want_layout) {
-		cgit_print_http_headers();
-		cgit_print_docstart();
-		cgit_print_pageheader();
-	}
-
 	cmd->fn();
-
-	if (cmd->want_layout)
-		cgit_print_docend();
 }
 
 static int cmp_repos(const void *a, const void *b)
diff --git a/cmd.c b/cmd.c
index 6a964b1..d399f7d 100644
--- a/cmd.c
+++ b/cmd.c
@@ -136,32 +136,32 @@ static void tree_fn(void)
 	cgit_print_tree(ctx.qry.sha1, ctx.qry.path);
 }
 
-#define def_cmd(name, want_repo, want_layout, want_vpath, is_clone) \
-	{#name, name##_fn, want_repo, want_layout, want_vpath, is_clone}
+#define def_cmd(name, want_repo, want_vpath, is_clone) \
+	{#name, name##_fn, want_repo, want_vpath, is_clone}
 
 struct cgit_cmd *cgit_get_cmd(void)
 {
 	static struct cgit_cmd cmds[] = {
-		def_cmd(HEAD, 1, 0, 0, 1),
-		def_cmd(atom, 1, 0, 0, 0),
-		def_cmd(about, 0, 0, 0, 0),
-		def_cmd(blob, 1, 0, 0, 0),
-		def_cmd(commit, 1, 0, 1, 0),
-		def_cmd(diff, 1, 0, 1, 0),
-		def_cmd(info, 1, 0, 0, 1),
-		def_cmd(log, 1, 0, 1, 0),
-		def_cmd(ls_cache, 0, 0, 0, 0),
-		def_cmd(objects, 1, 0, 0, 1),
-		def_cmd(patch, 1, 0, 1, 0),
-		def_cmd(plain, 1, 0, 0, 0),
-		def_cmd(rawdiff, 1, 0, 1, 0),
-		def_cmd(refs, 1, 0, 0, 0),
-		def_cmd(repolist, 0, 0, 0, 0),
-		def_cmd(snapshot, 1, 0, 0, 0),
-		def_cmd(stats, 1, 0, 1, 0),
-		def_cmd(summary, 1, 0, 0, 0),
-		def_cmd(tag, 1, 0, 0, 0),
-		def_cmd(tree, 1, 0, 1, 0),
+		def_cmd(HEAD, 1, 0, 1),
+		def_cmd(atom, 1, 0, 0),
+		def_cmd(about, 0, 0, 0),
+		def_cmd(blob, 1, 0, 0),
+		def_cmd(commit, 1, 1, 0),
+		def_cmd(diff, 1, 1, 0),
+		def_cmd(info, 1, 0, 1),
+		def_cmd(log, 1, 1, 0),
+		def_cmd(ls_cache, 0, 0, 0),
+		def_cmd(objects, 1, 0, 1),
+		def_cmd(patch, 1, 1, 0),
+		def_cmd(plain, 1, 0, 0),
+		def_cmd(rawdiff, 1, 1, 0),
+		def_cmd(refs, 1, 0, 0),
+		def_cmd(repolist, 0, 0, 0),
+		def_cmd(snapshot, 1, 0, 0),
+		def_cmd(stats, 1, 1, 0),
+		def_cmd(summary, 1, 0, 0),
+		def_cmd(tag, 1, 0, 0),
+		def_cmd(tree, 1, 1, 0),
 	};
 	int i;
 
diff --git a/cmd.h b/cmd.h
index 752f078..6249b1d 100644
--- a/cmd.h
+++ b/cmd.h
@@ -7,7 +7,6 @@ struct cgit_cmd {
 	const char *name;
 	cgit_cmd_fn fn;
 	unsigned int want_repo:1,
-		want_layout:1,
 		want_vpath:1,
 		is_clone:1;
 };
-- 
2.4.0.rc0.173.gb1cefcc

[PATCH 00/17] HTTP response code improvements

[john]

On Sun, Apr 05, 2015 at 04:54:46PM +0100, John Keeping wrote:
> Following Nicolas' report[0] about CGit caching error pages I've been
> looking into ways of (optionally) avoiding caching error responses.
> 
> This series doesn't achieve that but it does give us two features that
> will help to do so (neither of which are the case without this series of
> patches):
> 
> 1) Always return an HTTP error code if something goes wrong
> 2) Always set "ctx.page.status" when returning an error page

The change to avoid caching errors is quite simple on top of this
series:

-- >8 --
diff --git a/cache.c b/cache.c
index cd99812..be41467 100644
--- a/cache.c
+++ b/cache.c
@@ -261,6 +261,7 @@ unsigned long hash_str(const char *str)
 static int process_slot(struct cache_slot *slot)
 {
 	int err;
+	int use_new_slot;
 
 	err = open_slot(slot);
 	if (!err && slot->match) {
@@ -333,7 +334,8 @@ static int process_slot(struct cache_slot *slot)
 	// Lets avoid such a race by just printing the content of
 	// the lock file.
 	slot->cache_fd = slot->lock_fd;
-	unlock_slot(slot, 1);
+	use_new_slot = !ctx.page.status || ctx.page.status == 200;
+	unlock_slot(slot, use_new_slot);
 	if ((err = print_slot(slot)) != 0) {
 		cache_log("[cgit] error printing cache %s: %s (%d)\n",
 			  slot->cache_name,
-- 8< --

But I think we probably want something a bit more configurable than that
since there are some errors that users might want to cache.

[PATCH 16/17] tree: move layout into page function

[john]

On Sun, Apr 05, 2015 at 04:55:02PM +0100, John Keeping wrote:
> This also allows us to return proper HTTP error codes when the requested
> tree is not found and display an error message in one case (invalid path
> inside valid commit) where we previously just displayed an empty page.
> 
> Signed-off-by: John Keeping <john at keeping.me.uk>
> ---

This breaks displaying file contents in tree view; it needs something
like this squashed into it, which has the advantage of making a couple
more errors return proper HTTP status codes but indicates how
complicated the layout setup in ui-tree.c becomes:

diff --git a/ui-tree.c b/ui-tree.c
index 4c1f9cb..70ea2ea 100644
--- a/ui-tree.c
+++ b/ui-tree.c
@@ -93,16 +93,19 @@ static void print_object(const unsigned char *sha1, char *path, const char *base
 
 	type = sha1_object_info(sha1, &size);
 	if (type == OBJ_BAD) {
-		cgit_print_error("Bad object name: %s", sha1_to_hex(sha1));
+		cgit_print_error_page(404, "Not found",
+			"Bad object name: %s", sha1_to_hex(sha1));
 		return;
 	}
 
 	buf = read_sha1_file(sha1, &type, &size);
 	if (!buf) {
-		cgit_print_error("Error reading object %s", sha1_to_hex(sha1));
+		cgit_print_error_page(500, "Internal server error",
+			"Error reading object %s", sha1_to_hex(sha1));
 		return;
 	}
 
+	cgit_print_layout_start();
 	htmlf("blob: %s (", sha1_to_hex(sha1));
 	cgit_plain_link("plain", NULL, NULL, ctx.qry.head,
 		        rev, path);
@@ -235,6 +238,7 @@ static int walk_tree(const unsigned char *sha1, struct strbuf *base,
 			ls_head();
 			return READ_TREE_RECURSIVE;
 		} else {
+			walk_tree_ctx->state = 2;
 			print_object(sha1, buffer, pathname, walk_tree_ctx->curr_rev);
 			return 0;
 		}
@@ -290,6 +294,8 @@ void cgit_print_tree(const char *rev, char *path)
 	read_tree_recursive(commit->tree, "", 0, 0, &paths, walk_tree, &walk_tree_ctx);
 	if (walk_tree_ctx.state == 1)
 		ls_tail();
+	else if (walk_tree_ctx.state == 2)
+		cgit_print_layout_end();
 	else
 		cgit_print_error_page(404, "Not found", "Path not found");
 
>  cmd.c     |  2 +-
>  ui-tree.c | 13 ++++++++++---
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/cmd.c b/cmd.c
> index 27408f2..6a964b1 100644
> --- a/cmd.c
> +++ b/cmd.c
> @@ -161,7 +161,7 @@ struct cgit_cmd *cgit_get_cmd(void)
>  		def_cmd(stats, 1, 0, 1, 0),
>  		def_cmd(summary, 1, 0, 0, 0),
>  		def_cmd(tag, 1, 0, 0, 0),
> -		def_cmd(tree, 1, 1, 1, 0),
> +		def_cmd(tree, 1, 0, 1, 0),
>  	};
>  	int i;
>  
> diff --git a/ui-tree.c b/ui-tree.c
> index bbc468e..4c1f9cb 100644
> --- a/ui-tree.c
> +++ b/ui-tree.c
> @@ -182,6 +182,7 @@ static int ls_item(const unsigned char *sha1, struct strbuf *base,
>  
>  static void ls_head(void)
>  {
> +	cgit_print_layout_start();
>  	html("<table summary='tree listing' class='list'>\n");
>  	html("<tr class='nohover'>");
>  	html("<th class='left'>Mode</th>");
> @@ -194,6 +195,7 @@ static void ls_head(void)
>  static void ls_tail(void)
>  {
>  	html("</table>\n");
> +	cgit_print_layout_end();
>  }
>  
>  static void ls_tree(const unsigned char *sha1, char *path, struct walk_tree_context *walk_tree_ctx)
> @@ -205,7 +207,8 @@ static void ls_tree(const unsigned char *sha1, char *path, struct walk_tree_cont
>  
>  	tree = parse_tree_indirect(sha1);
>  	if (!tree) {
> -		cgit_print_error("Not a tree object: %s", sha1_to_hex(sha1));
> +		cgit_print_error_page(400, "Bad request",
> +			"Not a tree object: %s", sha1_to_hex(sha1));
>  		return;
>  	}
>  
> @@ -266,12 +269,14 @@ void cgit_print_tree(const char *rev, char *path)
>  		rev = ctx.qry.head;
>  
>  	if (get_sha1(rev, sha1)) {
> -		cgit_print_error("Invalid revision name: %s", rev);
> +		cgit_print_error_page(404, "Not found",
> +			"Invalid revision name: %s", rev);
>  		return;
>  	}
>  	commit = lookup_commit_reference(sha1);
>  	if (!commit || parse_commit(commit)) {
> -		cgit_print_error("Invalid commit reference: %s", rev);
> +		cgit_print_error_page(404, "Not found",
> +			"Invalid commit reference: %s", rev);
>  		return;
>  	}
>  
> @@ -285,6 +290,8 @@ void cgit_print_tree(const char *rev, char *path)
>  	read_tree_recursive(commit->tree, "", 0, 0, &paths, walk_tree, &walk_tree_ctx);
>  	if (walk_tree_ctx.state == 1)
>  		ls_tail();
> +	else
> +		cgit_print_error_page(404, "Not found", "Path not found");
>  
>  cleanup:
>  	free(walk_tree_ctx.curr_rev);
> -- 
> 2.4.0.rc0.173.gb1cefcc
> 
> _______________________________________________
> CGit mailing list
> CGit at lists.zx2c4.com
> http://lists.zx2c4.com/mailman/listinfo/cgit

[PATCH 01/17] ui-shared: add cgit_print_error_page() function

[Jason]

On Sun, Apr 5, 2015 at 5:54 PM, John Keeping <john at keeping.me.uk> wrote:
>
> +void cgit_print_error_page(int code, const char *msg, const char *fmt,
> ...)
> +{
> +       va_list ap;
> +       ctx.page.status = 404;
> +       ctx.page.statusmsg = "Not found";
>

Shouldn't status = code here, not hardcoded 404?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20150407/e2ef04b0/attachment.html>

[PATCH 01/17] ui-shared: add cgit_print_error_page() function

[john]

On Tue, Apr 07, 2015 at 03:23:03PM +0200, Jason A. Donenfeld wrote:
> On Sun, Apr 5, 2015 at 5:54 PM, John Keeping wrote:
> >
> > +void cgit_print_error_page(int code, const char *msg, const char *fmt,
> > ...)
> > +{
> > +       va_list ap;
> > +       ctx.page.status = 404;
> > +       ctx.page.statusmsg = "Not found";
> >
> 
> Shouldn't status = code here, not hardcoded 404?

Yes, copy+paste error I think :-(

There also needs to be a change to the "expires" field here (otherwise
we send an error page with an expiry date 10 years in the future!), but
I didn't want to spam the list with messages by sending a re-roll before
any feedback.

[PATCH 01/17] ui-shared: add cgit_print_error_page() function

[Jason]

I've put them in this branch with a few fixes:
http://git.zx2c4.com/cgit/log/?h=jk/http-status-codes

Feel free to send diffs I should squash into any of these commits.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20150407/ca65a08a/attachment.html>

[PATCH 01/17] ui-shared: add cgit_print_error_page() function

[john]

On Tue, Apr 07, 2015 at 03:36:32PM +0200, Jason A. Donenfeld wrote:
> I've put them in this branch with a few fixes:
> http://git.zx2c4.com/cgit/log/?h=jk/http-status-codes
> 
> Feel free to send diffs I should squash into any of these commits.

Thanks.  The only other change I have from testing this yesterday is
this; I'm not sure if 5 minutes is a sensible value here but I'm not
sure it's worth exposing this as a config variable and any downstream
cache can be configured with specific timeouts for different response
codes if required:

-- >8 --
Subject: [PATCH] fixup! ui-shared: add cgit_print_error_page() function

---
 ui-shared.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ui-shared.c b/ui-shared.c
index c11d9b6..5126962 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -770,6 +770,7 @@ void cgit_print_docend(void)
 void cgit_print_error_page(int code, const char *msg, const char *fmt, ...)
 {
 	va_list ap;
+	ctx.page.expires = 5 * 60; /* 5 minutes */
 	ctx.page.status = code;
 	ctx.page.statusmsg = msg;
 	cgit_print_http_headers();
-- 8< --

[PATCH v2 00/22] HTTP status code improvements

[john]

Changes since v1 (starts at [0]):
- Rebased on the latest master
- Following Christian's patch [1], a new patch changes ui-patch.c to use
  cgit_print_error_page() and other new patches fix similar issues in
  ui-blob.c and ui-snapshot.c
- The last patch is new, and uses "cache-dynamic-ttl" for error pages;
  I'm not 100% sure about that, but it's better than the current
  behaviour if someone tries to access a page by SHA1 before the object
  has been pushed to the repository CGit is displaying

[0] http://article.gmane.org/gmane.comp.version-control.cgit/2451
[1] http://article.gmane.org/gmane.comp.version-control.cgit/2589

John Keeping (22):
  ui-shared: add cgit_print_error_page() function
  cgit: use cgit_print_error_page() where appropriate
  clone: use cgit_print_error_page() instead of html_status()
  plain: use cgit_print_error_page() instead of html_status()
  snapshot: use cgit_print_error_page() instead of html_status()
  blob: use cgit_print_error_page() to add HTTP headers
  patch: use cgit_print_error_page() for HTTP status codes
  snapshot: use cgit_print_error_page() for HTTP status codes
  snapshot: don't reimplement cgit_print_error_page()
  html: remove html_status()
  ui-shared: add cgit_print_layout_{start,end}()
  about: move layout into page functions
  commit: move layout into page function
  diff: move layout to page function
  log: move layout into page function
  refs: move layout to page function
  stats: move layout into page function
  summary: move layout into page function
  tag: move layout into page function
  tree: move layout into page function
  cmd: remove "want_layout" field
  ui-shared: cache errors for "dynamic TTL"

 cgit.c        | 36 +++++++-----------------------------
 cmd.c         | 48 ++++++++++++++++++++++++------------------------
 cmd.h         |  1 -
 html.c        |  7 -------
 html.h        |  1 -
 ui-blob.c     | 12 ++++++++----
 ui-clone.c    | 10 +++++-----
 ui-commit.c   |  8 ++++++--
 ui-diff.c     | 19 ++++++++++++++-----
 ui-log.c      |  5 ++++-
 ui-patch.c    | 16 ++++++++--------
 ui-plain.c    | 10 +++++-----
 ui-refs.c     |  3 ++-
 ui-repolist.c |  5 ++++-
 ui-shared.c   | 27 +++++++++++++++++++++++++++
 ui-shared.h   |  5 +++++
 ui-snapshot.c | 29 +++++++++--------------------
 ui-stats.c    |  8 ++++++--
 ui-summary.c  | 10 ++++++++--
 ui-tag.c      | 13 ++++++++++---
 ui-tree.c     | 23 ++++++++++++++++++-----
 21 files changed, 170 insertions(+), 126 deletions(-)

-- 
2.5.0.466.g9af26fa

[PATCH v2 01/22] ui-shared: add cgit_print_error_page() function

[john]

This will allow us to generate error responses with the correct HTTP
response code without needing all of the layout boilerplate.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 ui-shared.c | 14 ++++++++++++++
 ui-shared.h |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/ui-shared.c b/ui-shared.c
index 36fcb21..06dd0a8 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -788,6 +788,20 @@ void cgit_print_docend(void)
 	html("</body>\n</html>\n");
 }
 
+void cgit_print_error_page(int code, const char *msg, const char *fmt, ...)
+{
+	va_list ap;
+	ctx.page.status = code;
+	ctx.page.statusmsg = msg;
+	cgit_print_http_headers();
+	cgit_print_docstart();
+	cgit_print_pageheader();
+	va_start(ap, fmt);
+	cgit_vprint_error(fmt, ap);
+	va_end(ap);
+	cgit_print_docend();
+}
+
 static void add_clone_urls(void (*fn)(const char *), char *txt, char *suffix)
 {
 	struct strbuf **url_list = strbuf_split_str(txt, ' ', 0);
diff --git a/ui-shared.h b/ui-shared.h
index d8a3551..652685e 100644
--- a/ui-shared.h
+++ b/ui-shared.h
@@ -64,6 +64,8 @@ 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();
+__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);
 extern void cgit_print_filemode(unsigned short mode);
 extern void cgit_print_snapshot_links(const char *repo, const char *head,
-- 
2.5.0.466.g9af26fa

[PATCH v2 02/22] cgit: use cgit_print_error_page() where appropriate

[john]

These are more-or-less one-to-one translations but in the final hunk we
gain an HTTP error code where we used to send "200 OK", which is an
improvement.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 cgit.c | 27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)

diff --git a/cgit.c b/cgit.c
index d84b4be..3a97563 100644
--- a/cgit.c
+++ b/cgit.c
@@ -614,13 +614,8 @@ static int prepare_repo_cmd(void)
 	if (get_sha1(ctx.qry.head, sha1)) {
 		char *tmp = xstrdup(ctx.qry.head);
 		ctx.qry.head = ctx.repo->defbranch;
-		ctx.page.status = 404;
-		ctx.page.statusmsg = "Not found";
-		cgit_print_http_headers();
-		cgit_print_docstart();
-		cgit_print_pageheader();
-		cgit_print_error("Invalid branch: %s", tmp);
-		cgit_print_docend();
+		cgit_print_error_page(404, "Not found",
+				"Invalid branch: %s", tmp);
 		free(tmp);
 		return 1;
 	}
@@ -713,18 +708,13 @@ static void process_request(void)
 	cmd = cgit_get_cmd();
 	if (!cmd) {
 		ctx.page.title = "cgit error";
-		ctx.page.status = 404;
-		ctx.page.statusmsg = "Not found";
-		cgit_print_http_headers();
-		cgit_print_docstart();
-		cgit_print_pageheader();
-		cgit_print_error("Invalid request");
-		cgit_print_docend();
+		cgit_print_error_page(404, "Not found", "Invalid request");
 		return;
 	}
 
 	if (!ctx.cfg.enable_http_clone && cmd->is_clone) {
-		html_status(404, "Not found", 0);
+		ctx.page.title = "cgit error";
+		cgit_print_error_page(404, "Not found", "Invalid request");
 		return;
 	}
 
@@ -735,11 +725,8 @@ static void process_request(void)
 	ctx.qry.vpath = cmd->want_vpath ? ctx.qry.path : NULL;
 
 	if (cmd->want_repo && !ctx.repo) {
-		cgit_print_http_headers();
-		cgit_print_docstart();
-		cgit_print_pageheader();
-		cgit_print_error("No repository selected");
-		cgit_print_docend();
+		cgit_print_error_page(400, "Bad request",
+				"No repository selected");
 		return;
 	}
 
-- 
2.5.0.466.g9af26fa

[PATCH v2 03/22] clone: use cgit_print_error_page() instead of html_status()

[john]

This provides a formatted error response rather than a simple HTTP
error.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 ui-clone.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/ui-clone.c b/ui-clone.c
index e4ddd34..f7b0b04 100644
--- a/ui-clone.c
+++ b/ui-clone.c
@@ -57,13 +57,13 @@ static void send_file(const char *path)
 	if (stat(path, &st)) {
 		switch (errno) {
 		case ENOENT:
-			html_status(404, "Not found", 0);
+			cgit_print_error_page(404, "Not found", "Not found");
 			break;
 		case EACCES:
-			html_status(403, "Forbidden", 0);
+			cgit_print_error_page(403, "Forbidden", "Forbidden");
 			break;
 		default:
-			html_status(400, "Bad request", 0);
+			cgit_print_error_page(400, "Bad request", "Bad request");
 		}
 		return;
 	}
@@ -78,7 +78,7 @@ static void send_file(const char *path)
 void cgit_clone_info(void)
 {
 	if (!ctx.qry.path || strcmp(ctx.qry.path, "refs")) {
-		html_status(400, "Bad request", 0);
+		cgit_print_error_page(400, "Bad request", "Bad request");
 		return;
 	}
 
@@ -91,7 +91,7 @@ void cgit_clone_info(void)
 void cgit_clone_objects(void)
 {
 	if (!ctx.qry.path) {
-		html_status(400, "Bad request", 0);
+		cgit_print_error_page(400, "Bad request", "Bad request");
 		return;
 	}
 
-- 
2.5.0.466.g9af26fa

[PATCH v2 04/22] plain: use cgit_print_error_page() instead of html_status()

[john]

This provides a formatted error response rather than a simple HTTP
error.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 ui-plain.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/ui-plain.c b/ui-plain.c
index 3a2cb47..0daa7bf 100644
--- a/ui-plain.c
+++ b/ui-plain.c
@@ -66,13 +66,13 @@ static int print_object(const unsigned char *sha1, const char *path)
 
 	type = sha1_object_info(sha1, &size);
 	if (type == OBJ_BAD) {
-		html_status(404, "Not found", 0);
+		cgit_print_error_page(404, "Not found", "Not found");
 		return 0;
 	}
 
 	buf = read_sha1_file(sha1, &type, &size);
 	if (!buf) {
-		html_status(404, "Not found", 0);
+		cgit_print_error_page(404, "Not found", "Not found");
 		return 0;
 	}
 	ctx.page.mimetype = NULL;
@@ -225,12 +225,12 @@ void cgit_print_plain(void)
 		rev = ctx.qry.head;
 
 	if (get_sha1(rev, sha1)) {
-		html_status(404, "Not found", 0);
+		cgit_print_error_page(404, "Not found", "Not found");
 		return;
 	}
 	commit = lookup_commit_reference(sha1);
 	if (!commit || parse_commit(commit)) {
-		html_status(404, "Not found", 0);
+		cgit_print_error_page(404, "Not found", "Not found");
 		return;
 	}
 	if (!path_items.match) {
@@ -243,7 +243,7 @@ void cgit_print_plain(void)
 		walk_tree_ctx.match_baselen = basedir_len(path_items.match);
 	read_tree_recursive(commit->tree, "", 0, 0, &paths, walk_tree, &walk_tree_ctx);
 	if (!walk_tree_ctx.match)
-		html_status(404, "Not found", 0);
+		cgit_print_error_page(404, "Not found", "Not found");
 	else if (walk_tree_ctx.match == 2)
 		print_dir_tail();
 }
-- 
2.5.0.466.g9af26fa

[PATCH v2 05/22] snapshot: use cgit_print_error_page() instead of html_status()

[john]

This provides a formatted error response rather than a simple HTTP
error.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 ui-snapshot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ui-snapshot.c b/ui-snapshot.c
index cb34f4b..bf4bcd7 100644
--- a/ui-snapshot.c
+++ b/ui-snapshot.c
@@ -213,7 +213,7 @@ void cgit_print_snapshot(const char *head, const char *hex,
 	if (!hex && dwim) {
 		hex = get_ref_from_filename(ctx.repo->url, filename, f);
 		if (hex == NULL) {
-			html_status(404, "Not found", 0);
+			cgit_print_error_page(404, "Not found", "Not found");
 			return;
 		}
 		prefix = xstrdup(filename);
-- 
2.5.0.466.g9af26fa

[PATCH v2 06/22] blob: use cgit_print_error_page() to add HTTP headers

[john]

This is a bugfix as well as an improvement to the HTTP status code
handling since previously we would not print HTTP headers on any of
these code paths.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 ui-blob.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/ui-blob.c b/ui-blob.c
index 388a017..b333f86 100644
--- a/ui-blob.c
+++ b/ui-blob.c
@@ -126,12 +126,14 @@ void cgit_print_blob(const char *hex, char *path, const char *head, int file_onl
 
 	if (hex) {
 		if (get_sha1_hex(hex, sha1)) {
-			cgit_print_error("Bad hex value: %s", hex);
+			cgit_print_error_page(400, "Bad request",
+					"Bad hex value: %s", hex);
 			return;
 		}
 	} else {
 		if (get_sha1(head, sha1)) {
-			cgit_print_error("Bad ref: %s", head);
+			cgit_print_error_page(404, "Not found",
+					"Bad ref: %s", head);
 			return;
 		}
 	}
@@ -145,13 +147,15 @@ void cgit_print_blob(const char *hex, char *path, const char *head, int file_onl
 	}
 
 	if (type == OBJ_BAD) {
-		cgit_print_error("Bad object name: %s", hex);
+		cgit_print_error_page(404, "Not found",
+				"Bad object name: %s", hex);
 		return;
 	}
 
 	buf = read_sha1_file(sha1, &type, &size);
 	if (!buf) {
-		cgit_print_error("Error reading object %s", hex);
+		cgit_print_error_page(500, "Internal server error",
+				"Error reading object %s", hex);
 		return;
 	}
 
-- 
2.5.0.466.g9af26fa

[PATCH v2 07/22] patch: use cgit_print_error_page() for HTTP status codes

[john]

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 ui-patch.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/ui-patch.c b/ui-patch.c
index 57ca2f8..430231e 100644
--- a/ui-patch.c
+++ b/ui-patch.c
@@ -25,26 +25,26 @@ void cgit_print_patch(const char *new_rev, const char *old_rev,
 		new_rev = ctx.qry.head;
 
 	if (get_sha1(new_rev, new_rev_sha1)) {
-		cgit_print_http_headers();
-		cgit_print_error("Bad object id: %s", new_rev);
+		cgit_print_error_page(404, "Not found",
+				"Bad object id: %s", new_rev);
 		return;
 	}
 	commit = lookup_commit_reference(new_rev_sha1);
 	if (!commit) {
-		cgit_print_http_headers();
-		cgit_print_error("Bad commit reference: %s", new_rev);
+		cgit_print_error_page(404, "Not found",
+				"Bad commit reference: %s", new_rev);
 		return;
 	}
 
 	if (old_rev) {
 		if (get_sha1(old_rev, old_rev_sha1)) {
-			cgit_print_http_headers();
-			cgit_print_error("Bad object id: %s", old_rev);
+			cgit_print_error_page(404, "Not found",
+					"Bad object id: %s", old_rev);
 			return;
 		}
 		if (!lookup_commit_reference(old_rev_sha1)) {
-			cgit_print_http_headers();
-			cgit_print_error("Bad commit reference: %s", old_rev);
+			cgit_print_error_page(404, "Not found",
+					"Bad commit reference: %s", old_rev);
 			return;
 		}
 	} else if (commit->parents && commit->parents->item) {
-- 
2.5.0.466.g9af26fa

[PATCH v2 08/22] snapshot: use cgit_print_error_page() for HTTP status codes

[john]

This is a bugfix as well as an improvement to the HTTP status code
handling since previously we would not print HTTP headers on any of
these code paths.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 ui-snapshot.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/ui-snapshot.c b/ui-snapshot.c
index bf4bcd7..9bcf13d 100644
--- a/ui-snapshot.c
+++ b/ui-snapshot.c
@@ -112,11 +112,13 @@ static int make_snapshot(const struct cgit_snapshot_format *format,
 	unsigned char sha1[20];
 
 	if (get_sha1(hex, sha1)) {
-		cgit_print_error("Bad object id: %s", hex);
+		cgit_print_error_page(404, "Not found",
+				"Bad object id: %s", hex);
 		return 1;
 	}
 	if (!lookup_commit_reference(sha1)) {
-		cgit_print_error("Not a commit reference: %s", hex);
+		cgit_print_error_page(400, "Bad request",
+				"Not a commit reference: %s", hex);
 		return 1;
 	}
 	ctx.page.etag = sha1_to_hex(sha1);
-- 
2.5.0.466.g9af26fa

[PATCH v2 09/22] snapshot: don't reimplement cgit_print_error_page()

[john]

cgit_print_error_page() has the advantage that it sets a suitable HTTP
status code for the response.  Note that setting "mimetype" is redundant
here since it cannot have changed since being initialized in
cgit.c::prepare_context(), so we do not need to worry that
cgit_print_error_page() does not set it.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 ui-snapshot.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/ui-snapshot.c b/ui-snapshot.c
index 9bcf13d..f68e877 100644
--- a/ui-snapshot.c
+++ b/ui-snapshot.c
@@ -180,21 +180,6 @@ out:
 	return result ? strbuf_detach(&snapshot, NULL) : NULL;
 }
 
-__attribute__((format (printf, 1, 2)))
-static void show_error(char *fmt, ...)
-{
-	va_list ap;
-
-	ctx.page.mimetype = "text/html";
-	cgit_print_http_headers();
-	cgit_print_docstart();
-	cgit_print_pageheader();
-	va_start(ap, fmt);
-	cgit_vprint_error(fmt, ap);
-	va_end(ap);
-	cgit_print_docend();
-}
-
 void cgit_print_snapshot(const char *head, const char *hex,
 			 const char *filename, int dwim)
 {
@@ -202,13 +187,15 @@ void cgit_print_snapshot(const char *head, const char *hex,
 	char *prefix = NULL;
 
 	if (!filename) {
-		show_error("No snapshot name specified");
+		cgit_print_error_page(400, "Bad request",
+				"No snapshot name specified");
 		return;
 	}
 
 	f = get_format(filename);
 	if (!f) {
-		show_error("Unsupported snapshot format: %s", filename);
+		cgit_print_error_page(400, "Bad request",
+				"Unsupported snapshot format: %s", filename);
 		return;
 	}
 
-- 
2.5.0.466.g9af26fa

[PATCH v2 10/22] html: remove html_status()

[john]

This is now unused.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 html.c | 7 -------
 html.h | 1 -
 2 files changed, 8 deletions(-)

diff --git a/html.c b/html.c
index a47cff0..959148c 100644
--- a/html.c
+++ b/html.c
@@ -121,13 +121,6 @@ void html_vtxtf(const char *format, va_list ap)
 	strbuf_release(&buf);
 }
 
-void html_status(int code, const char *msg, int more_headers)
-{
-	htmlf("Status: %d %s\n", code, msg);
-	if (!more_headers)
-		html("\n");
-}
-
 void html_txt(const char *txt)
 {
 	const char *t = txt;
diff --git a/html.h b/html.h
index be3b311..c554763 100644
--- a/html.h
+++ b/html.h
@@ -18,7 +18,6 @@ extern void html_vtxtf(const char *format, va_list ap);
 __attribute__((format (printf,1,2)))
 extern void html_attrf(const char *format,...);
 
-extern void html_status(int code, const char *msg, int more_headers);
 extern void html_txt(const char *txt);
 extern void html_ntxt(int len, const char *txt);
 extern void html_attr(const char *txt);
-- 
2.5.0.466.g9af26fa

[PATCH v2 11/22] ui-shared: add cgit_print_layout_{start,end}()

[john]

These will avoid needing to call three functions to start page layout in
subsequent patches when we move the layout setup into each individual
page.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 ui-shared.c | 12 ++++++++++++
 ui-shared.h |  3 +++
 2 files changed, 15 insertions(+)

diff --git a/ui-shared.c b/ui-shared.c
index 06dd0a8..de06256 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -802,6 +802,18 @@ void cgit_print_error_page(int code, const char *msg, const char *fmt, ...)
 	cgit_print_docend();
 }
 
+void cgit_print_layout_start(void)
+{
+	cgit_print_http_headers();
+	cgit_print_docstart();
+	cgit_print_pageheader();
+}
+
+void cgit_print_layout_end(void)
+{
+	cgit_print_docend();
+}
+
 static void add_clone_urls(void (*fn)(const char *), char *txt, char *suffix)
 {
 	struct strbuf **url_list = strbuf_split_str(txt, ' ', 0);
diff --git a/ui-shared.h b/ui-shared.h
index 652685e..246678b 100644
--- a/ui-shared.h
+++ b/ui-shared.h
@@ -54,6 +54,9 @@ extern void cgit_object_link(struct object *obj);
 extern void cgit_submodule_link(const char *class, char *path,
 				const char *rev);
 
+extern void cgit_print_layout_start(void);
+extern void cgit_print_layout_end(void);
+
 __attribute__((format (printf,1,2)))
 extern void cgit_print_error(const char *fmt, ...);
 __attribute__((format (printf,1,0)))
-- 
2.5.0.466.g9af26fa

[PATCH v2 12/22] about: move layout into page functions

[john]

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 cmd.c         | 2 +-
 ui-repolist.c | 5 ++++-
 ui-summary.c  | 8 ++++++--
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/cmd.c b/cmd.c
index 315edc3..616890c 100644
--- a/cmd.c
+++ b/cmd.c
@@ -155,7 +155,7 @@ struct cgit_cmd *cgit_get_cmd(void)
 	static struct cgit_cmd cmds[] = {
 		def_cmd(HEAD, 1, 0, 0, 1),
 		def_cmd(atom, 1, 0, 0, 0),
-		def_cmp(about, 0, 1, 0, 0),
+		def_cmp(about, 0, 0, 0, 0),
 		def_cmd(blob, 1, 0, 0, 0),
 		def_cmd(commit, 1, 1, 1, 0),
 		def_cmd(diff, 1, 1, 1, 0),
diff --git a/ui-repolist.c b/ui-repolist.c
index 43253ed..ac1b3e3 100644
--- a/ui-repolist.c
+++ b/ui-repolist.c
@@ -345,9 +345,12 @@ void cgit_print_repolist(void)
 
 void cgit_print_site_readme(void)
 {
+	cgit_print_layout_start();
 	if (!ctx.cfg.root_readme)
-		return;
+		goto done;
 	cgit_open_filter(ctx.cfg.about_filter, ctx.cfg.root_readme);
 	html_include(ctx.cfg.root_readme);
 	cgit_close_filter(ctx.cfg.about_filter);
+done:
+	cgit_print_layout_end();
 }
diff --git a/ui-summary.c b/ui-summary.c
index a5c7078..cd1fef5 100644
--- a/ui-summary.c
+++ b/ui-summary.c
@@ -101,8 +101,9 @@ void cgit_print_repo_readme(char *path)
 	char *filename, *ref;
 	int free_filename = 0;
 
+	cgit_print_layout_start();
 	if (ctx.repo->readme.nr == 0)
-		return;
+		goto done;
 
 	filename = ctx.repo->readme.items[0].string;
 	ref = ctx.repo->readme.items[0].util;
@@ -111,7 +112,7 @@ void cgit_print_repo_readme(char *path)
 		free_filename = 1;
 		filename = append_readme_path(filename, ref, path);
 		if (!filename)
-			return;
+			goto done;
 	}
 
 	/* Print the calculated readme, either from the git repo or from the
@@ -128,4 +129,7 @@ void cgit_print_repo_readme(char *path)
 	html("</div>");
 	if (free_filename)
 		free(filename);
+
+done:
+	cgit_print_layout_end();
 }
-- 
2.5.0.466.g9af26fa

[PATCH v2 13/22] commit: move layout into page function

[john]

This allows us to return a proper HTTP status code when an object is not
found by switching from cgit_print_error() to cgit_print_error_page().

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 cmd.c       | 2 +-
 ui-commit.c | 8 ++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/cmd.c b/cmd.c
index 616890c..1b42a47 100644
--- a/cmd.c
+++ b/cmd.c
@@ -157,7 +157,7 @@ struct cgit_cmd *cgit_get_cmd(void)
 		def_cmd(atom, 1, 0, 0, 0),
 		def_cmp(about, 0, 0, 0, 0),
 		def_cmd(blob, 1, 0, 0, 0),
-		def_cmd(commit, 1, 1, 1, 0),
+		def_cmd(commit, 1, 0, 1, 0),
 		def_cmd(diff, 1, 1, 1, 0),
 		def_cmd(info, 1, 0, 0, 1),
 		def_cmd(log, 1, 1, 1, 0),
diff --git a/ui-commit.c b/ui-commit.c
index d5a888d..2bca7a0 100644
--- a/ui-commit.c
+++ b/ui-commit.c
@@ -27,12 +27,14 @@ void cgit_print_commit(char *hex, const char *prefix)
 		hex = ctx.qry.head;
 
 	if (get_sha1(hex, sha1)) {
-		cgit_print_error("Bad object id: %s", hex);
+		cgit_print_error_page(400, "Bad request",
+				"Bad object id: %s", hex);
 		return;
 	}
 	commit = lookup_commit_reference(sha1);
 	if (!commit) {
-		cgit_print_error("Bad commit reference: %s", hex);
+		cgit_print_error_page(404, "Not found",
+				"Bad commit reference: %s", hex);
 		return;
 	}
 	info = cgit_parse_commit(commit);
@@ -41,6 +43,7 @@ void cgit_print_commit(char *hex, const char *prefix)
 
 	load_ref_decorations(DECORATE_FULL_REFS);
 
+	cgit_print_layout_start();
 	cgit_print_diff_ctrls();
 	html("<table summary='commit info' class='commit-info'>\n");
 	html("<tr><th>author</th><td>");
@@ -139,4 +142,5 @@ void cgit_print_commit(char *hex, const char *prefix)
 	}
 	strbuf_release(&notes);
 	cgit_free_commitinfo(info);
+	cgit_print_layout_end();
 }
-- 
2.5.0.466.g9af26fa

[PATCH v2 14/22] diff: move layout to page function

[john]

The existing "show_ctrls" flag is used to control whether we are running
in an existing page or control the page ourselves.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 cmd.c     |  2 +-
 ui-diff.c | 19 ++++++++++++++-----
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/cmd.c b/cmd.c
index 1b42a47..d9f6eff 100644
--- a/cmd.c
+++ b/cmd.c
@@ -158,7 +158,7 @@ struct cgit_cmd *cgit_get_cmd(void)
 		def_cmp(about, 0, 0, 0, 0),
 		def_cmd(blob, 1, 0, 0, 0),
 		def_cmd(commit, 1, 0, 1, 0),
-		def_cmd(diff, 1, 1, 1, 0),
+		def_cmd(diff, 1, 0, 1, 0),
 		def_cmd(info, 1, 0, 0, 1),
 		def_cmd(log, 1, 1, 1, 0),
 		def_cmd(ls_cache, 0, 0, 0, 0),
diff --git a/ui-diff.c b/ui-diff.c
index caebd5d..5d18296 100644
--- a/ui-diff.c
+++ b/ui-diff.c
@@ -403,19 +403,22 @@ void cgit_print_diff(const char *new_rev, const char *old_rev,
 	if (!new_rev)
 		new_rev = ctx.qry.head;
 	if (get_sha1(new_rev, new_rev_sha1)) {
-		cgit_print_error("Bad object name: %s", new_rev);
+		cgit_print_error_page(404, "Not found",
+			"Bad object name: %s", new_rev);
 		return;
 	}
 	commit = lookup_commit_reference(new_rev_sha1);
 	if (!commit || parse_commit(commit)) {
-		cgit_print_error("Bad commit: %s", sha1_to_hex(new_rev_sha1));
+		cgit_print_error_page(404, "Not found",
+			"Bad commit: %s", sha1_to_hex(new_rev_sha1));
 		return;
 	}
 	new_tree_sha1 = commit->tree->object.sha1;
 
 	if (old_rev) {
 		if (get_sha1(old_rev, old_rev_sha1)) {
-			cgit_print_error("Bad object name: %s", old_rev);
+			cgit_print_error_page(404, "Not found",
+				"Bad object name: %s", old_rev);
 			return;
 		}
 	} else if (commit->parents && commit->parents->item) {
@@ -427,7 +430,8 @@ void cgit_print_diff(const char *new_rev, const char *old_rev,
 	if (!is_null_sha1(old_rev_sha1)) {
 		commit2 = lookup_commit_reference(old_rev_sha1);
 		if (!commit2 || parse_commit(commit2)) {
-			cgit_print_error("Bad commit: %s", sha1_to_hex(old_rev_sha1));
+			cgit_print_error_page(404, "Not found",
+				"Bad commit: %s", sha1_to_hex(old_rev_sha1));
 			return;
 		}
 		old_tree_sha1 = commit2->tree->object.sha1;
@@ -460,8 +464,10 @@ void cgit_print_diff(const char *new_rev, const char *old_rev,
 	difftype = ctx.qry.has_difftype ? ctx.qry.difftype : ctx.cfg.difftype;
 	use_ssdiff = difftype == DIFF_SSDIFF;
 
-	if (show_ctrls)
+	if (show_ctrls) {
+		cgit_print_layout_start();
 		cgit_print_diff_ctrls();
+	}
 
 	/*
 	 * Clicking on a link to a file in the diff stat should show a diff
@@ -489,4 +495,7 @@ void cgit_print_diff(const char *new_rev, const char *old_rev,
 	if (!use_ssdiff)
 		html("</td></tr>");
 	html("</table>");
+
+	if (show_ctrls)
+		cgit_print_layout_end();
 }
-- 
2.5.0.466.g9af26fa

[PATCH v2 15/22] log: move layout into page function

[john]

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 cmd.c    | 2 +-
 ui-log.c | 5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/cmd.c b/cmd.c
index d9f6eff..0bf4c76 100644
--- a/cmd.c
+++ b/cmd.c
@@ -160,7 +160,7 @@ struct cgit_cmd *cgit_get_cmd(void)
 		def_cmd(commit, 1, 0, 1, 0),
 		def_cmd(diff, 1, 0, 1, 0),
 		def_cmd(info, 1, 0, 0, 1),
-		def_cmd(log, 1, 1, 1, 0),
+		def_cmd(log, 1, 0, 1, 0),
 		def_cmd(ls_cache, 0, 0, 0, 0),
 		def_cmd(objects, 1, 0, 0, 1),
 		def_cmd(patch, 1, 0, 1, 0),
diff --git a/ui-log.c b/ui-log.c
index 6bff948..0782478 100644
--- a/ui-log.c
+++ b/ui-log.c
@@ -442,8 +442,10 @@ void cgit_print_log(const char *tip, int ofs, int cnt, char *grep, char *pattern
 	compile_grep_patterns(&rev.grep_filter);
 	prepare_revision_walk(&rev);
 
-	if (pager)
+	if (pager) {
+		cgit_print_layout_start();
 		html("<table class='list nowrap'>");
+	}
 
 	html("<tr class='nohover'>");
 	if (commit_graph)
@@ -526,6 +528,7 @@ void cgit_print_log(const char *tip, int ofs, int cnt, char *grep, char *pattern
 			html("</li>");
 		}
 		html("</ul>");
+		cgit_print_layout_end();
 	} else if ((commit = get_revision(&rev)) != NULL) {
 		htmlf("<tr class='nohover'><td colspan='%d'>", columns);
 		cgit_log_link("[...]", NULL, NULL, ctx.qry.head, NULL,
-- 
2.5.0.466.g9af26fa

[PATCH v2 16/22] refs: move layout to page function

[john]

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 cmd.c     | 2 +-
 ui-refs.c | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/cmd.c b/cmd.c
index 0bf4c76..ce66af9 100644
--- a/cmd.c
+++ b/cmd.c
@@ -166,7 +166,7 @@ struct cgit_cmd *cgit_get_cmd(void)
 		def_cmd(patch, 1, 0, 1, 0),
 		def_cmd(plain, 1, 0, 0, 0),
 		def_cmd(rawdiff, 1, 0, 1, 0),
-		def_cmd(refs, 1, 1, 0, 0),
+		def_cmd(refs, 1, 0, 0, 0),
 		def_cmd(repolist, 0, 0, 0, 0),
 		def_cmd(snapshot, 1, 0, 0, 0),
 		def_cmd(stats, 1, 1, 1, 0),
diff --git a/ui-refs.c b/ui-refs.c
index 73a187b..be3572c 100644
--- a/ui-refs.c
+++ b/ui-refs.c
@@ -236,7 +236,7 @@ void cgit_print_tags(int maxcount)
 
 void cgit_print_refs(void)
 {
-
+	cgit_print_layout_start();
 	html("<table class='list nowrap'>");
 
 	if (ctx.qry.path && starts_with(ctx.qry.path, "heads"))
@@ -249,4 +249,5 @@ void cgit_print_refs(void)
 		cgit_print_tags(0);
 	}
 	html("</table>");
+	cgit_print_layout_end();
 }
-- 
2.5.0.466.g9af26fa

[PATCH v2 17/22] stats: move layout into page function

[john]

This also allows us to return proper HTTP error codes for invalid
requests.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 cmd.c      | 2 +-
 ui-stats.c | 8 ++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/cmd.c b/cmd.c
index ce66af9..4f8e71d 100644
--- a/cmd.c
+++ b/cmd.c
@@ -169,7 +169,7 @@ struct cgit_cmd *cgit_get_cmd(void)
 		def_cmd(refs, 1, 0, 0, 0),
 		def_cmd(repolist, 0, 0, 0, 0),
 		def_cmd(snapshot, 1, 0, 0, 0),
-		def_cmd(stats, 1, 1, 1, 0),
+		def_cmd(stats, 1, 0, 1, 0),
 		def_cmd(summary, 1, 1, 0, 0),
 		def_cmd(tag, 1, 1, 0, 0),
 		def_cmd(tree, 1, 1, 1, 0),
diff --git a/ui-stats.c b/ui-stats.c
index 9cd8247..74ce0f7 100644
--- a/ui-stats.c
+++ b/ui-stats.c
@@ -372,11 +372,13 @@ void cgit_show_stats(void)
 
 	i = cgit_find_stats_period(code, &period);
 	if (!i) {
-		cgit_print_error("Unknown statistics type: %c", code[0]);
+		cgit_print_error_page(404, "Not found",
+			"Unknown statistics type: %c", code[0]);
 		return;
 	}
 	if (i > ctx.repo->max_stats) {
-		cgit_print_error("Statistics type disabled: %s", period->name);
+		cgit_print_error_page(400, "Bad request",
+			"Statistics type disabled: %s", period->name);
 		return;
 	}
 	authors = collect_stats(period);
@@ -387,6 +389,7 @@ void cgit_show_stats(void)
 	if (!top)
 		top = 10;
 
+	cgit_print_layout_start();
 	html("<div class='cgit-panel'>");
 	html("<b>stat options</b>");
 	html("<form method='get' action=''>");
@@ -421,5 +424,6 @@ void cgit_show_stats(void)
 	}
 	html("</h2>");
 	print_authors(&authors, top, period);
+	cgit_print_layout_end();
 }
 
-- 
2.5.0.466.g9af26fa

[PATCH v2 18/22] summary: move layout into page function

[john]

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 cmd.c        | 2 +-
 ui-summary.c | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/cmd.c b/cmd.c
index 4f8e71d..928c8aa 100644
--- a/cmd.c
+++ b/cmd.c
@@ -170,7 +170,7 @@ struct cgit_cmd *cgit_get_cmd(void)
 		def_cmd(repolist, 0, 0, 0, 0),
 		def_cmd(snapshot, 1, 0, 0, 0),
 		def_cmd(stats, 1, 0, 1, 0),
-		def_cmd(summary, 1, 1, 0, 0),
+		def_cmd(summary, 1, 0, 0, 0),
 		def_cmd(tag, 1, 1, 0, 0),
 		def_cmd(tree, 1, 1, 1, 0),
 	};
diff --git a/ui-summary.c b/ui-summary.c
index cd1fef5..fb04dc3 100644
--- a/ui-summary.c
+++ b/ui-summary.c
@@ -48,6 +48,7 @@ void cgit_print_summary(void)
 	if (ctx.repo->enable_log_linecount)
 		columns++;
 
+	cgit_print_layout_start();
 	html("<table summary='repository info' class='list nowrap'>");
 	cgit_print_branches(ctx.cfg.summary_branches);
 	htmlf("<tr class='nohover'><td colspan='%d'>&nbsp;</td></tr>", columns);
@@ -60,6 +61,7 @@ void cgit_print_summary(void)
 	urls = 0;
 	cgit_add_clone_urls(print_url);
 	html("</table>");
+	cgit_print_layout_end();
 }
 
 /* The caller must free the return value. */
-- 
2.5.0.466.g9af26fa

[PATCH v2 19/22] tag: move layout into page function

[john]

This also allows us to return proper HTTP error codes when something
goes wrong.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 cmd.c    |  2 +-
 ui-tag.c | 13 ++++++++++---
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/cmd.c b/cmd.c
index 928c8aa..54961d9 100644
--- a/cmd.c
+++ b/cmd.c
@@ -171,7 +171,7 @@ struct cgit_cmd *cgit_get_cmd(void)
 		def_cmd(snapshot, 1, 0, 0, 0),
 		def_cmd(stats, 1, 0, 1, 0),
 		def_cmd(summary, 1, 0, 0, 0),
-		def_cmd(tag, 1, 1, 0, 0),
+		def_cmd(tag, 1, 0, 0, 0),
 		def_cmd(tree, 1, 1, 1, 0),
 	};
 	int i;
diff --git a/ui-tag.c b/ui-tag.c
index c1d1738..0afc663 100644
--- a/ui-tag.c
+++ b/ui-tag.c
@@ -52,20 +52,24 @@ void cgit_print_tag(char *revname)
 
 	strbuf_addf(&fullref, "refs/tags/%s", revname);
 	if (get_sha1(fullref.buf, sha1)) {
-		cgit_print_error("Bad tag reference: %s", revname);
+		cgit_print_error_page(404, "Not found",
+			"Bad tag reference: %s", revname);
 		goto cleanup;
 	}
 	obj = parse_object(sha1);
 	if (!obj) {
-		cgit_print_error("Bad object id: %s", sha1_to_hex(sha1));
+		cgit_print_error_page(500, "Internal server error",
+			"Bad object id: %s", sha1_to_hex(sha1));
 		goto cleanup;
 	}
 	if (obj->type == OBJ_TAG) {
 		tag = lookup_tag(sha1);
 		if (!tag || parse_tag(tag) || !(info = cgit_parse_tag(tag))) {
-			cgit_print_error("Bad tag object: %s", revname);
+			cgit_print_error_page(500, "Internal server error",
+				"Bad tag object: %s", revname);
 			goto cleanup;
 		}
+		cgit_print_layout_start();
 		html("<table class='commit-info'>\n");
 		htmlf("<tr><td>tag name</td><td>");
 		html_txt(revname);
@@ -93,7 +97,9 @@ void cgit_print_tag(char *revname)
 			print_download_links(revname);
 		html("</table>\n");
 		print_tag_content(info->msg);
+		cgit_print_layout_end();
 	} else {
+		cgit_print_layout_start();
 		html("<table class='commit-info'>\n");
 		htmlf("<tr><td>tag name</td><td>");
 		html_txt(revname);
@@ -104,6 +110,7 @@ void cgit_print_tag(char *revname)
 		if (ctx.repo->snapshots)
 			print_download_links(revname);
 		html("</table>\n");
+		cgit_print_layout_end();
 	}
 
 cleanup:
-- 
2.5.0.466.g9af26fa

[PATCH v2 20/22] tree: move layout into page function

[john]

This also allows us to return proper HTTP error codes when the requested
tree is not found and display an error message in one case (invalid path
inside valid commit) where we previously just displayed an empty page.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 cmd.c     |  2 +-
 ui-tree.c | 23 ++++++++++++++++++-----
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/cmd.c b/cmd.c
index 54961d9..3c90d0b 100644
--- a/cmd.c
+++ b/cmd.c
@@ -172,7 +172,7 @@ struct cgit_cmd *cgit_get_cmd(void)
 		def_cmd(stats, 1, 0, 1, 0),
 		def_cmd(summary, 1, 0, 0, 0),
 		def_cmd(tag, 1, 0, 0, 0),
-		def_cmd(tree, 1, 1, 1, 0),
+		def_cmd(tree, 1, 0, 1, 0),
 	};
 	int i;
 
diff --git a/ui-tree.c b/ui-tree.c
index 2dbe89e..1b310d5 100644
--- a/ui-tree.c
+++ b/ui-tree.c
@@ -92,16 +92,19 @@ static void print_object(const unsigned char *sha1, char *path, const char *base
 
 	type = sha1_object_info(sha1, &size);
 	if (type == OBJ_BAD) {
-		cgit_print_error("Bad object name: %s", sha1_to_hex(sha1));
+		cgit_print_error_page(404, "Not found",
+			"Bad object name: %s", sha1_to_hex(sha1));
 		return;
 	}
 
 	buf = read_sha1_file(sha1, &type, &size);
 	if (!buf) {
-		cgit_print_error("Error reading object %s", sha1_to_hex(sha1));
+		cgit_print_error_page(500, "Internal server error",
+			"Error reading object %s", sha1_to_hex(sha1));
 		return;
 	}
 
+	cgit_print_layout_start();
 	htmlf("blob: %s (", sha1_to_hex(sha1));
 	cgit_plain_link("plain", NULL, NULL, ctx.qry.head,
 		        rev, path);
@@ -181,6 +184,7 @@ static int ls_item(const unsigned char *sha1, struct strbuf *base,
 
 static void ls_head(void)
 {
+	cgit_print_layout_start();
 	html("<table summary='tree listing' class='list'>\n");
 	html("<tr class='nohover'>");
 	html("<th class='left'>Mode</th>");
@@ -193,6 +197,7 @@ static void ls_head(void)
 static void ls_tail(void)
 {
 	html("</table>\n");
+	cgit_print_layout_end();
 }
 
 static void ls_tree(const unsigned char *sha1, char *path, struct walk_tree_context *walk_tree_ctx)
@@ -204,7 +209,8 @@ static void ls_tree(const unsigned char *sha1, char *path, struct walk_tree_cont
 
 	tree = parse_tree_indirect(sha1);
 	if (!tree) {
-		cgit_print_error("Not a tree object: %s", sha1_to_hex(sha1));
+		cgit_print_error_page(404, "Not found",
+			"Not a tree object: %s", sha1_to_hex(sha1));
 		return;
 	}
 
@@ -231,6 +237,7 @@ static int walk_tree(const unsigned char *sha1, struct strbuf *base,
 			ls_head();
 			return READ_TREE_RECURSIVE;
 		} else {
+			walk_tree_ctx->state = 2;
 			print_object(sha1, buffer, pathname, walk_tree_ctx->curr_rev);
 			return 0;
 		}
@@ -265,12 +272,14 @@ void cgit_print_tree(const char *rev, char *path)
 		rev = ctx.qry.head;
 
 	if (get_sha1(rev, sha1)) {
-		cgit_print_error("Invalid revision name: %s", rev);
+		cgit_print_error_page(404, "Not found",
+			"Invalid revision name: %s", rev);
 		return;
 	}
 	commit = lookup_commit_reference(sha1);
 	if (!commit || parse_commit(commit)) {
-		cgit_print_error("Invalid commit reference: %s", rev);
+		cgit_print_error_page(404, "Not found",
+			"Invalid commit reference: %s", rev);
 		return;
 	}
 
@@ -284,6 +293,10 @@ void cgit_print_tree(const char *rev, char *path)
 	read_tree_recursive(commit->tree, "", 0, 0, &paths, walk_tree, &walk_tree_ctx);
 	if (walk_tree_ctx.state == 1)
 		ls_tail();
+	else if (walk_tree_ctx.state == 2)
+		cgit_print_layout_end();
+	else
+		cgit_print_error_page(404, "Not found", "Path not found");
 
 cleanup:
 	free(walk_tree_ctx.curr_rev);
-- 
2.5.0.466.g9af26fa

[PATCH v2 21/22] cmd: remove "want_layout" field

[john]

No commands use this any more.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 cgit.c |  9 ---------
 cmd.c  | 48 ++++++++++++++++++++++++------------------------
 cmd.h  |  1 -
 3 files changed, 24 insertions(+), 34 deletions(-)

diff --git a/cgit.c b/cgit.c
index 3a97563..7271412 100644
--- a/cgit.c
+++ b/cgit.c
@@ -736,16 +736,7 @@ static void process_request(void)
 	if (cmd->pre)
 		cmd->pre();
 
-	if (cmd->want_layout) {
-		cgit_print_http_headers();
-		cgit_print_docstart();
-		cgit_print_pageheader();
-	}
-
 	cmd->fn();
-
-	if (cmd->want_layout)
-		cgit_print_docend();
 }
 
 static int cmp_repos(const void *a, const void *b)
diff --git a/cmd.c b/cmd.c
index 3c90d0b..05494fe 100644
--- a/cmd.c
+++ b/cmd.c
@@ -145,34 +145,34 @@ static void tree_fn(void)
 	cgit_print_tree(ctx.qry.sha1, ctx.qry.path);
 }
 
-#define def_cmp(name, want_repo, want_layout, want_vpath, is_clone) \
-	{#name, name##_fn, name##_pre, want_repo, want_layout, want_vpath, is_clone}
-#define def_cmd(name, want_repo, want_layout, want_vpath, is_clone) \
-	{#name, name##_fn, NULL, want_repo, want_layout, want_vpath, is_clone}
+#define def_cmp(name, want_repo, want_vpath, is_clone) \
+	{#name, name##_fn, name##_pre, want_repo, want_vpath, is_clone}
+#define def_cmd(name, want_repo, want_vpath, is_clone) \
+	{#name, name##_fn, NULL, want_repo, want_vpath, is_clone}
 
 struct cgit_cmd *cgit_get_cmd(void)
 {
 	static struct cgit_cmd cmds[] = {
-		def_cmd(HEAD, 1, 0, 0, 1),
-		def_cmd(atom, 1, 0, 0, 0),
-		def_cmp(about, 0, 0, 0, 0),
-		def_cmd(blob, 1, 0, 0, 0),
-		def_cmd(commit, 1, 0, 1, 0),
-		def_cmd(diff, 1, 0, 1, 0),
-		def_cmd(info, 1, 0, 0, 1),
-		def_cmd(log, 1, 0, 1, 0),
-		def_cmd(ls_cache, 0, 0, 0, 0),
-		def_cmd(objects, 1, 0, 0, 1),
-		def_cmd(patch, 1, 0, 1, 0),
-		def_cmd(plain, 1, 0, 0, 0),
-		def_cmd(rawdiff, 1, 0, 1, 0),
-		def_cmd(refs, 1, 0, 0, 0),
-		def_cmd(repolist, 0, 0, 0, 0),
-		def_cmd(snapshot, 1, 0, 0, 0),
-		def_cmd(stats, 1, 0, 1, 0),
-		def_cmd(summary, 1, 0, 0, 0),
-		def_cmd(tag, 1, 0, 0, 0),
-		def_cmd(tree, 1, 0, 1, 0),
+		def_cmd(HEAD, 1, 0, 1),
+		def_cmd(atom, 1, 0, 0),
+		def_cmp(about, 0, 0, 0),
+		def_cmd(blob, 1, 0, 0),
+		def_cmd(commit, 1, 1, 0),
+		def_cmd(diff, 1, 1, 0),
+		def_cmd(info, 1, 0, 1),
+		def_cmd(log, 1, 1, 0),
+		def_cmd(ls_cache, 0, 0, 0),
+		def_cmd(objects, 1, 0, 1),
+		def_cmd(patch, 1, 1, 0),
+		def_cmd(plain, 1, 0, 0),
+		def_cmd(rawdiff, 1, 1, 0),
+		def_cmd(refs, 1, 0, 0),
+		def_cmd(repolist, 0, 0, 0),
+		def_cmd(snapshot, 1, 0, 0),
+		def_cmd(stats, 1, 1, 0),
+		def_cmd(summary, 1, 0, 0),
+		def_cmd(tag, 1, 0, 0),
+		def_cmd(tree, 1, 1, 0),
 	};
 	int i;
 
diff --git a/cmd.h b/cmd.h
index 2507ca5..1a98089 100644
--- a/cmd.h
+++ b/cmd.h
@@ -9,7 +9,6 @@ struct cgit_cmd {
 	cgit_cmd_fn fn;
 	cgit_cmd_pre_fn pre;
 	unsigned int want_repo:1,
-		want_layout:1,
 		want_vpath:1,
 		is_clone:1;
 };
-- 
2.5.0.466.g9af26fa

[PATCH v2 22/22] ui-shared: cache errors for "dynamic TTL"

[john]

Most errors we generate are (potentially) transient, such as
non-existent object IDs so we don't want them to be cached forever.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 ui-shared.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ui-shared.c b/ui-shared.c
index de06256..89c4897 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -791,6 +791,7 @@ void cgit_print_docend(void)
 void cgit_print_error_page(int code, const char *msg, const char *fmt, ...)
 {
 	va_list ap;
+	ctx.page.expires = ctx.cfg.cache_dynamic_ttl;
 	ctx.page.status = code;
 	ctx.page.statusmsg = msg;
 	cgit_print_http_headers();
-- 
2.5.0.466.g9af26fa

[PATCH v2 00/22] HTTP status code improvements

[nicolas.dely]

Hi,

I really appreciate cgit developers who have spent time working on our 
issue[0] ;-)

Please let me a recommended cgit version/sha1 so that I can send you any 
feedback.

Regards,

Nicolas

[0]http://lists.zx2c4.com/pipermail/cgit/2015-March/002464.html


On 08/14/2015 01:47 PM, John Keeping wrote:
> Changes since v1 (starts at [0]):
> - Rebased on the latest master
> - Following Christian's patch [1], a new patch changes ui-patch.c to use
>    cgit_print_error_page() and other new patches fix similar issues in
>    ui-blob.c and ui-snapshot.c
> - The last patch is new, and uses "cache-dynamic-ttl" for error pages;
>    I'm not 100% sure about that, but it's better than the current
>    behaviour if someone tries to access a page by SHA1 before the object
>    has been pushed to the repository CGit is displaying
>
> [0] http://article.gmane.org/gmane.comp.version-control.cgit/2451
> [1] http://article.gmane.org/gmane.comp.version-control.cgit/2589
>
> John Keeping (22):
>    ui-shared: add cgit_print_error_page() function
>    cgit: use cgit_print_error_page() where appropriate
>    clone: use cgit_print_error_page() instead of html_status()
>    plain: use cgit_print_error_page() instead of html_status()
>    snapshot: use cgit_print_error_page() instead of html_status()
>    blob: use cgit_print_error_page() to add HTTP headers
>    patch: use cgit_print_error_page() for HTTP status codes
>    snapshot: use cgit_print_error_page() for HTTP status codes
>    snapshot: don't reimplement cgit_print_error_page()
>    html: remove html_status()
>    ui-shared: add cgit_print_layout_{start,end}()
>    about: move layout into page functions
>    commit: move layout into page function
>    diff: move layout to page function
>    log: move layout into page function
>    refs: move layout to page function
>    stats: move layout into page function
>    summary: move layout into page function
>    tag: move layout into page function
>    tree: move layout into page function
>    cmd: remove "want_layout" field
>    ui-shared: cache errors for "dynamic TTL"
>
>   cgit.c        | 36 +++++++-----------------------------
>   cmd.c         | 48 ++++++++++++++++++++++++------------------------
>   cmd.h         |  1 -
>   html.c        |  7 -------
>   html.h        |  1 -
>   ui-blob.c     | 12 ++++++++----
>   ui-clone.c    | 10 +++++-----
>   ui-commit.c   |  8 ++++++--
>   ui-diff.c     | 19 ++++++++++++++-----
>   ui-log.c      |  5 ++++-
>   ui-patch.c    | 16 ++++++++--------
>   ui-plain.c    | 10 +++++-----
>   ui-refs.c     |  3 ++-
>   ui-repolist.c |  5 ++++-
>   ui-shared.c   | 27 +++++++++++++++++++++++++++
>   ui-shared.h   |  5 +++++
>   ui-snapshot.c | 29 +++++++++--------------------
>   ui-stats.c    |  8 ++++++--
>   ui-summary.c  | 10 ++++++++--
>   ui-tag.c      | 13 ++++++++++---
>   ui-tree.c     | 23 ++++++++++++++++++-----
>   21 files changed, 170 insertions(+), 126 deletions(-)
>

[PATCH v2 00/22] HTTP status code improvements

[Jason]

Hi John,

Thanks for this series. The approach to caching seems like a decent
tradeoff. I'd be interested in hearing dissenting opinions on this
though. It's not to late to come up with something better before
release time.

The set looks good, and I've gone ahead and merged it to master. This
also allowed me to add another patch ontop of it getting rid of the
cmd pre() function, which was nice.

Thanks a bunch,
Jason

[PATCH v2 00/22] HTTP status code improvements

[john]

On Fri, Aug 14, 2015 at 03:56:43PM +0200, Jason A. Donenfeld wrote:
> Thanks for this series. The approach to caching seems like a decent
> tradeoff. I'd be interested in hearing dissenting opinions on this
> though. It's not to late to come up with something better before
> release time.
> 
> The set looks good, and I've gone ahead and merged it to master. This
> also allowed me to add another patch ontop of it getting rid of the
> cmd pre() function, which was nice.

Nice, but it looks like you missed a bit...

-- >8 --
Subject: [PATCH] cmd: fix command definition

The previous commit removed the "pre" field from "struct cgit_cmd" but
forgot to update this macro.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 cmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cmd.c b/cmd.c
index 20c80b0..c991092 100644
--- a/cmd.c
+++ b/cmd.c
@@ -142,7 +142,7 @@ static void tree_fn(void)
 }
 
 #define def_cmd(name, want_repo, want_vpath, is_clone) \
-	{#name, name##_fn, NULL, want_repo, want_vpath, is_clone}
+	{#name, name##_fn, want_repo, want_vpath, is_clone}
 
 struct cgit_cmd *cgit_get_cmd(void)
 {
-- 
2.5.0.466.g9af26fa

[PATCH v2 00/22] HTTP status code improvements

[list]

John Keeping <john at keeping.me.uk> on Fri, 2015/08/14 15:20:
> On Fri, Aug 14, 2015 at 03:56:43PM +0200, Jason A. Donenfeld wrote:
> > Thanks for this series. The approach to caching seems like a decent
> > tradeoff. I'd be interested in hearing dissenting opinions on this
> > though. It's not to late to come up with something better before
> > release time.
> > 
> > The set looks good, and I've gone ahead and merged it to master. This
> > also allowed me to add another patch ontop of it getting rid of the
> > cmd pre() function, which was nice.
> 
> Nice, but it looks like you missed a bit...

Ah, John was three minutes faster, so...

Reviewed-by: Christian Hesse <mail at eworm.de>

> -- >8 --
> Subject: [PATCH] cmd: fix command definition
> 
> The previous commit removed the "pre" field from "struct cgit_cmd" but
> forgot to update this macro.
> 
> Signed-off-by: John Keeping <john at keeping.me.uk>
> ---
>  cmd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/cmd.c b/cmd.c
> index 20c80b0..c991092 100644
> --- a/cmd.c
> +++ b/cmd.c
> @@ -142,7 +142,7 @@ static void tree_fn(void)
>  }
>  
>  #define def_cmd(name, want_repo, want_vpath, is_clone) \
> -	{#name, name##_fn, NULL, want_repo, want_vpath, is_clone}
> +	{#name, name##_fn, want_repo, want_vpath, is_clone}
>  
>  struct cgit_cmd *cgit_get_cmd(void)
>  {



-- 
main(a){char*c=/*    Schoene Gruesse                         */"B?IJj;MEH"
"CX:;",b;for(a/*    Chris           get my mail address:    */=0;b=c[a++];)
putchar(b-1/(/*               gcc -o sig sig.c && ./sig    */b/42*2-3)*42);}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 473 bytes
Desc: OpenPGP digital signature
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20150814/36c02f2b/attachment.asc>

[PATCH v2 00/22] HTTP status code improvements

[Jason]

Thanks guys!