[PATCH] Return HTTP status codes for error conditions for various commands
John Keeping
john at keeping.me.uk
Fri Nov 15 15:09:58 CET 2013
On Fri, Nov 15, 2013 at 03:01:25PM +0100, Dirk Best wrote:
> This returns 404 Not found for most errors, the only exception is for the stats, where it will return 400
> Bad request for unknown statistics types. Note that this patch also fixes various incomplete header errors,
> which manifest themselves like this in the web server error log: "Premature end of script headers:
> cgit.cgi".
>
> Signed-off-by: Dirk Best <mail at dirk-best.de>
> ---
This change looks fairly sensible from a quick scan through, but there
are several different changes rolled into a single patch here.
At the very least I think there are three commits in here:
1/3 Add cgit_print_pagestart() helper function
Refactor existing code to reduce duplication.
2/3 Remove want_layout from command structure
Move print_pagestart and print_docend into individual commands,
no change in functionality.
3/3 Add error messages
Change in functionality.
> cgit.c | 33 +++++----------------------------
> cmd.c | 58 ++++++++++++++++++++++++++++++++++------------------------
> cmd.h | 1 -
> ui-commit.c | 11 ++++++++++-
> ui-diff.c | 20 +++++++++++++++++---
> ui-diff.h | 2 +-
> ui-patch.c | 8 ++++++++
> ui-shared.c | 16 ++++++++++++++++
> ui-shared.h | 3 +++
> ui-stats.c | 7 +++++++
> ui-summary.c | 5 +++++
> ui-tag.c | 16 +++++++++++++++-
> ui-tree.c | 8 ++++++++
> 13 files changed, 129 insertions(+), 59 deletions(-)
>
> diff --git a/cgit.c b/cgit.c
> index 861352a..f1ba6dd 100644
> --- a/cgit.c
> +++ b/cgit.c
> @@ -575,9 +575,7 @@ static int prepare_repo_cmd(struct cgit_context *ctx)
> ctx->page.title = fmtalloc("%s - %s", ctx->cfg.root_title,
> "config error");
> ctx->repo = NULL;
> - cgit_print_http_headers(ctx);
> - cgit_print_docstart(ctx);
> - cgit_print_pageheader(ctx);
> + cgit_print_pagestart(ctx);
> cgit_print_error("Failed to open %s: %s", name,
> rc ? strerror(rc) : "Not a valid git repository");
> cgit_print_docend();
> @@ -594,9 +592,7 @@ static int prepare_repo_cmd(struct cgit_context *ctx)
> }
>
> if (!ctx->qry.head) {
> - cgit_print_http_headers(ctx);
> - cgit_print_docstart(ctx);
> - cgit_print_pageheader(ctx);
> + cgit_print_pagestart(ctx);
> cgit_print_error("Repository seems to be empty");
> cgit_print_docend();
> return 1;
> @@ -605,11 +601,7 @@ static int prepare_repo_cmd(struct cgit_context *ctx)
> 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(ctx);
> - cgit_print_docstart(ctx);
> - cgit_print_pageheader(ctx);
> + cgit_print_pagestart_error(ctx, 404, "Not found");
> cgit_print_error("Invalid branch: %s", tmp);
> cgit_print_docend();
> return 1;
> @@ -628,11 +620,7 @@ static void process_request(void *cbdata)
> cmd = cgit_get_cmd(ctx);
> if (!cmd) {
> ctx->page.title = "cgit error";
> - ctx->page.status = 404;
> - ctx->page.statusmsg = "Not found";
> - cgit_print_http_headers(ctx);
> - cgit_print_docstart(ctx);
> - cgit_print_pageheader(ctx);
> + cgit_print_pagestart_error(ctx, 404, "Not found");
> cgit_print_error("Invalid request");
> cgit_print_docend();
> return;
> @@ -650,9 +638,7 @@ static void process_request(void *cbdata)
> ctx->qry.vpath = cmd->want_vpath ? ctx->qry.path : NULL;
>
> if (cmd->want_repo && !ctx->repo) {
> - cgit_print_http_headers(ctx);
> - cgit_print_docstart(ctx);
> - cgit_print_pageheader(ctx);
> + cgit_print_pagestart(ctx);
> cgit_print_error("No repository selected");
> cgit_print_docend();
> return;
> @@ -661,16 +647,7 @@ static void process_request(void *cbdata)
> if (ctx->repo && prepare_repo_cmd(ctx))
> return;
>
> - if (cmd->want_layout) {
> - cgit_print_http_headers(ctx);
> - cgit_print_docstart(ctx);
> - cgit_print_pageheader(ctx);
> - }
> -
> cmd->fn(ctx);
> -
> - 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 0202917..d414450 100644
> --- a/cmd.c
> +++ b/cmd.c
> @@ -39,10 +39,14 @@ static void atom_fn(struct cgit_context *ctx)
>
> static void about_fn(struct cgit_context *ctx)
> {
> + cgit_print_pagestart(ctx);
> +
> if (ctx->repo)
> cgit_print_repo_readme(ctx->qry.path);
> else
> cgit_print_site_readme();
> +
> + cgit_print_docend();
> }
>
> static void blob_fn(struct cgit_context *ctx)
> @@ -57,12 +61,12 @@ static void commit_fn(struct cgit_context *ctx)
>
> static void diff_fn(struct cgit_context *ctx)
> {
> - cgit_print_diff(ctx->qry.sha1, ctx->qry.sha2, ctx->qry.path, 1, 0);
> + cgit_print_diff(ctx->qry.sha1, ctx->qry.sha2, ctx->qry.path, 1, 0, 1);
> }
>
> static void rawdiff_fn(struct cgit_context *ctx)
> {
> - cgit_print_diff(ctx->qry.sha1, ctx->qry.sha2, ctx->qry.path, 1, 1);
> + cgit_print_diff(ctx->qry.sha1, ctx->qry.sha2, ctx->qry.path, 1, 1, 1);
> }
>
> static void info_fn(struct cgit_context *ctx)
> @@ -72,10 +76,14 @@ static void info_fn(struct cgit_context *ctx)
>
> static void log_fn(struct cgit_context *ctx)
> {
> + cgit_print_pagestart(ctx);
> +
> cgit_print_log(ctx->qry.sha1, ctx->qry.ofs, ctx->cfg.max_commit_count,
> ctx->qry.grep, ctx->qry.search, ctx->qry.path, 1,
> ctx->repo->enable_commit_graph,
> ctx->repo->commit_sort);
> +
> + cgit_print_docend();
> }
>
> static void ls_cache_fn(struct cgit_context *ctx)
> @@ -108,7 +116,9 @@ static void plain_fn(struct cgit_context *ctx)
>
> static void refs_fn(struct cgit_context *ctx)
> {
> + cgit_print_pagestart(ctx);
> cgit_print_refs();
> + cgit_print_docend();
> }
>
> static void snapshot_fn(struct cgit_context *ctx)
> @@ -137,32 +147,32 @@ static void tree_fn(struct cgit_context *ctx)
> 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(struct cgit_context *ctx)
> {
> 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(blob, 1, 0, 0, 0),
> - def_cmd(commit, 1, 1, 1, 0),
> - def_cmd(diff, 1, 1, 1, 0),
> - def_cmd(info, 1, 0, 0, 1),
> - def_cmd(log, 1, 1, 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, 1, 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(summary, 1, 1, 0, 0),
> - def_cmd(tag, 1, 1, 0, 0),
> - def_cmd(tree, 1, 1, 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 eb5bc87..8c88b32 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;
> };
> diff --git a/ui-commit.c b/ui-commit.c
> index ef85a49..d29039d 100644
> --- a/ui-commit.c
> +++ b/ui-commit.c
> @@ -27,14 +27,21 @@ void cgit_print_commit(char *hex, const char *prefix)
> hex = ctx.qry.head;
>
> if (get_sha1(hex, sha1)) {
> + cgit_print_pagestart_error(&ctx, 404, "Not found");
> cgit_print_error("Bad object id: %s", hex);
> + cgit_print_docend();
> return;
> }
> commit = lookup_commit_reference(sha1);
> if (!commit) {
> + cgit_print_pagestart_error(&ctx, 404, "Not found");
> cgit_print_error("Bad commit reference: %s", hex);
> + cgit_print_docend();
> return;
> }
> +
> + cgit_print_pagestart(&ctx);
> +
> info = cgit_parse_commit(commit);
>
> format_display_notes(sha1, ¬es, PAGE_ENCODING, 0);
> @@ -137,8 +144,10 @@ void cgit_print_commit(char *hex, const char *prefix)
> tmp = sha1_to_hex(commit->parents->item->object.sha1);
> else
> tmp = NULL;
> - cgit_print_diff(ctx.qry.sha1, tmp, prefix, 0, 0);
> + cgit_print_diff(ctx.qry.sha1, tmp, prefix, 0, 0, 0);
> }
> strbuf_release(¬es);
> cgit_free_commitinfo(info);
> +
> + cgit_print_docend();
> }
> diff --git a/ui-diff.c b/ui-diff.c
> index 1209c47..000c794 100644
> --- a/ui-diff.c
> +++ b/ui-diff.c
> @@ -358,25 +358,31 @@ void cgit_print_diff_ctrls()
> }
>
> void cgit_print_diff(const char *new_rev, const char *old_rev,
> - const char *prefix, int show_ctrls, int raw)
> + const char *prefix, int show_ctrls, int raw, int full_page)
> {
> struct commit *commit, *commit2;
>
> if (!new_rev)
> new_rev = ctx.qry.head;
> if (get_sha1(new_rev, new_rev_sha1)) {
> + cgit_print_pagestart_error(&ctx, 404, "Not found");
> cgit_print_error("Bad object name: %s", new_rev);
> - return;
> + cgit_print_docend();
> + return;
> }
> commit = lookup_commit_reference(new_rev_sha1);
> if (!commit || parse_commit(commit)) {
> + cgit_print_pagestart_error(&ctx, 404, "Not found");
> cgit_print_error("Bad commit: %s", sha1_to_hex(new_rev_sha1));
> - return;
> + cgit_print_docend();
> + return;
> }
>
> if (old_rev) {
> if (get_sha1(old_rev, old_rev_sha1)) {
> + cgit_print_pagestart_error(&ctx, 404, "Not found");
> cgit_print_error("Bad object name: %s", old_rev);
> + cgit_print_docend();
> return;
> }
> } else if (commit->parents && commit->parents->item) {
> @@ -388,7 +394,9 @@ 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_pagestart_error(&ctx, 404, "Not found");
> cgit_print_error("Bad commit: %s", sha1_to_hex(old_rev_sha1));
> + cgit_print_docend();
> return;
> }
> }
> @@ -401,6 +409,9 @@ void cgit_print_diff(const char *new_rev, const char *old_rev,
> return;
> }
>
> + if (full_page)
> + cgit_print_pagestart(&ctx);
> +
> use_ssdiff = ctx.qry.has_ssdiff ? ctx.qry.ssdiff : ctx.cfg.ssdiff;
>
> if (show_ctrls)
> @@ -419,4 +430,7 @@ void cgit_print_diff(const char *new_rev, const char *old_rev,
> if (!use_ssdiff)
> html("</td></tr>");
> html("</table>");
> +
> + if (full_page)
> + cgit_print_docend();
> }
> diff --git a/ui-diff.h b/ui-diff.h
> index 04f9029..dac7a59 100644
> --- a/ui-diff.h
> +++ b/ui-diff.h
> @@ -4,7 +4,7 @@
> extern void cgit_print_diff_ctrls();
>
> extern void cgit_print_diff(const char *new_hex, const char *old_hex,
> - const char *prefix, int show_ctrls, int raw);
> + const char *prefix, int show_ctrls, int raw, int full_page);
>
> extern struct diff_filespec *cgit_get_current_old_file(void);
> extern struct diff_filespec *cgit_get_current_new_file(void);
> diff --git a/ui-patch.c b/ui-patch.c
> index 4d35167..9aec0a3 100644
> --- a/ui-patch.c
> +++ b/ui-patch.c
> @@ -25,22 +25,30 @@ 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_pagestart_error(&ctx, 404, "Not found");
> cgit_print_error("Bad object id: %s", new_rev);
> + cgit_print_docend();
> return;
> }
> commit = lookup_commit_reference(new_rev_sha1);
> if (!commit) {
> + cgit_print_pagestart_error(&ctx, 404, "Not found");
> cgit_print_error("Bad commit reference: %s", new_rev);
> + cgit_print_docend();
> return;
> }
>
> if (old_rev) {
> if (get_sha1(old_rev, old_rev_sha1)) {
> + cgit_print_pagestart_error(&ctx, 404, "Not found");
> cgit_print_error("Bad object id: %s", old_rev);
> + cgit_print_docend();
> return;
> }
> if (!lookup_commit_reference(old_rev_sha1)) {
> + cgit_print_pagestart_error(&ctx, 404, "Not found");
> cgit_print_error("Bad commit reference: %s", old_rev);
> + cgit_print_docend();
> return;
> }
> } else if (commit->parents && commit->parents->item) {
> diff --git a/ui-shared.c b/ui-shared.c
> index 1e19421..2c0cac5 100644
> --- a/ui-shared.c
> +++ b/ui-shared.c
> @@ -696,6 +696,22 @@ void cgit_print_docstart(struct cgit_context *ctx)
> html_include(ctx->cfg.header);
> }
>
> +void cgit_print_pagestart(struct cgit_context *ctx)
> +{
> + cgit_print_http_headers(ctx);
> + cgit_print_docstart(ctx);
> + cgit_print_pageheader(ctx);
> +}
> +
> +void cgit_print_pagestart_error(struct cgit_context *ctx, int status, const char *reason)
> +{
> + ctx->page.status = status;
> + ctx->page.statusmsg = reason;
> + cgit_print_http_headers(ctx);
> + cgit_print_docstart(ctx);
> + cgit_print_pageheader(ctx);
> +}
> +
> void cgit_print_docend()
> {
> html("</div> <!-- class=content -->\n");
> diff --git a/ui-shared.h b/ui-shared.h
> index a337dce..4110f83 100644
> --- a/ui-shared.h
> +++ b/ui-shared.h
> @@ -60,6 +60,9 @@ extern void cgit_print_date(time_t secs, const char *format, int local_time);
> extern void cgit_print_age(time_t t, time_t max_relative, const char *format);
> extern void cgit_print_http_headers(struct cgit_context *ctx);
> extern void cgit_print_docstart(struct cgit_context *ctx);
> +extern void cgit_print_pagestart(struct cgit_context *ctx);
> +extern void cgit_print_pagestart_error(struct cgit_context *ctx, int status,
> + const char *reason);
> extern void cgit_print_docend();
> extern void cgit_print_pageheader(struct cgit_context *ctx);
> extern void cgit_print_filemode(unsigned short mode);
> diff --git a/ui-stats.c b/ui-stats.c
> index 28b794f..cefea9f 100644
> --- a/ui-stats.c
> +++ b/ui-stats.c
> @@ -374,9 +374,14 @@ void cgit_show_stats(struct cgit_context *ctx)
>
> i = cgit_find_stats_period(code, &period);
> if (!i) {
> + cgit_print_pagestart_error(ctx, 400, "Bad request");
> cgit_print_error("Unknown statistics type: %c", code[0]);
> + cgit_print_docend();
> return;
> }
> +
> + cgit_print_pagestart(ctx);
> +
> if (i > ctx->repo->max_stats) {
> cgit_print_error("Statistics type disabled: %s", period->name);
> return;
> @@ -423,5 +428,7 @@ void cgit_show_stats(struct cgit_context *ctx)
> }
> html("</h2>");
> print_authors(&authors, top, period);
> +
> + cgit_print_docend();
> }
>
> diff --git a/ui-summary.c b/ui-summary.c
> index 5598d08..8c15980 100644
> --- a/ui-summary.c
> +++ b/ui-summary.c
> @@ -13,6 +13,7 @@
> #include "ui-log.h"
> #include "ui-refs.h"
> #include "ui-blob.h"
> +#include "ui-shared.h"
> #include <libgen.h>
>
> static void print_url(char *base, char *suffix)
> @@ -80,6 +81,8 @@ void cgit_print_summary()
> if (ctx.repo->enable_log_linecount)
> columns++;
>
> + cgit_print_pagestart(&ctx);
> +
> html("<table summary='repository info' class='list nowrap'>");
> cgit_print_branches(ctx.cfg.summary_branches);
> htmlf("<tr class='nohover'><td colspan='%d'> </td></tr>", columns);
> @@ -94,6 +97,8 @@ void cgit_print_summary()
> else if (ctx.cfg.clone_prefix)
> print_urls(ctx.cfg.clone_prefix, ctx.repo->url);
> html("</table>");
> +
> + cgit_print_docend();
> }
>
> /* The caller must free the return value. */
> diff --git a/ui-tag.c b/ui-tag.c
> index aea7958..103f9be 100644
> --- a/ui-tag.c
> +++ b/ui-tag.c
> @@ -44,7 +44,7 @@ void cgit_print_tag(char *revname)
> struct strbuf fullref = STRBUF_INIT;
> unsigned char sha1[20];
> struct object *obj;
> - struct tag *tag;
> + struct tag *tag = NULL;
> struct taginfo *info;
>
> if (!revname)
> @@ -52,20 +52,32 @@ void cgit_print_tag(char *revname)
>
> strbuf_addf(&fullref, "refs/tags/%s", revname);
> if (get_sha1(fullref.buf, sha1)) {
> + cgit_print_pagestart_error(&ctx, 404, "Not found");
> cgit_print_error("Bad tag reference: %s", revname);
> + cgit_print_docend();
> goto cleanup;
> }
> obj = parse_object(sha1);
> if (!obj) {
> + cgit_print_pagestart_error(&ctx, 404, "Not found");
> cgit_print_error("Bad object id: %s", sha1_to_hex(sha1));
> + cgit_print_docend();
> goto cleanup;
> }
> +
> if (obj->type == OBJ_TAG) {
> tag = lookup_tag(sha1);
> if (!tag || parse_tag(tag) || !(info = cgit_parse_tag(tag))) {
> + cgit_print_pagestart_error(&ctx, 404, "Not found");
> cgit_print_error("Bad tag object: %s", revname);
> + cgit_print_docend();
> goto cleanup;
> }
> + }
> +
> + cgit_print_pagestart(&ctx);
> +
> + if (tag) {
> html("<table class='commit-info'>\n");
> htmlf("<tr><td>tag name</td><td>");
> html_txt(revname);
> @@ -104,6 +116,8 @@ void cgit_print_tag(char *revname)
> html("</table>\n");
> }
>
> + cgit_print_docend();
> +
> cleanup:
> strbuf_release(&fullref);
> }
> diff --git a/ui-tree.c b/ui-tree.c
> index aa5dee9..0074a69 100644
> --- a/ui-tree.c
> +++ b/ui-tree.c
> @@ -271,15 +271,21 @@ void cgit_print_tree(const char *rev, char *path)
> rev = ctx.qry.head;
>
> if (get_sha1(rev, sha1)) {
> + cgit_print_pagestart_error(&ctx, 404, "Not found");
> cgit_print_error("Invalid revision name: %s", rev);
> + cgit_print_docend();
> return;
> }
> commit = lookup_commit_reference(sha1);
> if (!commit || parse_commit(commit)) {
> + cgit_print_pagestart_error(&ctx, 404, "Not found");
> cgit_print_error("Invalid commit reference: %s", rev);
> + cgit_print_docend();
> return;
> }
>
> + cgit_print_pagestart(&ctx);
> +
> walk_tree_ctx.curr_rev = xstrdup(rev);
>
> if (path == NULL) {
> @@ -293,4 +299,6 @@ void cgit_print_tree(const char *rev, char *path)
>
> cleanup:
> free(walk_tree_ctx.curr_rev);
> +
> + cgit_print_docend();
> }
> --
> 1.8.3.2
>
> _______________________________________________
> CGit mailing list
> CGit at lists.zx2c4.com
> http://lists.zx2c4.com/mailman/listinfo/cgit
More information about the CGit
mailing list