{"thread":{"id":"13968","subject":"[PATCH] gitweb: return correct HTTP status codes","startedAt":"2008-06-15T21:15:15Z","lastAt":"2008-06-20T00:48:04Z","messageCount":25,"participants":["Lea Wiemann","Jakub Narebski","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"79960","messageId":"1213564515-14356-1-git-send-email-LeWiemann@gmail.com","threadId":"13968","inReplyTo":null,"subject":"[PATCH] gitweb: return correct HTTP status codes","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-15T21:15:15Z","receivedAt":"2008-06-15T21:15:15Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Many error status codes simply default to 403 Forbidden, which is not\ncorrect in most cases. This patch makes gitweb return semantically\ncorrect status codes.\n\nFor convenience the die_error function now only takes the status code\nwithout reason as first parameter (e.g. 404 instead of \"404 Not\nFound\"), and it now defaults to 500 (Internal Server Error), even\nthough the default is not used anywhere.\n\nSigned-off-by: Lea Wiemann \n---\nI recommend that you apply this patch beforehand:\n[PATCH] gitweb: remove git_blame and rename git_blame2 to git_blame\nhttp://article.gmane.org/gmane.comp.version-control.git/84036/raw\n\nIn some cases I've simply trusted the existing message (so when the\nmessage said \"not found\", I would make it a 404 error without checking\nwhether the message is accurate). Also, in some cases an overly\ngeneric 500 Internal Server Error is returned, e.g. in several cases\nlike this one ...\n\n \topen my $fd, \"-|\", git_cmd(), \"ls-tree\", $base, \"--\", $path\n-\t\tor die_error(undef, \"Open git-ls-tree failed\");\n+\t\tor die_error(500, \"Open git-ls-tree failed\");\n\n... I suspect that the error could be triggered by non-existent hashes\nas well, in which case the more specific 404 would be more appropriate\n-- however, the current implementation oftentimes doesn't allow for\nmore fine-granulared checking, and I don't want to add actual code as\nlong as we don't have tests. So we'll go with catch-all 500 errors in\nthe meantime; it's not a regression, at least.\n\nJakub: Let's make this patch a prerequisite for the Mechanize tests so\nwe can test for exact status codes rather than [45][0-9][0-9].\n\n gitweb/gitweb.perl | 196 ++++++++++++++++++++++++----------------------------\n 1 files changed, 91 insertions(+), 105 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 7b1b076..07e64da 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -386,7 +386,7 @@ $projects_list ||= $projectroot;\n our $action = $cgi->param('a');\n if (defined $action) {\n \tif ($action =~ m/[^0-9a-zA-Z\\.\\-_]/) {\n-\t\tdie_error(undef, \"Invalid action parameter\");\n+\t\tdie_error(400, \"Invalid action parameter\");\n \t}\n }\n \n@@ -399,21 +399,21 @@ if (defined $project) {\n \t ($export_ok && !(-e \"$projectroot/$project/$export_ok\")) ||\n \t ($strict_export && !project_in_list($project))) {\n \t\tundef $project;\n-\t\tdie_error(undef, \"No such project\");\n+\t\tdie_error(404, \"No such project\");\n \t}\n }\n \n our $file_name = $cgi->param('f');\n if (defined $file_name) {\n \tif (!validate_pathname($file_name)) {\n-\t\tdie_error(undef, \"Invalid file parameter\");\n+\t\tdie_error(400, \"Invalid file parameter\");\n \t}\n }\n \n our $file_parent = $cgi->param('fp');\n if (defined $file_parent) {\n \tif (!validate_pathname($file_parent)) {\n-\t\tdie_error(undef, \"Invalid file parent parameter\");\n+\t\tdie_error(400, \"Invalid file parent parameter\");\n \t}\n }\n \n@@ -421,21 +421,21 @@ if (defined $file_parent) {\n our $hash = $cgi->param('h');\n if (defined $hash) {\n \tif (!validate_refname($hash)) {\n-\t\tdie_error(undef, \"Invalid hash parameter\");\n+\t\tdie_error(400, \"Invalid hash parameter\");\n \t}\n }\n \n our $hash_parent = $cgi->param('hp');\n if (defined $hash_parent) {\n \tif (!validate_refname($hash_parent)) {\n-\t\tdie_error(undef, \"Invalid hash parent parameter\");\n+\t\tdie_error(400, \"Invalid hash parent parameter\");\n \t}\n }\n \n our $hash_base = $cgi->param('hb');\n if (defined $hash_base) {\n \tif (!validate_refname($hash_base)) {\n-\t\tdie_error(undef, \"Invalid hash base parameter\");\n+\t\tdie_error(400, \"Invalid hash base parameter\");\n \t}\n }\n \n@@ -447,10 +447,10 @@ our @extra_options = $cgi->param('opt');\n if (defined @extra_options) {\n \tforeach my $opt (@extra_options) {\n \t\tif (not exists $allowed_options{$opt}) {\n-\t\t\tdie_error(undef, \"Invalid option parameter\");\n+\t\t\tdie_error(400, \"Invalid option parameter\");\n \t\t}\n \t\tif (not grep(/^$action$/, @{$allowed_options{$opt}})) {\n-\t\t\tdie_error(undef, \"Invalid option parameter for this action\");\n+\t\t\tdie_error(400, \"Invalid option parameter for this action\");\n \t\t}\n \t}\n }\n@@ -458,7 +458,7 @@ if (defined @extra_options) {\n our $hash_parent_base = $cgi->param('hpb');\n if (defined $hash_parent_base) {\n \tif (!validate_refname($hash_parent_base)) {\n-\t\tdie_error(undef, \"Invalid hash parent base parameter\");\n+\t\tdie_error(400, \"Invalid hash parent base parameter\");\n \t}\n }\n \n@@ -466,14 +466,14 @@ if (defined $hash_parent_base) {\n our $page = $cgi->param('pg');\n if (defined $page) {\n \tif ($page =~ m/[^0-9]/) {\n-\t\tdie_error(undef, \"Invalid page parameter\");\n+\t\tdie_error(400, \"Invalid page parameter\");\n \t}\n }\n \n our $searchtype = $cgi->param('st');\n if (defined $searchtype) {\n \tif ($searchtype =~ m/[^a-z]/) {\n-\t\tdie_error(undef, \"Invalid searchtype parameter\");\n+\t\tdie_error(400, \"Invalid searchtype parameter\");\n \t}\n }\n \n@@ -483,7 +483,7 @@ our $searchtext = $cgi->param('s');\n our $search_regexp;\n if (defined $searchtext) {\n \tif (length($searchtext) < 2) {\n-\t\tdie_error(undef, \"At least two characters are required for search parameter\");\n+\t\tdie_error(403, \"At least two characters are required for search parameter\");\n \t}\n \t$search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;\n }\n@@ -580,11 +580,11 @@ if (!defined $action) {\n \t}\n }\n if (!defined($actions{$action})) {\n-\tdie_error(undef, \"Unknown action\");\n+\tdie_error(400, \"Unknown action\");\n }\n if ($action !~ m/^(opml|project_list|project_index)$/ &&\n !$project) {\n-\tdie_error(undef, \"Project needed\");\n+\tdie_error(400, \"Project needed\");\n }\n $actions{$action}->();\n exit;\n@@ -1661,7 +1661,7 @@ sub git_get_hash_by_path {\n \t$path =~ s,/+$,,;\n \n \topen my $fd, \"-|\", git_cmd(), \"ls-tree\", $base, \"--\", $path\n-\t\tor die_error(undef, \"Open git-ls-tree failed\");\n+\t\tor die_error(500, \"Open git-ls-tree failed\");\n \tmy $line = <$fd>;\n \tclose $fd or return undef;\n \n@@ -2123,7 +2123,7 @@ sub parse_commit {\n \t\t\"--max-count=1\",\n \t\t$commit_id,\n \t\t\"--\",\n-\t\tor die_error(undef, \"Open git-rev-list failed\");\n+\t\tor die_error(500, \"Open git-rev-list failed\");\n \t%co = parse_commit_text(<$fd>, 1);\n \tclose $fd;\n \n@@ -2148,7 +2148,7 @@ sub parse_commits {\n \t\t$commit_id,\n \t\t\"--\",\n \t\t($filename ? ($filename) : ())\n-\t\tor die_error(undef, \"Open git-rev-list failed\");\n+\t\tor die_error(500, \"Open git-rev-list failed\");\n \twhile (my $line = <$fd>) {\n \t\tmy %co = parse_commit_text($line);\n \t\tpush @cos, \\%co;\n@@ -2669,10 +2669,12 @@ sub git_footer_html {\n }\n \n sub die_error {\n-\tmy $status = shift || \"403 Forbidden\";\n-\tmy $error = shift || \"Malformed query, file missing or permission denied\";\n+\tmy $status = shift || 500;\n+\tmy $error = shift || \"Internal server error\";\n \n-\tgit_header_html($status);\n+\t# Use a generic \"Error\" reason, e.g. \"404 Error\" instead of\n+\t# \"404 Not Found\". This is permissible by RFC 2616.\n+\tgit_header_html(\"$status Error\");\n \tprint <\n

\n@@ -3920,12 +3922,12 @@ sub git_search_grep_body {\n sub git_project_list {\n \tmy $order = $cgi->param('o');\n \tif (defined $order && $order !~ m/none|project|descr|owner|age/) {\n-\t\tdie_error(undef, \"Unknown order parameter\");\n+\t\tdie_error(400, \"Unknown order parameter\");\n \t}\n \n \tmy @list = git_get_projects_list();\n \tif (!@list) {\n-\t\tdie_error(undef, \"No projects found\");\n+\t\tdie_error(404, \"No projects found\");\n \t}\n \n \tgit_header_html();\n@@ -3943,12 +3945,12 @@ sub git_project_list {\n sub git_forks {\n \tmy $order = $cgi->param('o');\n \tif (defined $order && $order !~ m/none|project|descr|owner|age/) {\n-\t\tdie_error(undef, \"Unknown order parameter\");\n+\t\tdie_error(400, \"Unknown order parameter\");\n \t}\n \n \tmy @list = git_get_projects_list($project);\n \tif (!@list) {\n-\t\tdie_error(undef, \"No forks found\");\n+\t\tdie_error(404, \"No forks found\");\n \t}\n \n \tgit_header_html();\n@@ -4077,7 +4079,7 @@ sub git_tag {\n \tmy %tag = parse_tag($hash);\n \n \tif (! %tag) {\n-\t\tdie_error(undef, \"Unknown tag object\");\n+\t\tdie_error(404, \"Unknown tag object\");\n \t}\n \n \tgit_print_header_div('commit', esc_html($tag{'name'}), $hash);\n@@ -4113,26 +4115,25 @@ sub git_blame {\n \tmy $fd;\n \tmy $ftype;\n \n-\tmy ($have_blame) = gitweb_check_feature('blame');\n-\tif (!$have_blame) {\n-\t\tdie_error('403 Permission denied', \"Permission denied\");\n-\t}\n-\tdie_error('404 Not Found', \"File name not defined\") if (!$file_name);\n+\tgitweb_check_feature('blame')\n+\t or die_error(403, \"Blame not allowed\");\n+\n+\tdie_error(400, \"No file name given\") unless $file_name;\n \t$hash_base ||= git_get_head_hash($project);\n-\tdie_error(undef, \"Couldn't find base commit\") unless ($hash_base);\n+\tdie_error(400, \"Couldn't find base commit\") unless ($hash_base);\n \tmy %co = parse_commit($hash_base)\n-\t\tor die_error(undef, \"Reading commit failed\");\n+\t\tor die_error(404, \"Commit not found\");\n \tif (!defined $hash) {\n \t\t$hash = git_get_hash_by_path($hash_base, $file_name, \"blob\")\n-\t\t\tor die_error(undef, \"Error looking up file\");\n+\t\t\tor die_error(404, \"Error looking up file\");\n \t}\n \t$ftype = git_get_type($hash);\n \tif ($ftype !~ \"blob\") {\n-\t\tdie_error('400 Bad Request', \"Object is not a blob\");\n+\t\tdie_error(400, \"Object is not a blob\");\n \t}\n \topen ($fd, \"-|\", git_cmd(), \"blame\", '-p', '--',\n \t $file_name, $hash_base)\n-\t\tor die_error(undef, \"Open git-blame failed\");\n+\t\tor die_error(500, \"Open git-blame failed\");\n \tgit_header_html();\n \tmy $formats_nav =\n \t\t$cgi->a({-href => href(action=>\"blob\", -replay=>1)},\n@@ -4194,7 +4195,7 @@ HTML\n \t\t\tprint \"\\n\";\n \t\t}\n \t\topen (my $dd, \"-|\", git_cmd(), \"rev-parse\", \"$full_rev^\")\n-\t\t\tor die_error(undef, \"Open git-rev-parse failed\");\n+\t\t\tor die_error(500, \"Open git-rev-parse failed\");\n \t\tmy $parent_commit = <$dd>;\n \t\tclose $dd;\n \t\tchomp($parent_commit);\n@@ -4251,9 +4252,9 @@ sub git_blob_plain {\n \t\tif (defined $file_name) {\n \t\t\tmy $base = $hash_base || git_get_head_hash($project);\n \t\t\t$hash = git_get_hash_by_path($base, $file_name, \"blob\")\n-\t\t\t\tor die_error(undef, \"Error lookup file\");\n+\t\t\t\tor die_error(404, \"Cannot find file\");\n \t\t} else {\n-\t\t\tdie_error(undef, \"No file name defined\");\n+\t\t\tdie_error(400, \"No file name defined\");\n \t\t}\n \t} elsif ($hash =~ m/^[0-9a-fA-F]{40}$/) {\n \t\t# blobs defined by non-textual hash id's can be cached\n@@ -4261,7 +4262,7 @@ sub git_blob_plain {\n \t}\n \n \topen my $fd, \"-|\", git_cmd(), \"cat-file\", \"blob\", $hash\n-\t\tor die_error(undef, \"Open git-cat-file blob '$hash' failed\");\n+\t\tor die_error(500, \"Open git-cat-file blob '$hash' failed\");\n \n \t# content-type (can include charset)\n \t$type = blob_contenttype($fd, $file_name, $type);\n@@ -4293,9 +4294,9 @@ sub git_blob {\n \t\tif (defined $file_name) {\n \t\t\tmy $base = $hash_base || git_get_head_hash($project);\n \t\t\t$hash = git_get_hash_by_path($base, $file_name, \"blob\")\n-\t\t\t\tor die_error(undef, \"Error lookup file\");\n+\t\t\t\tor die_error(404, \"Cannot find file\");\n \t\t} else {\n-\t\t\tdie_error(undef, \"No file name defined\");\n+\t\t\tdie_error(400, \"No file name defined\");\n \t\t}\n \t} elsif ($hash =~ m/^[0-9a-fA-F]{40}$/) {\n \t\t# blobs defined by non-textual hash id's can be cached\n@@ -4304,7 +4305,7 @@ sub git_blob {\n \n \tmy ($have_blame) = gitweb_check_feature('blame');\n \topen my $fd, \"-|\", git_cmd(), \"cat-file\", \"blob\", $hash\n-\t\tor die_error(undef, \"Couldn't cat $file_name, $hash\");\n+\t\tor die_error(500, \"Couldn't cat $file_name, $hash\");\n \tmy $mimetype = blob_mimetype($fd, $file_name);\n \tif ($mimetype !~ m!^(?:text/|image/(?:gif|png|jpeg)$)! && -B $fd) {\n \t\tclose $fd;\n@@ -4385,9 +4386,9 @@ sub git_tree {\n \t}\n \t$/ = \"\\0\";\n \topen my $fd, \"-|\", git_cmd(), \"ls-tree\", '-z', $hash\n-\t\tor die_error(undef, \"Open git-ls-tree failed\");\n+\t\tor die_error(500, \"Open git-ls-tree failed\");\n \tmy @entries = map { chomp; $_ } <$fd>;\n-\tclose $fd or die_error(undef, \"Reading tree failed\");\n+\tclose $fd or die_error(500, \"Reading tree failed\");\n \t$/ = \"\\n\";\n \n \tmy $refs = git_get_references();\n@@ -4477,16 +4478,16 @@ sub git_snapshot {\n \n \tmy $format = $cgi->param('sf');\n \tif (!@supported_fmts) {\n-\t\tdie_error('403 Permission denied', \"Permission denied\");\n+\t\tdie_error(403, \"Snapshots not allowed\");\n \t}\n \t# default to first supported snapshot format\n \t$format ||= $supported_fmts[0];\n \tif ($format !~ m/^[a-z0-9]+$/) {\n-\t\tdie_error(undef, \"Invalid snapshot format parameter\");\n+\t\tdie_error(400, \"Invalid snapshot format parameter\");\n \t} elsif (!exists($known_snapshot_formats{$format})) {\n-\t\tdie_error(undef, \"Unknown snapshot format\");\n+\t\tdie_error(400, \"Unknown snapshot format\");\n \t} elsif (!grep($_ eq $format, @supported_fmts)) {\n-\t\tdie_error(undef, \"Unsupported snapshot format\");\n+\t\tdie_error(403, \"Unsupported snapshot format\");\n \t}\n \n \tif (!defined $hash) {\n@@ -4514,7 +4515,7 @@ sub git_snapshot {\n \t\t-status => '200 OK');\n \n \topen my $fd, \"-|\", $cmd\n-\t\tor die_error(undef, \"Execute git-archive failed\");\n+\t\tor die_error(500, \"Execute git-archive failed\");\n \tbinmode STDOUT, ':raw';\n \tprint <$fd>;\n \tbinmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi\n@@ -4582,10 +4583,8 @@ sub git_log {\n \n sub git_commit {\n \t$hash ||= $hash_base || \"HEAD\";\n-\tmy %co = parse_commit($hash);\n-\tif (!%co) {\n-\t\tdie_error(undef, \"Unknown commit object\");\n-\t}\n+\tmy %co = parse_commit($hash)\n+\t or die_error(404, \"Unknown commit object\");\n \tmy %ad = parse_date($co{'author_epoch'}, $co{'author_tz'});\n \tmy %cd = parse_date($co{'committer_epoch'}, $co{'committer_tz'});\n \n@@ -4625,9 +4624,9 @@ sub git_commit {\n \t\t@diff_opts,\n \t\t(@$parents <= 1 ? $parent : '-c'),\n \t\t$hash, \"--\"\n-\t\tor die_error(undef, \"Open git-diff-tree failed\");\n+\t\tor die_error(500, \"Open git-diff-tree failed\");\n \t@difftree = map { chomp; $_ } <$fd>;\n-\tclose $fd or die_error(undef, \"Reading git-diff-tree failed\");\n+\tclose $fd or die_error(500, \"Reading git-diff-tree failed\");\n \n \t# non-textual hash id's can be cached\n \tmy $expires;\n@@ -4720,33 +4719,33 @@ sub git_object {\n \n \t\tmy $git_command = git_cmd_str();\n \t\topen my $fd, \"-|\", \"$git_command cat-file -t $object_id 2>/dev/null\"\n-\t\t\tor die_error('404 Not Found', \"Object does not exist\");\n+\t\t\tor die_error(404, \"Object does not exist\");\n \t\t$type = <$fd>;\n \t\tchomp $type;\n \t\tclose $fd\n-\t\t\tor die_error('404 Not Found', \"Object does not exist\");\n+\t\t\tor die_error(404, \"Object does not exist\");\n \n \t# - hash_base and file_name\n \t} elsif ($hash_base && defined $file_name) {\n \t\t$file_name =~ s,/+$,,;\n \n \t\tsystem(git_cmd(), \"cat-file\", '-e', $hash_base) == 0\n-\t\t\tor die_error('404 Not Found', \"Base object does not exist\");\n+\t\t\tor die_error(404, \"Base object does not exist\");\n \n \t\t# here errors should not hapen\n \t\topen my $fd, \"-|\", git_cmd(), \"ls-tree\", $hash_base, \"--\", $file_name\n-\t\t\tor die_error(undef, \"Open git-ls-tree failed\");\n+\t\t\tor die_error(500, \"Open git-ls-tree failed\");\n \t\tmy $line = <$fd>;\n \t\tclose $fd;\n \n \t\t#'100644 blob 0fa3f3a66fb6a137f6ec2c19351ed4d807070ffa\tpanic.c'\n \t\tunless ($line && $line =~ m/^([0-9]+) (.+) ([0-9a-fA-F]{40})\\t/) {\n-\t\t\tdie_error('404 Not Found', \"File or directory for given base does not exist\");\n+\t\t\tdie_error(404, \"File or directory for given base does not exist\");\n \t\t}\n \t\t$type = $2;\n \t\t$hash = $3;\n \t} else {\n-\t\tdie_error('404 Not Found', \"Not enough information to find object\");\n+\t\tdie_error(400, \"Not enough information to find object\");\n \t}\n \n \tprint $cgi->redirect(-uri => href(action=>$type, -full=>1,\n@@ -4771,12 +4770,12 @@ sub git_blobdiff {\n \t\t\topen $fd, \"-|\", git_cmd(), \"diff-tree\", '-r', @diff_opts,\n \t\t\t\t$hash_parent_base, $hash_base,\n \t\t\t\t\"--\", (defined $file_parent ? $file_parent : ()), $file_name\n-\t\t\t\tor die_error(undef, \"Open git-diff-tree failed\");\n+\t\t\t\tor die_error(500, \"Open git-diff-tree failed\");\n \t\t\t@difftree = map { chomp; $_ } <$fd>;\n \t\t\tclose $fd\n-\t\t\t\tor die_error(undef, \"Reading git-diff-tree failed\");\n+\t\t\t\tor die_error(500, \"Reading git-diff-tree failed\");\n \t\t\t@difftree\n-\t\t\t\tor die_error('404 Not Found', \"Blob diff not found\");\n+\t\t\t\tor die_error(404, \"Blob diff not found\");\n \n \t\t} elsif (defined $hash &&\n \t\t $hash =~ /[0-9a-fA-F]{40}/) {\n@@ -4785,23 +4784,23 @@ sub git_blobdiff {\n \t\t\t# read filtered raw output\n \t\t\topen $fd, \"-|\", git_cmd(), \"diff-tree\", '-r', @diff_opts,\n \t\t\t\t$hash_parent_base, $hash_base, \"--\"\n-\t\t\t\tor die_error(undef, \"Open git-diff-tree failed\");\n+\t\t\t\tor die_error(500, \"Open git-diff-tree failed\");\n \t\t\t@difftree =\n \t\t\t\t# ':100644 100644 03b21826... 3b93d5e7... M\tls-files.c'\n \t\t\t\t# $hash == to_id\n \t\t\t\tgrep { /^:[0-7]{6} [0-7]{6} [0-9a-fA-F]{40} $hash/ }\n \t\t\t\tmap { chomp; $_ } <$fd>;\n \t\t\tclose $fd\n-\t\t\t\tor die_error(undef, \"Reading git-diff-tree failed\");\n+\t\t\t\tor die_error(500, \"Reading git-diff-tree failed\");\n \t\t\t@difftree\n-\t\t\t\tor die_error('404 Not Found', \"Blob diff not found\");\n+\t\t\t\tor die_error(404, \"Blob diff not found\");\n \n \t\t} else {\n-\t\t\tdie_error('404 Not Found', \"Missing one of the blob diff parameters\");\n+\t\t\tdie_error(400, \"Missing one of the blob diff parameters\");\n \t\t}\n \n \t\tif (@difftree > 1) {\n-\t\t\tdie_error('404 Not Found', \"Ambiguous blob diff specification\");\n+\t\t\tdie_error(400, \"Ambiguous blob diff specification\");\n \t\t}\n \n \t\t%diffinfo = parse_difftree_raw_line($difftree[0]);\n@@ -4822,7 +4821,7 @@ sub git_blobdiff {\n \t\t\t'-p', ($format eq 'html' ? \"--full-index\" : ()),\n \t\t\t$hash_parent_base, $hash_base,\n \t\t\t\"--\", (defined $file_parent ? $file_parent : ()), $file_name\n-\t\t\tor die_error(undef, \"Open git-diff-tree failed\");\n+\t\t\tor die_error(500, \"Open git-diff-tree failed\");\n \t}\n \n \t# old/legacy style URI\n@@ -4858,9 +4857,9 @@ sub git_blobdiff {\n \t\topen $fd, \"-|\", git_cmd(), \"diff\", @diff_opts,\n \t\t\t'-p', ($format eq 'html' ? \"--full-index\" : ()),\n \t\t\t$hash_parent, $hash, \"--\"\n-\t\t\tor die_error(undef, \"Open git-diff failed\");\n+\t\t\tor die_error(500, \"Open git-diff failed\");\n \t} else {\n-\t\tdie_error('404 Not Found', \"Missing one of the blob diff parameters\")\n+\t\tdie_error(400, \"Missing one of the blob diff parameters\")\n \t\t\tunless %diffinfo;\n \t}\n \n@@ -4893,7 +4892,7 @@ sub git_blobdiff {\n \t\tprint \"X-Git-Url: \" . $cgi->self_url() . \"\\n\\n\";\n \n \t} else {\n-\t\tdie_error(undef, \"Unknown blobdiff format\");\n+\t\tdie_error(400, \"Unknown blobdiff format\");\n \t}\n \n \t# patch\n@@ -4928,10 +4927,8 @@ sub git_blobdiff_plain {\n sub git_commitdiff {\n \tmy $format = shift || 'html';\n \t$hash ||= $hash_base || \"HEAD\";\n-\tmy %co = parse_commit($hash);\n-\tif (!%co) {\n-\t\tdie_error(undef, \"Unknown commit object\");\n-\t}\n+\tmy %co = parse_commit($hash)\n+\t or die_error(404, \"Unknown commit object\");\n \n \t# choose format for commitdiff for merge\n \tif (! defined $hash_parent && @{$co{'parents'}} > 1) {\n@@ -5013,7 +5010,7 @@ sub git_commitdiff {\n \t\topen $fd, \"-|\", git_cmd(), \"diff-tree\", '-r', @diff_opts,\n \t\t\t\"--no-commit-id\", \"--patch-with-raw\", \"--full-index\",\n \t\t\t$hash_parent_param, $hash, \"--\"\n-\t\t\tor die_error(undef, \"Open git-diff-tree failed\");\n+\t\t\tor die_error(500, \"Open git-diff-tree failed\");\n \n \t\twhile (my $line = <$fd>) {\n \t\t\tchomp $line;\n@@ -5025,10 +5022,10 @@ sub git_commitdiff {\n \t} elsif ($format eq 'plain') {\n \t\topen $fd, \"-|\", git_cmd(), \"diff-tree\", '-r', @diff_opts,\n \t\t\t'-p', $hash_parent_param, $hash, \"--\"\n-\t\t\tor die_error(undef, \"Open git-diff-tree failed\");\n+\t\t\tor die_error(500, \"Open git-diff-tree failed\");\n \n \t} else {\n-\t\tdie_error(undef, \"Unknown commitdiff format\");\n+\t\tdie_error(400, \"Unknown commitdiff format\");\n \t}\n \n \t# non-textual hash id's can be cached\n@@ -5111,19 +5108,15 @@ sub git_history {\n \t\t$page = 0;\n \t}\n \tmy $ftype;\n-\tmy %co = parse_commit($hash_base);\n-\tif (!%co) {\n-\t\tdie_error(undef, \"Unknown commit object\");\n-\t}\n+\tmy %co = parse_commit($hash_base)\n+\t or die_error(404, \"Unknown commit object\");\n \n \tmy $refs = git_get_references();\n \tmy $limit = sprintf(\"--max-count=%i\", (100 * ($page+1)));\n \n \tmy @commitlist = parse_commits($hash_base, 101, (100 * $page),\n-\t $file_name, \"--full-history\");\n-\tif (!@commitlist) {\n-\t\tdie_error('404 Not Found', \"No such file or directory on given branch\");\n-\t}\n+\t $file_name, \"--full-history\")\n+\t or die_error(404, \"No such file or directory on given branch\");\n \n \tif (!defined $hash && defined $file_name) {\n \t\t# some commits could have deleted file in question,\n@@ -5137,7 +5130,7 @@ sub git_history {\n \t\t$ftype = git_get_type($hash);\n \t}\n \tif (!defined $ftype) {\n-\t\tdie_error(undef, \"Unknown type of object\");\n+\t\tdie_error(500, \"Unknown type of object\");\n \t}\n \n \tmy $paging_nav = '';\n@@ -5175,19 +5168,16 @@ sub git_history {\n }\n \n sub git_search {\n-\tmy ($have_search) = gitweb_check_feature('search');\n-\tif (!$have_search) {\n-\t\tdie_error('403 Permission denied', \"Permission denied\");\n-\t}\n+\tgitweb_check_feature('search') or die_error(403, \"Search is disabled\");\n \tif (!defined $searchtext) {\n-\t\tdie_error(undef, \"Text field empty\");\n+\t\tdie_error(400, \"Text field is empty\");\n \t}\n \tif (!defined $hash) {\n \t\t$hash = git_get_head_hash($project);\n \t}\n \tmy %co = parse_commit($hash);\n \tif (!%co) {\n-\t\tdie_error(undef, \"Unknown commit object\");\n+\t\tdie_error(404, \"Unknown commit object\");\n \t}\n \tif (!defined $page) {\n \t\t$page = 0;\n@@ -5197,16 +5187,12 @@ sub git_search {\n \tif ($searchtype eq 'pickaxe') {\n \t\t# pickaxe may take all resources of your box and run for several minutes\n \t\t# with every query - so decide by yourself how public you make this feature\n-\t\tmy ($have_pickaxe) = gitweb_check_feature('pickaxe');\n-\t\tif (!$have_pickaxe) {\n-\t\t\tdie_error('403 Permission denied', \"Permission denied\");\n-\t\t}\n+\t\tgitweb_check_feature('pickaxe')\n+\t\t or die_error(403, \"Pickaxe is disabled\");\n \t}\n \tif ($searchtype eq 'grep') {\n-\t\tmy ($have_grep) = gitweb_check_feature('grep');\n-\t\tif (!$have_grep) {\n-\t\t\tdie_error('403 Permission denied', \"Permission denied\");\n-\t\t}\n+\t\tgitweb_check_feature('grep')\n+\t\t or die_error(403, \"Grep is disabled\");\n \t}\n \n \tgit_header_html();\n@@ -5480,7 +5466,7 @@ sub git_feed {\n \t# Atom: http://www.atomenabled.org/developers/syndication/\n \t# RSS: http://www.notestips.com/80256B3A007F2692/1/NAMO5P9UPQ\n \tif ($format ne 'rss' && $format ne 'atom') {\n-\t\tdie_error(undef, \"Unknown web feed format\");\n+\t\tdie_error(400, \"Unknown web feed format\");\n \t}\n \n \t# log/feed of current (HEAD) branch, log of given branch, history of file/directory\n-- \n1.5.6.rc3.7.ged9620\n"},{"id":"79989","messageId":"m37icqpb5f.fsf@localhost.localdomain","threadId":"13968","inReplyTo":"1213564515-14356-1-git-send-email-LeWiemann@gmail.com","subject":"Re: [PATCH] gitweb: return correct HTTP status codes","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-15T22:48:46Z","receivedAt":"2008-06-15T22:48:46Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Lea Wiemann writes:\n\nI would perhaps choose \"gitweb: standarize HTTP status codes\"\nas the subject, but the one you chosen is also good.\n\n> Many error status codes simply default to 403 Forbidden, which is not\n> correct in most cases. This patch makes gitweb return semantically\n> correct status codes.\n\nI'm not sure if \"403 Forbidden\" is not correct error code in the\nabsence of further information.\n\nI'd like to have reasoning when, and why, we use which responses (HTTP\nstatus codes) in which situations. Link to appropriate RFC you used\n(and perhaps to some other information) wouldn't be out of the\nquestion. See also below.\n\n> For convenience the die_error function now only takes the status code\n> without reason as first parameter (e.g. 404 instead of \"404 Not\n> Found\"), and it now defaults to 500 (Internal Server Error), even\n> though the default is not used anywhere.\n\nWell, gitweb sometimes used different wording (different explanation)\nfor the same error message / HTTP status code, for example it is\ngeneric \"403 Forbidden\", but when trying to access page which is not\navailable due to some restrictions (like feature being disabled) it is\n\"403 Permission Denied\".\n\n> Signed-off-by: Lea Wiemann \n> ---\n> I recommend that you apply this patch beforehand:\n> [PATCH] gitweb: remove git_blame and rename git_blame2 to git_blame\n> http://article.gmane.org/gmane.comp.version-control.git/84036/raw\n\nFirst, why? \n\nSecond, I think it is not possible, as IIRC removal of git-annotate\nbased git_blame got already applied.\n\n> In some cases I've simply trusted the existing message (so when the\n> message said \"not found\", I would make it a 404 error without checking\n> whether the message is accurate). Also, in some cases an overly\n> generic 500 Internal Server Error is returned, e.g. in several cases\n> like this one ...\n> \n> \topen my $fd, \"-|\", git_cmd(), \"ls-tree\", $base, \"--\", $path\n> -\t\tor die_error(undef, \"Open git-ls-tree failed\");\n> +\t\tor die_error(500, \"Open git-ls-tree failed\");\n\nShould we really use \"500 Internal Server Error\" here? Usually this\nwould be not an error at all, but wrong parameters to git command,\ni.e. it is _user_ who erred, not some error on _server_ side. I would\nuse \"500 Internal Server Error\" if for example git binary could not be\nfound...\n\n> ... I suspect that the error could be triggered by non-existent hashes\n> as well, in which case the more specific 404 would be more appropriate\n> -- however, the current implementation oftentimes doesn't allow for\n> more fine-granulared checking, and I don't want to add actual code as\n> long as we don't have tests. So we'll go with catch-all 500 errors in\n> the meantime; it's not a regression, at least.\n\n...that is why I'd rather have \"403 Forbidden\" as catch-all error, as\nit was done in gitweb. But that also seems not very good idea.\n\nIt seems like this is a matter of taste.\n\n> Jakub: Let's make this patch a prerequisite for the Mechanize tests so\n> we can test for exact status codes rather than [45][0-9][0-9].\n\nCould you please Cc me if you address me in email? I am not\nsubscribed to git mailing list, I read it via GMane NNTP (Usenet news)\ninterface; and even if I had been subscribed it is better to Cc people\nwho might be interested (in the case of gitweb caching it would be\nprobably me, Petr Baudis, John Hawley, perhaps Luben Tuikov).\n\n[...]\n> sub die_error {\n> -\tmy $status = shift || \"403 Forbidden\";\n> -\tmy $error = shift || \"Malformed query, file missing or permission denied\";\n> +\tmy $status = shift || 500;\n> +\tmy $error = shift || \"Internal server error\";\n\nErrr, I think \"Malformed query, file missing or permission denied\" is\nactually closer to truth, and better error message (it is displayed in\nbody of message)\n \n> -\tgit_header_html($status);\n> +\t# Use a generic \"Error\" reason, e.g. \"404 Error\" instead of\n> +\t# \"404 Not Found\". This is permissible by RFC 2616.\n> +\tgit_header_html(\"$status Error\");\n> \tprint < [Subject:] I would perhaps choose \"gitweb: standarize HTTP status codes\"\n\nWill fix if there's a follow-up patch.\n\n> I'd like to have reasoning when, and why, we use which responses (HTTP\n> status codes) in which situations. Link to appropriate RFC you used\n> (and perhaps to some other information)\n\nRFC 2616 has clear descriptions of the codes we use. Maybe a really\nshort list of conventions though, like you listed below? If we have\nagreement I'll happily put it in.\n\n>> For convenience the die_error function now only takes the status code\n>> without reason as first parameter\n> \n> Well, gitweb sometimes used different wording\n\nThe 'reason' only appeared in the HTTP response header -- see below.\n\n>> I recommend that you apply [PATCH] gitweb: remove git_blame\n> \n> I think it is not possible, as IIRC [it] got already applied.\n\nIt isn't applied on master (at the time I'm writing this), only on next.\nShould I be working on the next branch?\n\n>> \topen my $fd, \"-|\", git_cmd(), \"ls-tree\", $base, \"--\", $path\n>> -\t\tor die_error(undef, \"Open git-ls-tree failed\");\n>> +\t\tor die_error(500, \"Open git-ls-tree failed\");\n> \n> Should we really use \"500 Internal Server Error\" here? Usually this\n> would be not an error at all, but wrong parameters to git command,\n> i.e. it is _user_ who erred, not some error on _server_ side.\n\nYou cannot tell for sure -- all you know here is that the command\nsomehow failed when it shouldn't have, and so all you can give is 500;\nsee below. I don't think we should apply reasoning like \"most commonly\nit's a wrong hash, so let's return 404\" -- we don't know, and we\nshouldn't assume.\n\n>> ... I suspect that the error could be triggered by non-existent hashes\n>> as well, in which case the more specific 404 would be more appropriate\n>> -- however, the current implementation oftentimes doesn't allow for\n>> more fine-granulared checking\n> \n> ...that is why I'd rather have \"403 Forbidden\" as catch-all error, as\n> it was done in gitweb. But that also seems not very good idea.\n\n[IANA HTTP expert, but:] All we have in cases like the one above is \"I\ncannot deliver the content you requested, but I'm so confused that I\ndon't even know why\" -- which sounds like 500 to me (definitely not\n403). 403 would be used for some deactivated feature (commonly for\ndisabled directory listings I think) or something that shouldn't be\naccessible to the public.\n\n>> sub die_error {\n>> -\tmy $status = shift || \"403 Forbidden\";\n>> -\tmy $error = shift || \"Malformed query, file missing or permission denied\";\n>> +\tmy $status = shift || 500;\n>> +\tmy $error = shift || \"Internal server error\";\n> \n> Errr, I think \"Malformed query, file missing or permission denied\" is\n> actually closer to truth, and better error message\n\nThis default is intended for expressions like \"some_action(...) or\ndie_error\" (without parameters), in which case you really can't tell\nwhat triggered the error, so \"Internal server error\" is as specific as\nyou can get.\n\n> (it is displayed in body of message)\n\nJust to make sure we're on the same boat, the $error variable above *is*\nwhat's displayed in the body.\n\n>> +\t# Use a generic \"Error\" reason, e.g. \"404 Error\" instead of\n>> +\t# \"404 Not Found\". This is permissible by RFC 2616.\n>> +\tgit_header_html(\"$status Error\");\n> \n> I don't think it is _recommended_ practice. IIRC it is recommnded\n> [...] to give more detailed explanation in the body of message.\n\nThe code above prints the HTTP response headers, not the body -- the\nbody contains the detailed error message passed to the die_error\nfunction. Here's a stripped down version of gitweb's error response \n(X'ed out against spam filter):\n\nHTXTP/1.0 404 Error <------------ \"Error\" here\nCoXntent-Type: text/htXml\n\n... 404 - Hash not found ...\n\nI don't know of *any* browser or tool that actually displays the reason\ngiven in the HTTP response header, so defaulting to Error is fine here\n(especially since RFC 261 explicitly allow it).\n\n(I don't want to put the $error passed to die_error into the HTTP\nresponse header, since badly interpolated strings might invalidate the\nresponse. So using \"Error\" makes sure we always die gracefully.)\n\n> Proposed HTTP server/client error status codes [heavily snipped]:\n> \n> 1. Basic CGI parameter validation; 400 or perhaps 404 as catch-all\n\nYes, 400 is appropriate here; I don't think there are any catch-all\ncases to catch. :)\n\n> 2. Gitweb cannot find requested resource: 404 Not Found\n> 3. Some required parameters are missing: 400 Bad Request\n> 5. When some feature is requested that is not enabled, but with\n> different gitweb configuration would be available: 403\n\nYup, that's what I used.\n\n> 4. Project list is requested, but there are no projects (or no forks)\n> to be displayed by gitweb: not sure [...]. I don't think that \n> \"404 Not Found\" would be good solution...\n\nI see three possibilities:\n\n404: \"I couldn't find any projects/forks.\"\n500: \"The admin didn't set this up properly\" (doesn't apply to forks)\n200: \"Here's the list your requested (it's empty though).\"\n\nSince 500 doesn't work for forks, and since 200 requires additional code\nin order to not display an error page (which should be done in a\nseparate patch, with tests), 404 seemed like a reasonable default to me. \nIt's better than the previous 403, so it's not a regression.\n\n> 6. Request is made for a wrong type of object, for example 'blame' on\n> object which is not a blob: 400 (if not 404).\n\nYup. I wasn't sure, but I used 400 since it seemed a little more\nappropriate (\"you asked me for an invalid action on this object\").\n\n> 7. When the git command failed, and we don't know the reason...\n\n... 500 (see above). :)\n\n\n > Could you please Cc me if you address me in email?\n\nWill do.\n\n > probably me, Petr Baudis, John Hawley, perhaps Luben Tuikov\n\nI wouldn't want to Cc people if I don't address them personally -- e.g.\nneither Petr nor John are currently working on gitweb, so flooding their\nmailboxes might seem a little rude; if they're interested they can\nalways filter for subjects. (Unless someone requests to always be CC'ed\nof course.)\n\n-- Lea\n"},{"id":"80049","messageId":"200806161843.09372.jnareb@gmail.com","threadId":"13968","inReplyTo":"48568D5C.5090909@gmail.com","subject":"Re: [PATCH] gitweb: return correct HTTP status codes","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-16T16:43:08Z","receivedAt":"2008-06-16T16:43:08Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"[Fast reply, I will reply more in depth later]\n\nLea Wiemann wrote:\n> Jakub Narebski wrote:\n>> Lea Wiemann wrote:\n\n>>> \topen my $fd, \"-|\", git_cmd(), \"ls-tree\", $base, \"--\", $path\n>>> -\t\tor die_error(undef, \"Open git-ls-tree failed\");\n>>> +\t\tor die_error(500, \"Open git-ls-tree failed\");\n>> \n>> Should we really use \"500 Internal Server Error\" here? Usually this\n>> would be not an error at all, but wrong parameters to git command,\n>> i.e. it is _user_ who erred, not some error on _server_ side.\n> \n> You cannot tell for sure -- all you know here is that the command\n> somehow failed when it shouldn't have, and so all you can give is 500;\n> see below. I don't think we should apply reasoning like \"most commonly\n> it's a wrong hash, so let's return 404\" -- we don't know, and we\n> shouldn't assume.\n\nWell, we could, perhaps, examine stderr (or redirect it to stdout and\nexamine upon error) to check what was the error. Or when/if gitweb\nstart to use Git.pm methods, examine catched Error (for example\n\"fatal: bad revision '$hash'\" would mean \"404 Not Found\" revision).\n\nBut I think in all, or almost all cases, the source is wrong parameters\nin URL. Now, returning 5xx _server_ error would make me want to email\nwebmaster about error on his/her server, while 4xx _user_ error would\nmake me examine my input, i.e. URL I have entered, or handcrafted, or\nmagled. That is IMVHO *very* important difference, and why I am\nagainst using \"500 Internal Server Error\" as catch-all; I can agree\nthat \"403 Forbidden\" (which is from the times where gitweb was developed\nas separate project by Kay Sievers, old, old times of v056) is better\nleft for disabled features[*1*], and \"404 Not Found\" is better\ncatch-all.\n\nKay, do you remember why \"403 Forbidden\" was used as default catch-all\ngitweb HTTP error status code?\n \n> > probably me, Petr Baudis, John Hawley, perhaps Luben Tuikov\n> \n> I wouldn't want to Cc people if I don't address them personally -- e.g.\n> neither Petr nor John are currently working on gitweb, so flooding their\n> mailboxes might seem a little rude; if they're interested they can\n> always filter for subjects. (Unless someone requests to always be CC'ed\n> of course.)\n\nO.K. although I have though that as John is your GSoC mentor, he might\nbe interested gitweb caching related posts. But this is something\nbetter made agree with him.\n\nBTW. I got three copies of this email: was it you fighting VGER\nanti-spam filter?\n-- \nJakub Narebski\nPoland\n"},{"id":"80081","messageId":"4856DFE8.9010809@gmail.com","threadId":"13968","inReplyTo":"200806161843.09372.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: return correct HTTP status codes","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-16T21:49:28Z","receivedAt":"2008-06-16T21:49:28Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Jakub Narebski wrote:\n> Well, we could, perhaps, examine stderr (or redirect it to stdout and\n> examine upon error) to check what was the error.\n\nWe don't have to -- gitweb's current (suboptimal) error checking is \nbecause it doesn't interface with git very well. The API I'm writing \nwill fix this (i.e. provide proper feedback in all cases) so we'll have \nmore specific status codes. IOW, we'll be able to differentiate between \n500 and 404. Trust me on this one. ;-)\n\n> But I think in all, or almost all cases, the source is wrong parameters\n> in URL. Now, returning 5xx _server_ error would make me want to email\n> webmaster about error on his/her server, while 4xx _user_ error would\n> make me examine my input\n\nSince the status codes will get better (more accurate) anyway, I care \nmore about correctness than practicalities right now (and I'm convinced \nthat only 500 is correct in the cases we're talking about). That said, \nif you really want 404s in there, go ahead and send a follow-up patch, I \nwon't object.\n\n> BTW. I got three copies of this email: was it you fighting VGER\n> anti-spam filter?\n\nYup. Apparently it simply greps for Content-TypXe: text/hXtml. *shakes \nhead* :-)\n\n-- Lea\n"},{"id":"80083","messageId":"200806170034.42621.jnareb@gmail.com","threadId":"13968","inReplyTo":"4856DFE8.9010809@gmail.com","subject":"Re: [PATCH] gitweb: return correct HTTP status codes","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-16T22:34:41Z","receivedAt":"2008-06-16T22:34:41Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, 16 Jun 2008, Lea Wiemann wrote:\n> Jakub Narebski wrote:\n>>\n>> Well, we could, perhaps, examine stderr (or redirect it to stdout and\n>> examine upon error) to check what was the error.\n> \n> We don't have to -- gitweb's current (suboptimal) error checking is \n> because it doesn't interface with git very well.\n\nThe \"examine stderr\" was a bit tongue-in-cheek, i.e. solution which\nwould require least changes... but I guess very impractical.\n\n> The API I'm writing \n> will fix this (i.e. provide proper feedback in all cases) so we'll have \n> more specific status codes. IOW, we'll be able to differentiate between \n> 500 and 404. Trust me on this one. ;-)\n\nI have thought that Git.pm API together with catching (and examining)\nError, perhaps with redirecting STDERR somewhere (but it would be best\nif it would not be needed) would be enough.\n\n>> But I think in all, or almost all cases, the source is wrong parameters\n>> in URL. Now, returning 5xx _server_ error would make me want to email\n>> webmaster about error on his/her server, while 4xx _user_ error would\n>> make me examine my input\n> \n> Since the status codes will get better (more accurate) anyway, I care \n> more about correctness than practicalities right now (and I'm convinced \n> that only 500 is correct in the cases we're talking about). That said, \n> if you really want 404s in there, go ahead and send a follow-up patch, I \n> won't object.\n\nIf the source of error is some misconfiguration on server, then 5xx is\nappropriate (for example git binary is not found, something which\nperhaps we should check upfront at the beginning). But I think it\nshould be very, very rare, and result of misconfigured gitweb, or error\ninstalling git... or corrupted repository.\n\nIf source of error is mistake in URL, I would certainly want 4xx error.\nSo the user knows that he/she has to look at the URL.\n\nThat said, perhaps I am worrying over nothing, and\n or die_error(undef, \"Open failed\");\ncan happen only on some serious server error (like corrupted\nrepository).\n\n\n>From RFC 2616 (http://tools.ietf.org/html/rfc2616)\n\n 10.4 Client Error 4xx\n\n The 4xx class of status code is intended for cases in which the\n client seems to have erred.\n\n [...]\n\n 10.5 Server Error 5xx\n\n Response status codes beginning with the digit \"5\" indicate cases in\n which the server is aware that it has erred or is incapable of\n performing the request.\n \n>> BTW. I got three copies of this email: was it you fighting VGER\n>> anti-spam filter?\n> \n> Yup. Apparently it simply greps for Content-TypXe: text/hXtml. *shakes \n> head* :-)\n\nIt is unfortunately very simple pattern based filter, not Markovian,\nspam/ham Bayesian, or even simple Bayesian.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"80084","messageId":"7vy75580p1.fsf@gitster.siamese.dyndns.org","threadId":"13968","inReplyTo":"48568D5C.5090909@gmail.com","subject":"Re: [PATCH] gitweb: return correct HTTP status codes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-16T22:38:50Z","receivedAt":"2008-06-16T22:38:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lea Wiemann writes:\n\n> Jakub Narebski wrote:\n>...\n>>> I recommend that you apply [PATCH] gitweb: remove git_blame\n>>\n>> I think it is not possible, as IIRC [it] got already applied.\n>\n> It isn't applied on master (at the time I'm writing this), only on next.\n> Should I be working on the next branch?\n\nI think it was sensible for you to mention the patch dependency, and if\nyou said the same thing with a different phrasing \"this depends on remove\ngit_blame patch\" you wouldn't have opened yourself up to such a nitpick\n;-)\n\nSince you are working on something that is outside the current 1.5.6\nfreeze, and removal of the unused \"other\" blame sub was something nobody\nseemed to have objected to, I think it is Ok to assume that the removal\nwill happen when this new code is ready and after 1.5.6 ships.\n\nIt's up to you to work on 'next' or 'master with extra patches'. Just\nstate what you are basing your development on and with what extra patches\nif there is a risk of confusion among your readers.\n\n>>> \topen my $fd, \"-|\", git_cmd(), \"ls-tree\", $base, \"--\", $path\n>>> -\t\tor die_error(undef, \"Open git-ls-tree failed\");\n>>> +\t\tor die_error(500, \"Open git-ls-tree failed\");\n>>\n>> Should we really use \"500 Internal Server Error\" here? Usually this\n>> would be not an error at all, but wrong parameters to git command,\n>> i.e. it is _user_ who erred, not some error on _server_ side.\n>\n> You cannot tell for sure -- all you know here is that the command\n> somehow failed when it shouldn't have, and so all you can give is 500;\n> see below. I don't think we should apply reasoning like \"most commonly\n> it's a wrong hash, so let's return 404\" -- we don't know, and we\n> shouldn't assume.\n>\n>>> ... I suspect that the error could be triggered by non-existent hashes\n>>> as well, in which case the more specific 404 would be more appropriate\n>>> -- however, the current implementation oftentimes doesn't allow for\n>>> more fine-granulared checking\n>>\n>> ...that is why I'd rather have \"403 Forbidden\" as catch-all error, as\n>> it was done in gitweb. But that also seems not very good idea.\n>\n> [IANA HTTP expert, but:] All we have in cases like the one above is \"I\n> cannot deliver the content you requested, but I'm so confused that I\n> don't even know why\" -- which sounds like 500 to me (definitely not\n> 403). 403 would be used for some deactivated feature (commonly for\n> disabled directory listings I think) or something that shouldn't be\n> accessible to the public.\n\nHmph... if we cared deeply enough about this issue, isn't it possible for\nyou to unconfuse yourself in a slow path and figure out exactly why it\nfailed?\n\n unless (open $fd, '-|', ls-tree $base -- $path) {\n # Oh, an error? why?\n if (!object_exists($base)) {\n die_error(404, \"No such object $base\");\n } elsif (!is_a_treeish($base)) {\n die_error(404, \"No such tree-ish $base\");\n } else {\n die_error();\n }\n }\n"},{"id":"80088","messageId":"200806170137.56263.jnareb@gmail.com","threadId":"13968","inReplyTo":"48568D5C.5090909@gmail.com","subject":"Re: [PATCH] gitweb: return correct HTTP status codes","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-16T23:37:55Z","receivedAt":"2008-06-16T23:37:55Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, 16 Jun 2008, Lea Wiemann wrote:\n> Jakub Narebski wrote:\n>> \n>> I'd like to have reasoning when, and why, we use which responses (HTTP\n>> status codes) in which situations. Link to appropriate RFC you used\n>> (and perhaps to some other information)\n> \n> RFC 2616 has clear descriptions of the codes we use.\n\nThe RFC number was not mentioned in commit message, if I remember\ncorrectly. URL like http://tools.ietf.org/html/rfc2616 would also\nbe nice (especially when/if gitweb finally acquires committags\nsupport).\n\n> Maybe a really short list of conventions though, like you listed\n> below? \n\nThat would be nice.\n\nPerhaps it should be organized differently, i.e. start with different\nHTTP error status codes, and when they are used (situations), and not\nwith categories of situations and poposed HTTP codes.\n\n> If we have agreement I'll happily put it in.\n\nEven if there is no agreement, it is something that can be discussed\nwhen talking about given patch sent.\n\n>>> For convenience the die_error function now only takes the status code\n>>> without reason as first parameter\n>> \n>> Well, gitweb sometimes used different wording\n> \n> The 'reason' only appeared in the HTTP response header -- see below.\n\nI'd rather have explanation in HTTP resonse header, not only pure\nnumber. For example Test::WWW::Mechanize::CGI shows both code\nand _description_ when there is error (for get_ok).\n \n>>> \topen my $fd, \"-|\", git_cmd(), \"ls-tree\", $base, \"--\", $path\n>>> -\t\tor die_error(undef, \"Open git-ls-tree failed\");\n>>> +\t\tor die_error(500, \"Open git-ls-tree failed\");\n>> \n>> Should we really use \"500 Internal Server Error\" here? Usually this\n>> would be not an error at all, but wrong parameters to git command,\n>> i.e. it is _user_ who erred, not some error on _server_ side.\n> \n> You cannot tell for sure -- all you know here is that the command\n> somehow failed when it shouldn't have, and so all you can give is 500;\n> see below. I don't think we should apply reasoning like \"most commonly\n> it's a wrong hash, so let's return 404\" -- we don't know, and we\n> shouldn't assume.\n\nI think that in this situation, \"open or die_error(...)\", if error can\nhappen at all, it can happen only because of some serious error, like\ngit not installed[*1*], or corrupted repository, or something.\n\nWe would have to examine \"close or die_error(...)\" to check (perhaps\nlike Junio proposed examining details in a slow path, perhaps catching\nand examining Error from Git.pm methods, perhaps examining stderr)\nwhether this is user error with 4xx HTTP status code, or server error\nwith 5xx HTTP status code.\n \n\n[*1*] Perhaps we should have upfront in gitweb something like\n\n unless (-x \"$GIT\")\n \tdie_error(\"500 Internal Server Error\", 'Git binary not found');\n\nor something like that. Here it is _server_ error, and you should mail\nwebmaster / web page admin, and not examine your URL as in case of\n_user_ error of 4xx variety.\n\n>>> ... I suspect that the error could be triggered by non-existent hashes\n>>> as well, in which case the more specific 404 would be more appropriate\n>>> -- however, the current implementation oftentimes doesn't allow for\n>>> more fine-granulared checking\n>> \n>> ...that is why I'd rather have \"403 Forbidden\" as catch-all error, as\n>> it was done in gitweb. But that also seems not very good idea.\n> \n> [IANA HTTP expert, but:] All we have in cases like the one above is \"I\n> cannot deliver the content you requested, but I'm so confused that I\n> don't even know why\" -- which sounds like 500 to me (definitely not\n> 403). 403 would be used for some deactivated feature (commonly for\n> disabled directory listings I think) or something that shouldn't be\n> accessible to the public.\n\nI think that \"404 Not Found\" would be better catch-all than\n\"500 Internal Server Error\"... unless we are sure or almost sure that\nit _is_ server error.\n \n>>> sub die_error {\n>>> -\tmy $status = shift || \"403 Forbidden\";\n>>> -\tmy $error = shift || \"Malformed query, file missing or permission denied\";\n>>> +\tmy $status = shift || 500;\n>>> +\tmy $error = shift || \"Internal server error\";\n>> \n>> Errr, I think \"Malformed query, file missing or permission denied\" is\n>> actually closer to truth, and better error message\n> \n> This default is intended for expressions like \"some_action(...) or\n> die_error\" (without parameters), in which case you really can't tell\n> what triggered the error, so \"Internal server error\" is as specific as\n> you can get.\n\nI think that most errors are of the above... but I guess that it is\na moot point, because even if status sometimes is taken default,\nI don't think there is callsite that makes use of default for $error,\ni.e. second argument of die_error() is always set explicitely.\n \n>> (it is displayed in body of message)\n> \n> Just to make sure we're on the same boat, the $error variable above *is*\n> what's displayed in the body.\n\nTrue.\n \n>>> +\t# Use a generic \"Error\" reason, e.g. \"404 Error\" instead of\n>>> +\t# \"404 Not Found\". This is permissible by RFC 2616.\n>>> +\tgit_header_html(\"$status Error\");\n>> \n>> I don't think it is _recommended_ practice. IIRC it is recommnded\n>> [...] to give more detailed explanation in the body of message.\n> \n> The code above prints the HTTP response headers, not the body -- the\n> body contains the detailed error message passed to the die_error\n> function. Here's a stripped down version of gitweb's error response \n> (X'ed out against spam filter):\n> \n> HTXTP/1.0 404 Error <------------ \"Error\" here\n> CoXntent-Type: text/htXml\n> \n> ... 404 - Hash not found ...\n> \n> I don't know of *any* browser or tool that actually displays the reason\n> given in the HTTP response header, so defaulting to Error is fine here\n\nTest::WWW::Mechanize displays both numeric HTTP response *and* reason\ngiven in HTTP response header when get_ok(...) fails.\n \nBesides we would want some explanation also on HEAD reuests (supported\nnow only for web feeds, IIRC).\n\n> (especially since RFC 2616 explicitly allow it).\n\nCould you point me to where it is? And no, I don't think that \n\"The reason phrases listed here are only recommendations -- they MAY\n be replaced by local equivalents without affecting the protocol.\" \nis it.\n\n> (I don't want to put the $error passed to die_error into the HTTP\n> response header, since badly interpolated strings might invalidate the\n> response. So using \"Error\" makes sure we always die gracefully.)\n\nErrr... I think we can trust gitweb programmers to not screw up.\nPerhaps there should be comment that first argument should be plain\ntext, while second can be HTML fragment.\n \n\n>> Proposed HTTP server/client error status codes [heavily snipped]:\n\nLet us put it in different order.\n\n1. 4xx: Client Error - The request contains bad syntax or cannot\n be fulfilled\n\n1.1. 400 Bad Request - The request could not be understood by the\n server due to malformed syntax.\n \n Here I think we should put all the cases where we know that URL\n is invalid even without examining any data, after purely\n syntaxical checking. This means params with values outside their\n domain, like for example $page ('pg') parameter which is not\n a number.\n\n It *might* be also used for some unreasonable discrepancies\n in parameters which need access to git repository to check, like\n asking for blame view of non-blob... although this could also be\n \"404 Not Found\".\n\n\n1.2. 403 Forbidden - The server understood the request, but is refusing\n to fulfill it. Authorization will not help (...)\n\n (Sometimes as \"403 Permission Denied\").\n\n Here I think we should put all the cases where access is denied\n because of gitweb or repo configuration; this I think does not\n include non-exported projects. This usually means some 'feature'\n disabled (not enabled).\n\n1.3. 404 Not Found - The server has not found anything matching the\n Request-URI. No indication is given of whether the condition\n is temporary or permanent. (...) This status code is commonly\n used when the server does not wish to reveal exactly why the\n request has been refused, or when no other response is\n applicable.\n\n Here we should put all the situations where we ask for commit, and\n does not get it, as for a project, and such project does not exist,\n ask for a path, and there is no such path in specified tree-ish,\n asking for a tag while there is nothing under given name, etc.\n\n It could also be a reasonable catch-all.\n\n\n2. 5xx: Server Error - The server failed to fulfill an apparently\n valid request\n\n2.1. 500 Internal Server Error - The server encountered an unexpected\n condition which prevented it from fulfilling the request.\n\n Here I think we can put problems with git binaries, corrupted\n repositories, some Perl error etc. Something you should mail\n webmaster (web admin) about.\n\n All \"open or die_error(...)\" should I guess go there, although\n I'm not sure when/if that can happen. About \"close or die...\":\n those ones would need carefull examination if they should result\n in 404 or in 500 error code zone.\n\n2.2. 503 Service Unavailable - The server is currently unable to handle\n the request due to a temporary overloading or maintenance of\n the server. The implication is that this is a temporary\n condition which will be alleviated after some delay.\n\n Perhaps during longer regenerating cache this should be\n response...\n\n-- \nJakub Narebski\nShadeHawk on #git\nPoland\n"},{"id":"80137","messageId":"4857C1D7.2020702@gmail.com","threadId":"13968","inReplyTo":"200806170034.42621.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: return correct HTTP status codes","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-17T13:53:27Z","receivedAt":"2008-06-17T13:53:27Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Jakub Narebski wrote:\n> I have thought that Git.pm API together with catching (and examining)\n> Error, perhaps with redirecting STDERR somewhere (but it would be best\n> if it would not be needed) would be enough.\n\n(Just to clarify, I'm not touching Git.pm with my current work; I'm \nadding a few Git::* modules instead, and they work differently from \nGit.pm. So there's no Error, and there won't be any STDERR to catch at \nall.)\n\n-- Lea\n"},{"id":"80138","messageId":"4857C469.1000401@gmail.com","threadId":"13968","inReplyTo":"7vy75580p1.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] gitweb: return correct HTTP status codes","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-17T14:04:25Z","receivedAt":"2008-06-17T14:04:25Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Junio C Hamano wrote:\n> It's up to you to work on 'next' or 'master with extra patches'.\n\nOkay; I guess I'll use 'next with extra patches'. ;-)\n\n> [Gitweb's error handling:] isn't it possible for you to unconfuse\n> yourself in a slow path and figure out exactly why it failed?\n> \n> unless (open $fd, '-|', ls-tree $base -- $path) {\n> # Oh, an error? why?\n> if (!object_exists($base)) { [...]\n> } elsif (!is_a_treeish($base)) { [...]\n\nThat's possible, but the API I'm writing is designed the other way \nround: First, get the hash & type of $base; if it fails or the type is \nwrong, die accordingly. Then pass the hash you got into whatever call \nto git, and if that fails, you can quite safely assume that something \nserious went wrong. (The example above has an additional $path to deal \nwith, but you get the idea.)\n\nIOW, the strategy is \"don't let the git binaries resolve any object \nnames for you\", which should make both error handling and caching a lot \neasier.\n\n-- Lea\n"},{"id":"80145","messageId":"200806171633.26864.jnareb@gmail.com","threadId":"13968","inReplyTo":"4857C469.1000401@gmail.com","subject":"Re: [PATCH] gitweb: return correct HTTP status codes","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-17T14:33:26Z","receivedAt":"2008-06-17T14:33:26Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Lea Wiemann wrote:\n> Junio C Hamano wrote:\n \n> > [Gitweb's error handling:] isn't it possible for you to unconfuse\n> > yourself in a slow path and figure out exactly why it failed?\n> > \n> > unless (open $fd, '-|', ls-tree $base -- $path) {\n> > # Oh, an error? why?\n> > if (!object_exists($base)) { [...]\n> > } elsif (!is_a_treeish($base)) { [...]\n\nBTW. you can catch such errors on close(), not on open(), my mistake.\nOn open you can catch only fairly fatal errors (5xx category I think).\n \n> That's possible, but the API I'm writing is designed the other way \n> round: First, get the hash & type of $base; if it fails or the type is \n> wrong, die accordingly. Then pass the hash you got into whatever call \n> to git, and if that fails, you can quite safely assume that something \n> serious went wrong. (The example above has an additional $path to deal \n> with, but you get the idea.)\n> \n> IOW, the strategy is \"don't let the git binaries resolve any object \n> names for you\", which should make both error handling and caching a lot \n> easier.\n\nBut that means checking arguments in the \"fast path\", which means\nadditional calls to git commands in the _common_ case, not only in\nthe case of errors. I think that even with caching it is not a good\nidea.\n\nI'll try to come with example implementation using Error.pm and Git.pm\n-- \nJakub Narebski\nPoland\n"},{"id":"80175","messageId":"48583A73.7020508@gmail.com","threadId":"13968","inReplyTo":"200806171633.26864.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: return correct HTTP status codes","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-17T22:28:03Z","receivedAt":"2008-06-17T22:28:03Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Jakub Narebski wrote:\n> But that means checking arguments in the \"fast path\", which means\n> additional calls to git commands in the _common_ case, not only in\n> the case of errors.\n\nNo, it doesn't, it just pipes stuff into cat-file --batch-check, which \nhas to be opened on virtually any call to gitweb. Before telling me \nabout the performance of my code, can you please (a) read it and (b) \nbenchmark it? We lose a lot of time on pointless discussion otherwise.\n\n> I'll try to come with example implementation using Error.pm and Git.pm\n\nI don't think it's worth the time; I think I understand your point \nwithout an example implementation. :)\n\nUnrelatedly, I thought we had agreement not to use Error.pm as of \n \n(though Git.pm still does use Error.pm, so it can still be used as long \nas there's no patch that removes all instances of it). I got *really* \nweird unreproduceable errors when throwing exceptions with Error.pm in \ngitweb, and I suspect that Error.pm (perhaps in interaction with some \nother module) was to blame. Just a warning. ;-)\n\n-- Lea\n"},{"id":"80180","messageId":"200806180054.33490.jnareb@gmail.com","threadId":"13968","inReplyTo":"48583A73.7020508@gmail.com","subject":"Re: [PATCH] gitweb: return correct HTTP status codes","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-17T22:54:33Z","receivedAt":"2008-06-17T22:54:33Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Lea Wiemann wrote:\n> Jakub Narebski wrote:\n> >\n> > But that means checking arguments in the \"fast path\", which means\n> > additional calls to git commands in the _common_ case, not only in\n> > the case of errors.\n> \n> No, it doesn't, it just pipes stuff into cat-file --batch-check,\n\nAh, O.K., it does add additional call to git command, which should not\nmatter much performance wise on sane operating systems; it would matter\non OS with slow fork, like MS Windows, if gitweb ran on Windows\n(perhaps it can, but certainly not with ActiveState Perl, IIRC).\n\nBut what are arguments for \"check params; run command\" vs \"run command;\ncheck params if error\" proposed by Junio? Why do you want to check\nparameters upfront? Does it make code much, much easier?\n\n> which has to be opened on virtually any call to gitweb.\n\nIIRC it is for checking parameters? Even then, using it only on \"slow \npatch\", i.e. in preence of error might be better solution.\n\nOr is it needed for something else too?\n\n> Before telling me about the performance of my code, can you please\n> (a) read it [...]\n\nBy the way, would you be sending your current WIP for review?\n\n\nP.S. I wanted to ask in another subthread if adding object oriented \ninterface (wrapper) to git repositories (similar to the one used by \nStGIT / git-python perhaps?) is really needed for implementing gitweb \ncaching?; I still think that you have to chose which spots to cache, \nand do it from gitweb, and not cache everything. But if it is needed \nfor sane error reporting, and perhaps better ETags...\n\n-- \nJakub Narebski\nPoland\n"},{"id":"80185","messageId":"48584D1F.5070000@gmail.com","threadId":"13968","inReplyTo":"200806180054.33490.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: return correct HTTP status codes","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-17T23:47:43Z","receivedAt":"2008-06-17T23:47:43Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Jakub Narebski wrote:\n> But what are arguments for \"check params; run command\" vs \"run command;\n> check params if error\" proposed by Junio? Why do you want to check\n> parameters upfront?\n\nIt's actually not checking, it's resolving. Instead of ...\n\nget_commit($symbol)\n\n... (with $symbol = 'HEAD' for instance), you do (pseudocode):\n\n$hash = get_hash($symbol, 'commit'); # 'commit' to resolve tags\ncheck that $hash is defined\nget_commit($hash)\n\n(And get_commit won't even accept anything but 40-byte hashes.) This is \nfor two reasons:\n\n1. Caching: Resolving symbols first gives you some (very few) cache \nentries that need to be expired (namely, get_hash results for symbols \nthat are not SHA1 hashes already), but most cache entries (like the \nget_commit) are infinitely valid.\n\n2. Besides being a little more straightforward to implement, it ensures \nthat you have well-defined failure points. IOW, first you resolve the \nsymbols you're getting from the user and check them for existence (and \ncorrect type). From then on, any hashes you pass around are guaranteed \nto be valid, so failures indicate that something serious went wrong. \nApart from making things easier for the developer by reminding them of \nthe points where they *need* to check for errors, it means that you can \nvery easily check the error-handling code for completeness (by only \nreading the first few lines that resolve symbols).\n\n> By the way, would you be sending your current WIP for review?\n\nWill do in the next 2 days after some cleanup.\n\n> [is] adding object oriented interface (wrapper) to git repositories\n> really needed for implementing gitweb caching?\n\nIt makes things better to read for sure, and it means that you can have \na plain API without caching, and implement the caching layer as a \nsubclass (which overrides the methods with cacheable results).\n\n-- Lea\n"},{"id":"80187","messageId":"200806180212.15392.jnareb@gmail.com","threadId":"13968","inReplyTo":"48584D1F.5070000@gmail.com","subject":"Re: [PATCH] gitweb: return correct HTTP status codes","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-18T00:12:15Z","receivedAt":"2008-06-18T00:12:15Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Lea Wiemann wrote:\n> Jakub Narebski wrote:\n> >\n> > But what are arguments for \"check params; run command\" vs \"run command;\n> > check params if error\" proposed by Junio? Why do you want to check\n> > parameters upfront?\n> \n> It's actually not checking, it's resolving. Instead of ...\n> \n> get_commit($symbol)\n> \n> ... (with $symbol = 'HEAD' for instance), you do (pseudocode):\n> \n> $hash = get_hash($symbol, 'commit'); # 'commit' to resolve tags\n\nErrr... is there equivalent to ^{}, i.e. resolve to non-tag?\n\n> check that $hash is defined\n> get_commit($hash)\n\nNote that you would have to examine gitweb sources to check if it\nuses href(..., -replay=>1) when it should, so the links from \"volatile\"\nlink, e.g. HEAD version URL, also lead to appropriate version,\ne.g. \"tree\" from current / HEAD version.\n\n> (And get_commit won't even accept anything but 40-byte hashes.) This is \n> for two reasons:\n> \n> 1. Caching: Resolving symbols first gives you some (very few) cache \n> entries that need to be expired (namely, get_hash results for symbols \n> that are not SHA1 hashes already), but most cache entries (like the \n> get_commit) are infinitely valid.\n\nBTW. one of earliest idea was to fully resolve hashes, add missing\nparameters if possible (like 'h', 'hp', 'f') and convert hashes to\nsha-1. One of intended uses was (weak) ETag for simple HTTP caching.\n\nBut it was before git-cat-file --batch-check, and before\nhref(...,-replay=>1).\n\n[...]\n\nThanks for explanation. It makes clear why you have chosen this\nsolution (I think that (1) is deciding factor here).\n\n\n> > By the way, would you be sending your current WIP for review?\n> \n> Will do in the next 2 days after some cleanup.\n\nThanks.\n\nI have read your current code, but I'd rather not comment on it\n(like no_plan and skipping tests, or elaborate wrapping fields\nvs. perltie) until it is ready, i.e. sent for peer review.\n\n> > [is] adding object oriented interface (wrapper) to git repositories\n> > really needed for implementing gitweb caching?\n> \n> It makes things better to read for sure, and it means that you can have \n> a plain API without caching, and implement the caching layer as a \n> subclass (which overrides the methods with cacheable results).\n\nAll the time I think that caching _everything_ is a bad solution.\nBesides sometimes you need to cache information specific for gitweb,\nor information _aggregated_ by gitweb, which doesn't have place as\nsingle entity in Git::Repo.\n\nSo while I think that Git::Repo and making gitweb use it is\na worthwhile goal, I think caching should be explicitely in gitweb.\nSee CGI::Cache for good solution of inobtrusive output caching, and\nCHI (or other in recommended thread) for inobtrusive data caching\nusing callbacks (and Cache::None engine). That not said that we\nshould use such non-standard Perl modules!\n\n\n-- \nJakub Narebski\nPoland\n"},{"id":"80186","messageId":"1213748115-4038-1-git-send-email-LeWiemann@gmail.com","threadId":"13968","inReplyTo":"1213564515-14356-1-git-send-email-LeWiemann@gmail.com","subject":"[PATCH] gitweb: standarize HTTP status codes","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-18T00:15:15Z","receivedAt":"2008-06-18T00:15:15Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Many error status codes simply default to 403 Forbidden, which is not\ncorrect in most cases. This patch makes gitweb return semantically\ncorrect status codes.\n\nFor convenience the die_error function now only takes the status code\nwithout reason as first parameter (e.g. 404 instead of \"404 Not\nFound\"), and it now defaults to 500 (Internal Server Error), even\nthough the default is not used anywhere.\n\nAlso documented status code conventions in die_error.\n\nSigned-off-by: Lea Wiemann \n---\nChanges since v1:\n\n- Changed subject per Jakub's suggestion.\n- Resolved merge conflict caused by \"[PATCH v2] gitweb: quote commands\n properly when calling the shell\".\n- Added the following documentation to die_error (I know Jakub might\n disagree with me on the last line ;-)):\n\n# die_error(, )\n# Example: die_error(404, 'Hash not found')\n# By convention, use the following status codes (as defined in RFC 2616):\n# 400: Invalid or missing CGI parameters, or\n# requested object exists but has wrong type.\n# 403: Requested feature (like \"pickaxe\" or \"snapshot\") not enabled on\n# this server or project.\n# 404: Requested object/revision/project doesn't exist.\n# 500: The server isn't configured properly, or\n# an internal error occurred (e.g. failed assertions caused by bugs), or\n# an unknown error occurred (e.g. the git binary died unexpectedly).\n\n\n gitweb/gitweb.perl | 207 ++++++++++++++++++++++++++--------------------------\n 1 files changed, 102 insertions(+), 105 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 3a7adae..3ca45fd 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -386,7 +386,7 @@ $projects_list ||= $projectroot;\n our $action = $cgi->param('a');\n if (defined $action) {\n \tif ($action =~ m/[^0-9a-zA-Z\\.\\-_]/) {\n-\t\tdie_error(undef, \"Invalid action parameter\");\n+\t\tdie_error(400, \"Invalid action parameter\");\n \t}\n }\n \n@@ -399,21 +399,21 @@ if (defined $project) {\n \t ($export_ok && !(-e \"$projectroot/$project/$export_ok\")) ||\n \t ($strict_export && !project_in_list($project))) {\n \t\tundef $project;\n-\t\tdie_error(undef, \"No such project\");\n+\t\tdie_error(404, \"No such project\");\n \t}\n }\n \n our $file_name = $cgi->param('f');\n if (defined $file_name) {\n \tif (!validate_pathname($file_name)) {\n-\t\tdie_error(undef, \"Invalid file parameter\");\n+\t\tdie_error(400, \"Invalid file parameter\");\n \t}\n }\n \n our $file_parent = $cgi->param('fp');\n if (defined $file_parent) {\n \tif (!validate_pathname($file_parent)) {\n-\t\tdie_error(undef, \"Invalid file parent parameter\");\n+\t\tdie_error(400, \"Invalid file parent parameter\");\n \t}\n }\n \n@@ -421,21 +421,21 @@ if (defined $file_parent) {\n our $hash = $cgi->param('h');\n if (defined $hash) {\n \tif (!validate_refname($hash)) {\n-\t\tdie_error(undef, \"Invalid hash parameter\");\n+\t\tdie_error(400, \"Invalid hash parameter\");\n \t}\n }\n \n our $hash_parent = $cgi->param('hp');\n if (defined $hash_parent) {\n \tif (!validate_refname($hash_parent)) {\n-\t\tdie_error(undef, \"Invalid hash parent parameter\");\n+\t\tdie_error(400, \"Invalid hash parent parameter\");\n \t}\n }\n \n our $hash_base = $cgi->param('hb');\n if (defined $hash_base) {\n \tif (!validate_refname($hash_base)) {\n-\t\tdie_error(undef, \"Invalid hash base parameter\");\n+\t\tdie_error(400, \"Invalid hash base parameter\");\n \t}\n }\n \n@@ -447,10 +447,10 @@ our @extra_options = $cgi->param('opt');\n if (defined @extra_options) {\n \tforeach my $opt (@extra_options) {\n \t\tif (not exists $allowed_options{$opt}) {\n-\t\t\tdie_error(undef, \"Invalid option parameter\");\n+\t\t\tdie_error(400, \"Invalid option parameter\");\n \t\t}\n \t\tif (not grep(/^$action$/, @{$allowed_options{$opt}})) {\n-\t\t\tdie_error(undef, \"Invalid option parameter for this action\");\n+\t\t\tdie_error(400, \"Invalid option parameter for this action\");\n \t\t}\n \t}\n }\n@@ -458,7 +458,7 @@ if (defined @extra_options) {\n our $hash_parent_base = $cgi->param('hpb');\n if (defined $hash_parent_base) {\n \tif (!validate_refname($hash_parent_base)) {\n-\t\tdie_error(undef, \"Invalid hash parent base parameter\");\n+\t\tdie_error(400, \"Invalid hash parent base parameter\");\n \t}\n }\n \n@@ -466,14 +466,14 @@ if (defined $hash_parent_base) {\n our $page = $cgi->param('pg');\n if (defined $page) {\n \tif ($page =~ m/[^0-9]/) {\n-\t\tdie_error(undef, \"Invalid page parameter\");\n+\t\tdie_error(400, \"Invalid page parameter\");\n \t}\n }\n \n our $searchtype = $cgi->param('st');\n if (defined $searchtype) {\n \tif ($searchtype =~ m/[^a-z]/) {\n-\t\tdie_error(undef, \"Invalid searchtype parameter\");\n+\t\tdie_error(400, \"Invalid searchtype parameter\");\n \t}\n }\n \n@@ -483,7 +483,7 @@ our $searchtext = $cgi->param('s');\n our $search_regexp;\n if (defined $searchtext) {\n \tif (length($searchtext) < 2) {\n-\t\tdie_error(undef, \"At least two characters are required for search parameter\");\n+\t\tdie_error(403, \"At least two characters are required for search parameter\");\n \t}\n \t$search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;\n }\n@@ -580,11 +580,11 @@ if (!defined $action) {\n \t}\n }\n if (!defined($actions{$action})) {\n-\tdie_error(undef, \"Unknown action\");\n+\tdie_error(400, \"Unknown action\");\n }\n if ($action !~ m/^(opml|project_list|project_index)$/ &&\n !$project) {\n-\tdie_error(undef, \"Project needed\");\n+\tdie_error(400, \"Project needed\");\n }\n $actions{$action}->();\n exit;\n@@ -1665,7 +1665,7 @@ sub git_get_hash_by_path {\n \t$path =~ s,/+$,,;\n \n \topen my $fd, \"-|\", git_cmd(), \"ls-tree\", $base, \"--\", $path\n-\t\tor die_error(undef, \"Open git-ls-tree failed\");\n+\t\tor die_error(500, \"Open git-ls-tree failed\");\n \tmy $line = <$fd>;\n \tclose $fd or return undef;\n \n@@ -2127,7 +2127,7 @@ sub parse_commit {\n \t\t\"--max-count=1\",\n \t\t$commit_id,\n \t\t\"--\",\n-\t\tor die_error(undef, \"Open git-rev-list failed\");\n+\t\tor die_error(500, \"Open git-rev-list failed\");\n \t%co = parse_commit_text(<$fd>, 1);\n \tclose $fd;\n \n@@ -2152,7 +2152,7 @@ sub parse_commits {\n \t\t$commit_id,\n \t\t\"--\",\n \t\t($filename ? ($filename) : ())\n-\t\tor die_error(undef, \"Open git-rev-list failed\");\n+\t\tor die_error(500, \"Open git-rev-list failed\");\n \twhile (my $line = <$fd>) {\n \t\tmy %co = parse_commit_text($line);\n \t\tpush @cos, \\%co;\n@@ -2672,11 +2672,24 @@ sub git_footer_html {\n \t \"\";\n }\n \n+# die_error(, )\n+# Example: die_error(404, 'Hash not found')\n+# By convention, use the following status codes (as defined in RFC 2616):\n+# 400: Invalid or missing CGI parameters, or\n+# requested object exists but has wrong type.\n+# 403: Requested feature (like \"pickaxe\" or \"snapshot\") not enabled on\n+# this server or project.\n+# 404: Requested object/revision/project doesn't exist.\n+# 500: The server isn't configured properly, or\n+# an internal error occurred (e.g. failed assertions caused by bugs), or\n+# an unknown error occurred (e.g. the git binary died unexpectedly).\n sub die_error {\n-\tmy $status = shift || \"403 Forbidden\";\n-\tmy $error = shift || \"Malformed query, file missing or permission denied\";\n+\tmy $status = shift || 500;\n+\tmy $error = shift || \"Internal server error\";\n \n-\tgit_header_html($status);\n+\t# Use a generic \"Error\" reason, e.g. \"404 Error\" instead of\n+\t# \"404 Not Found\". This is permissible by RFC 2616.\n+\tgit_header_html(\"$status Error\");\n \tprint <\n

\n@@ -3924,12 +3937,12 @@ sub git_search_grep_body {\n sub git_project_list {\n \tmy $order = $cgi->param('o');\n \tif (defined $order && $order !~ m/none|project|descr|owner|age/) {\n-\t\tdie_error(undef, \"Unknown order parameter\");\n+\t\tdie_error(400, \"Unknown order parameter\");\n \t}\n \n \tmy @list = git_get_projects_list();\n \tif (!@list) {\n-\t\tdie_error(undef, \"No projects found\");\n+\t\tdie_error(404, \"No projects found\");\n \t}\n \n \tgit_header_html();\n@@ -3947,12 +3960,12 @@ sub git_project_list {\n sub git_forks {\n \tmy $order = $cgi->param('o');\n \tif (defined $order && $order !~ m/none|project|descr|owner|age/) {\n-\t\tdie_error(undef, \"Unknown order parameter\");\n+\t\tdie_error(400, \"Unknown order parameter\");\n \t}\n \n \tmy @list = git_get_projects_list($project);\n \tif (!@list) {\n-\t\tdie_error(undef, \"No forks found\");\n+\t\tdie_error(404, \"No forks found\");\n \t}\n \n \tgit_header_html();\n@@ -4081,7 +4094,7 @@ sub git_tag {\n \tmy %tag = parse_tag($hash);\n \n \tif (! %tag) {\n-\t\tdie_error(undef, \"Unknown tag object\");\n+\t\tdie_error(404, \"Unknown tag object\");\n \t}\n \n \tgit_print_header_div('commit', esc_html($tag{'name'}), $hash);\n@@ -4117,26 +4130,25 @@ sub git_blame {\n \tmy $fd;\n \tmy $ftype;\n \n-\tmy ($have_blame) = gitweb_check_feature('blame');\n-\tif (!$have_blame) {\n-\t\tdie_error('403 Permission denied', \"Permission denied\");\n-\t}\n-\tdie_error('404 Not Found', \"File name not defined\") if (!$file_name);\n+\tgitweb_check_feature('blame')\n+\t or die_error(403, \"Blame not allowed\");\n+\n+\tdie_error(400, \"No file name given\") unless $file_name;\n \t$hash_base ||= git_get_head_hash($project);\n-\tdie_error(undef, \"Couldn't find base commit\") unless ($hash_base);\n+\tdie_error(400, \"Couldn't find base commit\") unless ($hash_base);\n \tmy %co = parse_commit($hash_base)\n-\t\tor die_error(undef, \"Reading commit failed\");\n+\t\tor die_error(404, \"Commit not found\");\n \tif (!defined $hash) {\n \t\t$hash = git_get_hash_by_path($hash_base, $file_name, \"blob\")\n-\t\t\tor die_error(undef, \"Error looking up file\");\n+\t\t\tor die_error(404, \"Error looking up file\");\n \t}\n \t$ftype = git_get_type($hash);\n \tif ($ftype !~ \"blob\") {\n-\t\tdie_error('400 Bad Request', \"Object is not a blob\");\n+\t\tdie_error(400, \"Object is not a blob\");\n \t}\n \topen ($fd, \"-|\", git_cmd(), \"blame\", '-p', '--',\n \t $file_name, $hash_base)\n-\t\tor die_error(undef, \"Open git-blame failed\");\n+\t\tor die_error(500, \"Open git-blame failed\");\n \tgit_header_html();\n \tmy $formats_nav =\n \t\t$cgi->a({-href => href(action=>\"blob\", -replay=>1)},\n@@ -4198,7 +4210,7 @@ HTML\n \t\t\tprint \"\\n\";\n \t\t}\n \t\topen (my $dd, \"-|\", git_cmd(), \"rev-parse\", \"$full_rev^\")\n-\t\t\tor die_error(undef, \"Open git-rev-parse failed\");\n+\t\t\tor die_error(500, \"Open git-rev-parse failed\");\n \t\tmy $parent_commit = <$dd>;\n \t\tclose $dd;\n \t\tchomp($parent_commit);\n@@ -4255,9 +4267,9 @@ sub git_blob_plain {\n \t\tif (defined $file_name) {\n \t\t\tmy $base = $hash_base || git_get_head_hash($project);\n \t\t\t$hash = git_get_hash_by_path($base, $file_name, \"blob\")\n-\t\t\t\tor die_error(undef, \"Error lookup file\");\n+\t\t\t\tor die_error(404, \"Cannot find file\");\n \t\t} else {\n-\t\t\tdie_error(undef, \"No file name defined\");\n+\t\t\tdie_error(400, \"No file name defined\");\n \t\t}\n \t} elsif ($hash =~ m/^[0-9a-fA-F]{40}$/) {\n \t\t# blobs defined by non-textual hash id's can be cached\n@@ -4265,7 +4277,7 @@ sub git_blob_plain {\n \t}\n \n \topen my $fd, \"-|\", git_cmd(), \"cat-file\", \"blob\", $hash\n-\t\tor die_error(undef, \"Open git-cat-file blob '$hash' failed\");\n+\t\tor die_error(500, \"Open git-cat-file blob '$hash' failed\");\n \n \t# content-type (can include charset)\n \t$type = blob_contenttype($fd, $file_name, $type);\n@@ -4297,9 +4309,9 @@ sub git_blob {\n \t\tif (defined $file_name) {\n \t\t\tmy $base = $hash_base || git_get_head_hash($project);\n \t\t\t$hash = git_get_hash_by_path($base, $file_name, \"blob\")\n-\t\t\t\tor die_error(undef, \"Error lookup file\");\n+\t\t\t\tor die_error(404, \"Cannot find file\");\n \t\t} else {\n-\t\t\tdie_error(undef, \"No file name defined\");\n+\t\t\tdie_error(400, \"No file name defined\");\n \t\t}\n \t} elsif ($hash =~ m/^[0-9a-fA-F]{40}$/) {\n \t\t# blobs defined by non-textual hash id's can be cached\n@@ -4308,7 +4320,7 @@ sub git_blob {\n \n \tmy ($have_blame) = gitweb_check_feature('blame');\n \topen my $fd, \"-|\", git_cmd(), \"cat-file\", \"blob\", $hash\n-\t\tor die_error(undef, \"Couldn't cat $file_name, $hash\");\n+\t\tor die_error(500, \"Couldn't cat $file_name, $hash\");\n \tmy $mimetype = blob_mimetype($fd, $file_name);\n \tif ($mimetype !~ m!^(?:text/|image/(?:gif|png|jpeg)$)! && -B $fd) {\n \t\tclose $fd;\n@@ -4389,9 +4401,9 @@ sub git_tree {\n \t}\n \t$/ = \"\\0\";\n \topen my $fd, \"-|\", git_cmd(), \"ls-tree\", '-z', $hash\n-\t\tor die_error(undef, \"Open git-ls-tree failed\");\n+\t\tor die_error(500, \"Open git-ls-tree failed\");\n \tmy @entries = map { chomp; $_ } <$fd>;\n-\tclose $fd or die_error(undef, \"Reading tree failed\");\n+\tclose $fd or die_error(500, \"Reading tree failed\");\n \t$/ = \"\\n\";\n \n \tmy $refs = git_get_references();\n@@ -4481,16 +4493,16 @@ sub git_snapshot {\n \n \tmy $format = $cgi->param('sf');\n \tif (!@supported_fmts) {\n-\t\tdie_error('403 Permission denied', \"Permission denied\");\n+\t\tdie_error(403, \"Snapshots not allowed\");\n \t}\n \t# default to first supported snapshot format\n \t$format ||= $supported_fmts[0];\n \tif ($format !~ m/^[a-z0-9]+$/) {\n-\t\tdie_error(undef, \"Invalid snapshot format parameter\");\n+\t\tdie_error(400, \"Invalid snapshot format parameter\");\n \t} elsif (!exists($known_snapshot_formats{$format})) {\n-\t\tdie_error(undef, \"Unknown snapshot format\");\n+\t\tdie_error(400, \"Unknown snapshot format\");\n \t} elsif (!grep($_ eq $format, @supported_fmts)) {\n-\t\tdie_error(undef, \"Unsupported snapshot format\");\n+\t\tdie_error(403, \"Unsupported snapshot format\");\n \t}\n \n \tif (!defined $hash) {\n@@ -4518,7 +4530,7 @@ sub git_snapshot {\n \t\t-status => '200 OK');\n \n \topen my $fd, \"-|\", $cmd\n-\t\tor die_error(undef, \"Execute git-archive failed\");\n+\t\tor die_error(500, \"Execute git-archive failed\");\n \tbinmode STDOUT, ':raw';\n \tprint <$fd>;\n \tbinmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi\n@@ -4586,10 +4598,8 @@ sub git_log {\n \n sub git_commit {\n \t$hash ||= $hash_base || \"HEAD\";\n-\tmy %co = parse_commit($hash);\n-\tif (!%co) {\n-\t\tdie_error(undef, \"Unknown commit object\");\n-\t}\n+\tmy %co = parse_commit($hash)\n+\t or die_error(404, \"Unknown commit object\");\n \tmy %ad = parse_date($co{'author_epoch'}, $co{'author_tz'});\n \tmy %cd = parse_date($co{'committer_epoch'}, $co{'committer_tz'});\n \n@@ -4629,9 +4639,9 @@ sub git_commit {\n \t\t@diff_opts,\n \t\t(@$parents <= 1 ? $parent : '-c'),\n \t\t$hash, \"--\"\n-\t\tor die_error(undef, \"Open git-diff-tree failed\");\n+\t\tor die_error(500, \"Open git-diff-tree failed\");\n \t@difftree = map { chomp; $_ } <$fd>;\n-\tclose $fd or die_error(undef, \"Reading git-diff-tree failed\");\n+\tclose $fd or die_error(500, \"Reading git-diff-tree failed\");\n \n \t# non-textual hash id's can be cached\n \tmy $expires;\n@@ -4724,33 +4734,33 @@ sub git_object {\n \n \t\topen my $fd, \"-|\", quote_command(\n \t\t\tgit_cmd(), 'cat-file', '-t', $object_id) . ' 2> /dev/null'\n-\t\t\tor die_error('404 Not Found', \"Object does not exist\");\n+\t\t\tor die_error(404, \"Object does not exist\");\n \t\t$type = <$fd>;\n \t\tchomp $type;\n \t\tclose $fd\n-\t\t\tor die_error('404 Not Found', \"Object does not exist\");\n+\t\t\tor die_error(404, \"Object does not exist\");\n \n \t# - hash_base and file_name\n \t} elsif ($hash_base && defined $file_name) {\n \t\t$file_name =~ s,/+$,,;\n \n \t\tsystem(git_cmd(), \"cat-file\", '-e', $hash_base) == 0\n-\t\t\tor die_error('404 Not Found', \"Base object does not exist\");\n+\t\t\tor die_error(404, \"Base object does not exist\");\n \n \t\t# here errors should not hapen\n \t\topen my $fd, \"-|\", git_cmd(), \"ls-tree\", $hash_base, \"--\", $file_name\n-\t\t\tor die_error(undef, \"Open git-ls-tree failed\");\n+\t\t\tor die_error(500, \"Open git-ls-tree failed\");\n \t\tmy $line = <$fd>;\n \t\tclose $fd;\n \n \t\t#'100644 blob 0fa3f3a66fb6a137f6ec2c19351ed4d807070ffa\tpanic.c'\n \t\tunless ($line && $line =~ m/^([0-9]+) (.+) ([0-9a-fA-F]{40})\\t/) {\n-\t\t\tdie_error('404 Not Found', \"File or directory for given base does not exist\");\n+\t\t\tdie_error(404, \"File or directory for given base does not exist\");\n \t\t}\n \t\t$type = $2;\n \t\t$hash = $3;\n \t} else {\n-\t\tdie_error('404 Not Found', \"Not enough information to find object\");\n+\t\tdie_error(400, \"Not enough information to find object\");\n \t}\n \n \tprint $cgi->redirect(-uri => href(action=>$type, -full=>1,\n@@ -4775,12 +4785,12 @@ sub git_blobdiff {\n \t\t\topen $fd, \"-|\", git_cmd(), \"diff-tree\", '-r', @diff_opts,\n \t\t\t\t$hash_parent_base, $hash_base,\n \t\t\t\t\"--\", (defined $file_parent ? $file_parent : ()), $file_name\n-\t\t\t\tor die_error(undef, \"Open git-diff-tree failed\");\n+\t\t\t\tor die_error(500, \"Open git-diff-tree failed\");\n \t\t\t@difftree = map { chomp; $_ } <$fd>;\n \t\t\tclose $fd\n-\t\t\t\tor die_error(undef, \"Reading git-diff-tree failed\");\n+\t\t\t\tor die_error(500, \"Reading git-diff-tree failed\");\n \t\t\t@difftree\n-\t\t\t\tor die_error('404 Not Found', \"Blob diff not found\");\n+\t\t\t\tor die_error(404, \"Blob diff not found\");\n \n \t\t} elsif (defined $hash &&\n \t\t $hash =~ /[0-9a-fA-F]{40}/) {\n@@ -4789,23 +4799,23 @@ sub git_blobdiff {\n \t\t\t# read filtered raw output\n \t\t\topen $fd, \"-|\", git_cmd(), \"diff-tree\", '-r', @diff_opts,\n \t\t\t\t$hash_parent_base, $hash_base, \"--\"\n-\t\t\t\tor die_error(undef, \"Open git-diff-tree failed\");\n+\t\t\t\tor die_error(500, \"Open git-diff-tree failed\");\n \t\t\t@difftree =\n \t\t\t\t# ':100644 100644 03b21826... 3b93d5e7... M\tls-files.c'\n \t\t\t\t# $hash == to_id\n \t\t\t\tgrep { /^:[0-7]{6} [0-7]{6} [0-9a-fA-F]{40} $hash/ }\n \t\t\t\tmap { chomp; $_ } <$fd>;\n \t\t\tclose $fd\n-\t\t\t\tor die_error(undef, \"Reading git-diff-tree failed\");\n+\t\t\t\tor die_error(500, \"Reading git-diff-tree failed\");\n \t\t\t@difftree\n-\t\t\t\tor die_error('404 Not Found', \"Blob diff not found\");\n+\t\t\t\tor die_error(404, \"Blob diff not found\");\n \n \t\t} else {\n-\t\t\tdie_error('404 Not Found', \"Missing one of the blob diff parameters\");\n+\t\t\tdie_error(400, \"Missing one of the blob diff parameters\");\n \t\t}\n \n \t\tif (@difftree > 1) {\n-\t\t\tdie_error('404 Not Found', \"Ambiguous blob diff specification\");\n+\t\t\tdie_error(400, \"Ambiguous blob diff specification\");\n \t\t}\n \n \t\t%diffinfo = parse_difftree_raw_line($difftree[0]);\n@@ -4826,7 +4836,7 @@ sub git_blobdiff {\n \t\t\t'-p', ($format eq 'html' ? \"--full-index\" : ()),\n \t\t\t$hash_parent_base, $hash_base,\n \t\t\t\"--\", (defined $file_parent ? $file_parent : ()), $file_name\n-\t\t\tor die_error(undef, \"Open git-diff-tree failed\");\n+\t\t\tor die_error(500, \"Open git-diff-tree failed\");\n \t}\n \n \t# old/legacy style URI\n@@ -4862,9 +4872,9 @@ sub git_blobdiff {\n \t\topen $fd, \"-|\", git_cmd(), \"diff\", @diff_opts,\n \t\t\t'-p', ($format eq 'html' ? \"--full-index\" : ()),\n \t\t\t$hash_parent, $hash, \"--\"\n-\t\t\tor die_error(undef, \"Open git-diff failed\");\n+\t\t\tor die_error(500, \"Open git-diff failed\");\n \t} else {\n-\t\tdie_error('404 Not Found', \"Missing one of the blob diff parameters\")\n+\t\tdie_error(400, \"Missing one of the blob diff parameters\")\n \t\t\tunless %diffinfo;\n \t}\n \n@@ -4897,7 +4907,7 @@ sub git_blobdiff {\n \t\tprint \"X-Git-Url: \" . $cgi->self_url() . \"\\n\\n\";\n \n \t} else {\n-\t\tdie_error(undef, \"Unknown blobdiff format\");\n+\t\tdie_error(400, \"Unknown blobdiff format\");\n \t}\n \n \t# patch\n@@ -4932,10 +4942,8 @@ sub git_blobdiff_plain {\n sub git_commitdiff {\n \tmy $format = shift || 'html';\n \t$hash ||= $hash_base || \"HEAD\";\n-\tmy %co = parse_commit($hash);\n-\tif (!%co) {\n-\t\tdie_error(undef, \"Unknown commit object\");\n-\t}\n+\tmy %co = parse_commit($hash)\n+\t or die_error(404, \"Unknown commit object\");\n \n \t# choose format for commitdiff for merge\n \tif (! defined $hash_parent && @{$co{'parents'}} > 1) {\n@@ -5017,7 +5025,7 @@ sub git_commitdiff {\n \t\topen $fd, \"-|\", git_cmd(), \"diff-tree\", '-r', @diff_opts,\n \t\t\t\"--no-commit-id\", \"--patch-with-raw\", \"--full-index\",\n \t\t\t$hash_parent_param, $hash, \"--\"\n-\t\t\tor die_error(undef, \"Open git-diff-tree failed\");\n+\t\t\tor die_error(500, \"Open git-diff-tree failed\");\n \n \t\twhile (my $line = <$fd>) {\n \t\t\tchomp $line;\n@@ -5029,10 +5037,10 @@ sub git_commitdiff {\n \t} elsif ($format eq 'plain') {\n \t\topen $fd, \"-|\", git_cmd(), \"diff-tree\", '-r', @diff_opts,\n \t\t\t'-p', $hash_parent_param, $hash, \"--\"\n-\t\t\tor die_error(undef, \"Open git-diff-tree failed\");\n+\t\t\tor die_error(500, \"Open git-diff-tree failed\");\n \n \t} else {\n-\t\tdie_error(undef, \"Unknown commitdiff format\");\n+\t\tdie_error(400, \"Unknown commitdiff format\");\n \t}\n \n \t# non-textual hash id's can be cached\n@@ -5115,19 +5123,15 @@ sub git_history {\n \t\t$page = 0;\n \t}\n \tmy $ftype;\n-\tmy %co = parse_commit($hash_base);\n-\tif (!%co) {\n-\t\tdie_error(undef, \"Unknown commit object\");\n-\t}\n+\tmy %co = parse_commit($hash_base)\n+\t or die_error(404, \"Unknown commit object\");\n \n \tmy $refs = git_get_references();\n \tmy $limit = sprintf(\"--max-count=%i\", (100 * ($page+1)));\n \n \tmy @commitlist = parse_commits($hash_base, 101, (100 * $page),\n-\t $file_name, \"--full-history\");\n-\tif (!@commitlist) {\n-\t\tdie_error('404 Not Found', \"No such file or directory on given branch\");\n-\t}\n+\t $file_name, \"--full-history\")\n+\t or die_error(404, \"No such file or directory on given branch\");\n \n \tif (!defined $hash && defined $file_name) {\n \t\t# some commits could have deleted file in question,\n@@ -5141,7 +5145,7 @@ sub git_history {\n \t\t$ftype = git_get_type($hash);\n \t}\n \tif (!defined $ftype) {\n-\t\tdie_error(undef, \"Unknown type of object\");\n+\t\tdie_error(500, \"Unknown type of object\");\n \t}\n \n \tmy $paging_nav = '';\n@@ -5179,19 +5183,16 @@ sub git_history {\n }\n \n sub git_search {\n-\tmy ($have_search) = gitweb_check_feature('search');\n-\tif (!$have_search) {\n-\t\tdie_error('403 Permission denied', \"Permission denied\");\n-\t}\n+\tgitweb_check_feature('search') or die_error(403, \"Search is disabled\");\n \tif (!defined $searchtext) {\n-\t\tdie_error(undef, \"Text field empty\");\n+\t\tdie_error(400, \"Text field is empty\");\n \t}\n \tif (!defined $hash) {\n \t\t$hash = git_get_head_hash($project);\n \t}\n \tmy %co = parse_commit($hash);\n \tif (!%co) {\n-\t\tdie_error(undef, \"Unknown commit object\");\n+\t\tdie_error(404, \"Unknown commit object\");\n \t}\n \tif (!defined $page) {\n \t\t$page = 0;\n@@ -5201,16 +5202,12 @@ sub git_search {\n \tif ($searchtype eq 'pickaxe') {\n \t\t# pickaxe may take all resources of your box and run for several minutes\n \t\t# with every query - so decide by yourself how public you make this feature\n-\t\tmy ($have_pickaxe) = gitweb_check_feature('pickaxe');\n-\t\tif (!$have_pickaxe) {\n-\t\t\tdie_error('403 Permission denied', \"Permission denied\");\n-\t\t}\n+\t\tgitweb_check_feature('pickaxe')\n+\t\t or die_error(403, \"Pickaxe is disabled\");\n \t}\n \tif ($searchtype eq 'grep') {\n-\t\tmy ($have_grep) = gitweb_check_feature('grep');\n-\t\tif (!$have_grep) {\n-\t\t\tdie_error('403 Permission denied', \"Permission denied\");\n-\t\t}\n+\t\tgitweb_check_feature('grep')\n+\t\t or die_error(403, \"Grep is disabled\");\n \t}\n \n \tgit_header_html();\n@@ -5484,7 +5481,7 @@ sub git_feed {\n \t# Atom: http://www.atomenabled.org/developers/syndication/\n \t# RSS: http://www.notestips.com/80256B3A007F2692/1/NAMO5P9UPQ\n \tif ($format ne 'rss' && $format ne 'atom') {\n-\t\tdie_error(undef, \"Unknown web feed format\");\n+\t\tdie_error(400, \"Unknown web feed format\");\n \t}\n \n \t# log/feed of current (HEAD) branch, log of given branch, history of file/directory\n-- \n1.5.6.rc3.7.ged9620\n"},{"id":"80191","messageId":"48586400.4000504@gmail.com","threadId":"13968","inReplyTo":"200806180212.15392.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: return correct HTTP status codes","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-18T01:25:20Z","receivedAt":"2008-06-18T01:25:20Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Jakub Narebski wrote:\n> Lea Wiemann wrote:\n>> $hash = get_hash($symbol, 'commit'); # 'commit' to resolve tags\n> \n> Errr... is there equivalent to ^{}, i.e. resolve to non-tag?\n\nYup. Haven't quite decided whether to simply use \"$symbol^{type}\" or \nmake type a separate parameter.\n\n> Note that you would have to examine gitweb sources to check if it\n> uses href(..., -replay=>1) when it should,\n\nGood point, will do.\n\n> BTW. one of earliest idea was to fully resolve hashes, add missing\n> parameters if possible (like 'h', 'hp', 'f') and convert hashes to\n> sha-1. One of intended uses was (weak) ETag for simple HTTP caching.\n\nInteresting. Something to keep in mind is that using name-rev still can \nwreck with this since it has the unique property of taking hashes but \nstill depending on the current refs. Gitweb isn't using name-rev a lot \nright now, but that might change of course (e.g. I think that it would \nbe convenient to always display names along with any commit hashes).\n\n> All the time I think that caching _everything_ is a bad solution.\n\nSo? We can easily add an option to the cache; e.g. no_cache => \n['get_blob', 'ls_tree']. I doubt that it will be needed, but if it \ndoes, it's easy to add it. Don't worry about it, really.\n\n> CHI (or other in recommended thread) for inobtrusive data caching\n\nThanks for the pointer! On the one hand CHI is very recent and not even \nin Debian, on the other hand it provides things like busy_lock on top of \nMemcached (AFAICS), at fairly little cost. I'll look into it.\n\n-- Lea\n"},{"id":"80208","messageId":"200806180935.54712.jnareb@gmail.com","threadId":"13968","inReplyTo":"48586400.4000504@gmail.com","subject":"Re: [PATCH] gitweb: return correct HTTP status codes","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-18T07:35:53Z","receivedAt":"2008-06-18T07:35:53Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Lea Wiemann wrote:\n> Jakub Narebski wrote:\n>> Lea Wiemann wrote:\n>>>\n>>> $hash = get_hash($symbol, 'commit'); # 'commit' to resolve tags\n>> \n>> Errr... is there equivalent to ^{}, i.e. resolve to non-tag?\n> \n> Yup. Haven't quite decided whether to simply use \"$symbol^{type}\" or \n> make type a separate parameter.\n\nAlthough I think this won't be necessary for gitweb, the ability\nshould (I guess) be in generic object-oriented interface like\nGit::Repo module.\n\n>> Note that you would have to examine gitweb sources to check if it\n>> uses href(..., -replay=>1) when it should,\n> \n> Good point, will do.\n\nIt should, but I might have missed something.\n\n>> BTW. one of earliest idea was to fully resolve hashes, add missing\n>> parameters if possible (like 'h', 'hp', 'f') and convert hashes to\n>> sha-1. One of intended uses was (weak) ETag for simple HTTP caching.\n> \n> Interesting. Something to keep in mind is that using name-rev still can \n> wreck with this since it has the unique property of taking hashes but \n> still depending on the current refs. Gitweb isn't using name-rev a lot \n> right now, but that might change of course (e.g. I think that it would \n> be convenient to always display names along with any commit hashes).\n\nWell, you will have two level cache: first from refnames[*1*] to hashes\n(by the way I think you can treat tag names as valid indifinitely,\nand use them in the place of full sha-1 hashes; thanks to heads<->tags\nambiguity gitweb now spells tags in full as refs/heads/), second\nfrom \"normalized\" URLs to content, or rather from \"normalized\" URL\nderivative to data used to generate content.\n\nSo I think you can use first-level cache to calculate equivalent\nof 'name-rev', or keep cached 'name-rev' there.\n\nCurrently gitweb uses name-rev only in (I think) rarely used 'raw'\n(text/plain) version of 'commit' and 'commitdiff' views... and I think\nit is here to stay thanks to gitk like displaying refs marks in\nlog-like views (I think result of git_get_references() should also\nbe cached...).\n\n[*1*] Well, project name (repository path) + refname.\n\n\nWe will see if this two-tier cache solution is good idea...\n\n>> All the time I think that caching _everything_ is a bad solution.\n> \n> So? We can easily add an option to the cache; e.g. no_cache => \n> ['get_blob', 'ls_tree']. I doubt that it will be needed, but if it \n> does, it's easy to add it. Don't worry about it, really.\n\nWell, on the other hand from what I understand kernel.org gitweb caches\nunconditionally (but adaptively) every page, same with CGit (used for\nexample on freedesktop.org).\n\nIt would be nice to have some stats of gitweb access (e.g. from\nrepo.or.cz and kernel.org).\n\n>> CHI (or other in recommended thread) for inobtrusive data caching\n> \n> Thanks for the pointer! On the one hand CHI is very recent and not even \n> in Debian, on the other hand it provides things like busy_lock on top of \n> Memcached (AFAICS), at fairly little cost. I'll look into it.\n\nI was not thinking about using CHI, or any Perl module metioned in\n\n \"[RFD] Gitweb caching, part 3: examining Perl modules for caching (long)\"\n http://thread.gmane.org/gmane.comp.version-control.git/77529/focus=77527\n\n(perhaps with exception of Cache::Cache and its submodules/engines), but\nrather about emulating best of its interfaces (well, perhaps also\n\"borrowing\" some of its code, if license permits).\n\n-- \nJakub Narebski\nPoland\n"},{"id":"80277","messageId":"m3y752melj.fsf_-_@localhost.localdomain","threadId":"13968","inReplyTo":"1213748115-4038-1-git-send-email-LeWiemann@gmail.com","subject":"Re: [PATCH v2] gitweb: standarize HTTP status codes","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-19T00:51:38Z","receivedAt":"2008-06-19T00:51:38Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Lea Wiemann writes:\n\n> Many error status codes simply default to 403 Forbidden, which is not\n> correct in most cases. This patch makes gitweb return semantically\n> correct status codes.\n\nO.K.\n\n> For convenience the die_error function now only takes the status code\n> without reason as first parameter (e.g. 404 instead of \"404 Not\n> Found\"), and it now defaults to 500 (Internal Server Error), even\n> though the default is not used anywhere.\n\n_Whose_ convenience?\n\nIMHO using \"bare\" status code is bad idea for the following reasons:\n\n * I don't think that RFC 2616 allows blanket replacing reason phrase\n by generic \"Error\", as I don't think that for example \"Error\" is\n 'local equivalent' of \"Not Found\" for 404 client error. And even\n if it would allow it, I don't think it is best / recommended\n practice.\n\n If the fragment cited below isn't what you mean by \"RFC 2616 allows\n this\" please point me out to the fragment that states this.\n\n > +\t# Use a generic \"Error\" reason, e.g. \"404 Error\" instead of\n > +\t# \"404 Not Found\". This is permissible by RFC 2616.\n > +\tgit_header_html(\"$status Error\");\n\n\n * Test::WWW::Mechanize displays both HTTP error status code and\n reason phrase when get_ok(...) fails:\n\n not ok 18 - GET http://localhost/?p=non-existent.git\n # Failed test 'GET http://localhost/?p=non-existent.git\n # at ../t9503/test.pl line 105.\n # 403\n # Forbidden\n\n It does not display error _page_, so it helps if reason phrase\n explains the error.\n\n\n * From the point of view of someone examinimg gitweb.perl code, 400,\n 403, 404, 500 are _magic numbers_; one would have to take a look at\n RFC 2616 (or Wikipedia) to check what given number means. It is\n much more readable to have e.g. '404 Not Found' there.\n\n\nPosible solution would be to use constants similar to the ones in\nHTTP::Status (but best without requiring this module, as it might be\nnot present), for example HTTP_OK, HTTP_INVALID, HTTP_FORBIDDEN,\nHTTP_NOT_FOUND, HTTP_SERVER_ERROR or similar (HTTP::Status uses\nRC_OK (200), RC_BAD_REQUEST (400), RC_FORBIDDEN (403),\nRC_NOT_FOUND (404), RC_INTERNAL_SERVER_ERROR (500)).\n\n..................................................................\nRelevant fragment of RFC 2616 (http://tools.ietf.org/html/rfc2616)\n\n 6.1.1 Status Code and Reason Phrase\n\n The Status-Code element is a 3-digit integer result code of the\n attempt to understand and satisfy the request. These codes are\n fully defined in section 10. The Reason-Phrase is intended to\n give a short textual description of the Status-Code. The\n Status-Code is intended for use by automata and the Reason-Phrase\n is intended for the human user. The client is not required to\n examine or display the Reason-Phrase.\n\n [...]\n\n (...) The reason phrases listed here are only recommendations --\n they MAY be replaced by local equivalents without affecting the\n protocol.\n..................................................................\n\n> Also documented status code conventions in die_error.\n\nPutting copy of those status code conventions in the commit message,\nand not only in the comment section, would be also good idea.\n\n> Signed-off-by: Lea Wiemann \n> ---\n> Changes since v1:\n\nWould be better to change subject to contain \"[PATCH v2]\", too?\n \n> - Changed subject per Jakub's suggestion.\n\nIt was just suggestion. IMVHO current version reads better, but again\nI am not a native English speaker.\n\n> - Resolved merge conflict caused by \"[PATCH v2] gitweb: quote commands\n> properly when calling the shell\".\n> - Added the following documentation to die_error (I know Jakub might\n> disagree with me on the last line ;-)):\n\nNo, I won't disagree. \"403 Forbidden\" is bad catch-all, and bad\ndefault. \"404 Not Found\" or \"500 Internal Server Error\" are much\nbetter default values.\n\n> # die_error(, )\n> # Example: die_error(404, 'Hash not found')\n\nAs I mentioned above, I'd rather have\n # Example: die_error(HTTP_NOT_FOUND, 'Hash not found')\nhere, where HTTP_NOT_FOUND is expanded to '404 Not Found'.\n\n> # By convention, use the following status codes (as defined in RFC 2616):\n> # 400: Invalid or missing CGI parameters, or\n> # requested object exists but has wrong type.\n> # 403: Requested feature (like \"pickaxe\" or \"snapshot\") not enabled on\n> # this server or project.\n> # 404: Requested object/revision/project doesn't exist.\n\nNote that all above 4xx client errors make one look at URL to check if\nit is correct in the case of errors.\n\n> # 500: The server isn't configured properly, or\n> # an internal error occurred (e.g. failed assertions caused by bugs), or\n> # an unknown error occurred (e.g. the git binary died unexpectedly).\n\nNote that all 5xx server errors make one search for webmaster /\nnetwork administrator email (or other contact info, like IM or contact\nform) to notify about error.\n \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 3a7adae..3ca45fd 100755\n[...]\n> @@ -399,21 +399,21 @@ if (defined $project) {\n> \t ($export_ok && !(-e \"$projectroot/$project/$export_ok\")) ||\n> \t ($strict_export && !project_in_list($project))) {\n> \t\tundef $project;\n> -\t\tdie_error(undef, \"No such project\");\n> +\t\tdie_error(404, \"No such project\");\n> \t}\n> }\n\nHere is one thing worth considering: if project exists, but is\nexcluded due to either $export_ok or $strict_export being set,\nshould we use '404 Not Found' or '403 Forbidden'?\n\nIt is easier to use catch-all '404 Not Found', and I think this is\nbetter for security reason, but I guess that is worth thinking about a\nbit.\n\n> @@ -483,7 +483,7 @@ our $searchtext = $cgi->param('s');\n> our $search_regexp;\n> if (defined $searchtext) {\n> \tif (length($searchtext) < 2) {\n> -\t\tdie_error(undef, \"At least two characters are required for search parameter\");\n> +\t\tdie_error(403, \"At least two characters are required for search parameter\");\n> \t}\n> \t$search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;\n> }\n\nShould gitweb use there '403 Forbidden', or '400 Bad Request'?\nThis is failing static validation of CGI parameters, not a matter of\nsome permissions...\n\n\n> @@ -1665,7 +1665,7 @@ sub git_get_hash_by_path {\n> \t$path =~ s,/+$,,;\n> \n> \topen my $fd, \"-|\", git_cmd(), \"ls-tree\", $base, \"--\", $path\n> -\t\tor die_error(undef, \"Open git-ls-tree failed\");\n> +\t\tor die_error(500, \"Open git-ls-tree failed\");\n> \tmy $line = <$fd>;\n> \tclose $fd or return undef;\n>\n\nI'm sorry for my mistake in earlier email.\n\n*All* \"open ... or die_error\" constructs should use '500 Internal\nServer Error', as the only errors that can be detected on open are\nvery serious, server related ones: cannot find git binaries, git\nbinaries doesn't have executable permissions for web server, number of\navailable filehandles exceeded, cannot fork (because of limits), etc.\n\nAll of those are result of error on the server side, not the client\nmangling URL or something like that.\n\n> +\t# Use a generic \"Error\" reason, e.g. \"404 Error\" instead of\n> +\t# \"404 Not Found\". This is permissible by RFC 2616.\n> +\tgit_header_html(\"$status Error\");\n\nSee comments above. In short: I don't think it is permissible, if it\nis permissible I don't think it is recommended practice, if it is\ncommon practice I don't think this is a good idea for gitweb.\n \n> -\tmy ($have_blame) = gitweb_check_feature('blame');\n> -\tif (!$have_blame) {\n> -\t\tdie_error('403 Permission denied', \"Permission denied\");\n> -\t}\n> -\tdie_error('404 Not Found', \"File name not defined\") if (!$file_name);\n> +\tgitweb_check_feature('blame')\n> +\t or die_error(403, \"Blame not allowed\");\n\nThat is a bit separate change, i.e. better explanation of an error\n(although I'd say rather \"Blame view not allowed\").\n\nBut what about security reasons?\n\n> -\tdie_error(undef, \"Couldn't find base commit\") unless ($hash_base);\n> +\tdie_error(400, \"Couldn't find base commit\") unless ($hash_base);\n\nWound't it be '404 Not Found', as the explanation suggests?\n\n> \tmy %co = parse_commit($hash_base)\n> -\t\tor die_error(undef, \"Reading commit failed\");\n> +\t\tor die_error(404, \"Commit not found\");\n\nGood!\n\n> \t$/ = \"\\0\";\n> \topen my $fd, \"-|\", git_cmd(), \"ls-tree\", '-z', $hash\n> -\t\tor die_error(undef, \"Open git-ls-tree failed\");\n> +\t\tor die_error(500, \"Open git-ls-tree failed\");\n\nO.K.\n\n> \tmy @entries = map { chomp; $_ } <$fd>;\n> -\tclose $fd or die_error(undef, \"Reading tree failed\");\n> +\tclose $fd or die_error(500, \"Reading tree failed\");\n> \t$/ = \"\\n\";\n\nNot O.K. Barring errors in gitweb code this might happen when object\ngiven by CGI parameters does not exist ('fatal: Not a valid object\nname foo'), project does not exist ('fatal: Not a git repository:\nproject'), or the object is not a tree-ish ('fatal: not a tree\nobject'). All those are clearly 4xx _client_ errors, and perhaps with\nthe exception of last one which could be '400 Bad Request', all are of\nthe '404 Nof Found'; but '404 Not FOund' is good also for the last\nreason, so for simplicity catch-all '404 Not Found' would be best.\n\nOne should examine URL for mistakes, not contact server admin here...\n\n> sub git_commit {\n> \t$hash ||= $hash_base || \"HEAD\";\n> -\tmy %co = parse_commit($hash);\n> -\tif (!%co) {\n> -\t\tdie_error(undef, \"Unknown commit object\");\n> -\t}\n> +\tmy %co = parse_commit($hash)\n> +\t or die_error(404, \"Unknown commit object\");\n\nGood, although if it makes code more readable or not is a matter of\ntaste.\n\n> -\tclose $fd or die_error(undef, \"Reading git-diff-tree failed\");\n> +\tclose $fd or die_error(500, \"Reading git-diff-tree failed\");\n\nAgain, this is ussually 4xx client error, and one should examine URL\nfor mistakes, not contact server admin.\n \n> \t} else {\n> -\t\tdie_error('404 Not Found', \"Not enough information to find object\");\n> +\t\tdie_error(400, \"Not enough information to find object\");\n> \t}\n\nI'm not sure if it should be '400 Bad Request' or '404 Not Found'\nhere. RFC 2616 states:\n\n 10.4.5 404 Not Found\n\n (...) This status code is commonly used when the server does not\n wish to reveal exactly why the request has been refused, or when\n no other response is applicable.\n\nso I guess '404' _might_ be better here. But this is IMVHO a matter\nof taste here, a bit.\n\n\n> \t\t\tclose $fd\n> -\t\t\t\tor die_error(undef, \"Reading git-diff-tree failed\");\n> +\t\t\t\tor die_error(500, \"Reading git-diff-tree failed\");\n\nAgain, I think it is usually 4xx client error, i.e. '404 Nof Found'\nand not '500 Internal Sever Error' should be used here.\n\n> \t\t\t@difftree\n> -\t\t\t\tor die_error('404 Not Found', \"Blob diff not found\");\n> +\t\t\t\tor die_error(404, \"Blob diff not found\");\n\nThis, if I am not mistaken, can happen only if path limiters doesn'\ncatch anything.\n \n\n> \t\t\tclose $fd\n> -\t\t\t\tor die_error(undef, \"Reading git-diff-tree failed\");\n> +\t\t\t\tor die_error(500, \"Reading git-diff-tree failed\");\n\nAnd again...\n\n> \t\t} else {\n> -\t\t\tdie_error('404 Not Found', \"Missing one of the blob diff parameters\");\n> +\t\t\tdie_error(400, \"Missing one of the blob diff parameters\");\n> \t\t}\n\nO.K. here.\n\n> \t\tif (@difftree > 1) {\n> -\t\t\tdie_error('404 Not Found', \"Ambiguous blob diff specification\");\n> +\t\t\tdie_error(400, \"Ambiguous blob diff specification\");\n> \t\t}\n\nOr perhaps '404 Not Dound' here?\n \n> \t} else {\n> -\t\tdie_error('404 Not Found', \"Missing one of the blob diff parameters\")\n> +\t\tdie_error(400, \"Missing one of the blob diff parameters\")\n> \t\t\tunless %diffinfo;\n> \t}\n\nThe \"unless %diffinfo\" makes it look more like '404 Not Found'...\n \n> \tif (!defined $ftype) {\n> -\t\tdie_error(undef, \"Unknown type of object\");\n> +\t\tdie_error(500, \"Unknown type of object\");\n> \t}\n\nErrr... shouldn't be '400 Bad Request' here, per convention?\n \n> -\t\tmy ($have_pickaxe) = gitweb_check_feature('pickaxe');\n> -\t\tif (!$have_pickaxe) {\n> -\t\t\tdie_error('403 Permission denied', \"Permission denied\");\n> -\t\t}\n> +\t\tgitweb_check_feature('pickaxe')\n> +\t\t or die_error(403, \"Pickaxe is disabled\");\n\n\"Pickaxe\" is git jargon... (just mentioning it).\n\n> @@ -5484,7 +5481,7 @@ sub git_feed {\n> \t# Atom: http://www.atomenabled.org/developers/syndication/\n> \t# RSS: http://www.notestips.com/80256B3A007F2692/1/NAMO5P9UPQ\n> \tif ($format ne 'rss' && $format ne 'atom') {\n> -\t\tdie_error(undef, \"Unknown web feed format\");\n> +\t\tdie_error(400, \"Unknown web feed format\");\n> \t}\n\nThat is a good example of unambiguously '400 Bad Request'.\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"80353","messageId":"485AAEB9.2080100@gmail.com","threadId":"13968","inReplyTo":"m3y752melj.fsf_-_@localhost.localdomain","subject":"Re: [PATCH v2] gitweb: standarize HTTP status codes","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-19T19:08:41Z","receivedAt":"2008-06-19T19:08:41Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Jakub Narebski wrote:\n> Lea Wiemann writes:\n>> For convenience the die_error function now only takes the status code\n>> without reason as first parameter (e.g. 404 instead of \"404 Not Found\")\n> \n> _Whose_ convenience?\n\nThe developer's convenience of course. It's plain redundant.\n\n> * I don't think that RFC 2616 allows blanket replacing reason phrase\n> by generic \"Error\",\n> * Test::WWW::Mechanize displays both HTTP error status code and\n> reason phrase when get_ok(...) fails:\n> * From the point of view of someone examinimg gitweb.perl code, 400,\n> 403, 404, 500 are _magic numbers_;\n\nI think we're really arguing about the color of the bikeshed here. IMO \nwe're not stretching RFC 2616 too much by putting \"Error\" there (since \nreason codes don't matter on a technical level), and the status codes \nmake enough sense to me (and I'm not even a web developer) that I'm not \nconcerned about readability.\n\nI don't think your constants a la HTTP_INVALID are a good idea (I \nremember the status codes in a year, but maybe not the constants); \ndie_error could figure out the right reason code using a hash. Either \nway will be fine with me though.\n\nLook, I really don't care about this and I think it's plain irrelevant \n-- do you mind fixing the patch with whatever solution you prefer and \nresend it? I'd rather spend my time on getting caching implemented, but \nif you insist on me changing this myself, please let me know.\n\n>> Also documented status code conventions in die_error.\n> \n> Putting copy of those status code conventions in the commit message,\n> and not only in the comment section, would be also good idea.\n\n*shrugs* I'd rather not duplicate, and it's in the code anyway.\n\n>> -\t\tdie_error(undef, \"No such project\");\n>> +\t\tdie_error(404, \"No such project\");\n> \n> Here is one thing worth considering: if project exists, but is\n> excluded due to either $export_ok or $strict_export being set,\n> should we use '404 Not Found' or '403 Forbidden'?\n\nNo, because (a) we'd have add code to check this (so it should be in a \nseparate patch), and (b) I don't think we even want to return 403, since \n(as you say) it would reveal the existence of a project on the server. \nProject names can contain sensitive/confidential information though.\n\n>> -\t\tdie_error(undef, \"At least two characters are required for search parameter\");\n>> +\t\tdie_error(403, \"At least two characters are required for search parameter\");\n> \n> Should gitweb use there '403 Forbidden', or '400 Bad Request'?\n> This is failing static validation of CGI parameters, not a matter of\n> some permissions...\n\nI used 403 in the sense of \"sorry, we don't have shorter search strings \nactivated for performance reasons\". The '2' could even become \nconfigurable. 400 is fine too, though, I don't care.\n\n> *All* \"open ... or die_error\" constructs should use '500 Internal\n> Server Error', as the only errors that can be detected on open are\n> very serious, server related ones:\n\nRight, thanks for pointing this out.\n\n>> -\tmy ($have_blame) = gitweb_check_feature('blame');\n>> -\tif (!$have_blame) {\n>> -\t\tdie_error('403 Permission denied', \"Permission denied\");\n>> -\t}\n>> +\tgitweb_check_feature('blame')\n>> +\t or die_error(403, \"Blame not allowed\");\n> \n> That is a bit separate change, i.e. better explanation of an error\n> (although I'd say rather \"Blame view not allowed\").\n> \n> But what about security reasons?\n\nSeparate change: I don't think this has to be a separate patch. ;-)\n\"Blame view not allowed\": Fine with me.\nSecurity concerns: I don't see any, and anyway you can probably infer \nfrom getting 403 on a=blame that it's disabled.\n\n>> -\tdie_error(undef, \"Couldn't find base commit\") unless ($hash_base);\n>> +\tdie_error(400, \"Couldn't find base commit\") unless ($hash_base);\n> \n> Wound't it be '404 Not Found', as the explanation suggests?\n\nYup, right.\n\n>> -\tclose $fd or die_error(undef, \"Reading tree failed\");\n>> +\tclose $fd or die_error(500, \"Reading tree failed\");\n> \n> Not O.K. Barring errors in gitweb code this might happen when\n> [X Y Z]. All those are clearly 4xx _client_ errors,\n\nI haven't verified that, so until we have better error handling I prefer \n500, but I really won't bother objecting to 404. (Same goes for all \nother occurrences of 500 you quoted, which I've snipped.) FWIW I'm \nassuming that once gitweb uses the new API, that error handling code \nwill go away anyway.\n\n> One should examine URL for mistakes, not contact server admin here...\n\nI don't think that'll be a practical concern. ;-)\n\n>> -\t\tdie_error('404 Not Found', \"Not enough information to find object\");\n>> +\t\tdie_error(400, \"Not enough information to find object\");\n> \n> I'm not sure if it should be '400 Bad Request' or '404 Not Found' here.\n\nIt's about missing CGI parameters, so 400 should be fine.\n\n>> \t\t\t@difftree\n>> -\t\t\t\tor die_error('404 Not Found', \"Blob diff not found\");\n>> +\t\t\t\tor die_error(404, \"Blob diff not found\");\n> \n> This, if I am not mistaken, can happen only if path limiters doesn'\n> catch anything.\n\nYour sentence doesn't quite parse -- why is 404 wrong?\n\n>> -\t\t\tdie_error('404 Not Found', \"Ambiguous blob diff specification\");\n>> +\t\t\tdie_error(400, \"Ambiguous blob diff specification\");\n> \n> Or perhaps '404 Not Dound' here?\n\nEither way is fine.\n\n>> -\t\tdie_error('404 Not Found', \"Missing one of the blob diff parameters\")\n>> +\t\tdie_error(400, \"Missing one of the blob diff parameters\")\n>> \t\t\tunless %diffinfo;\n> \n> The \"unless %diffinfo\" makes it look more like '404 Not Found'...\n\nLooking at the preceding code (the if statement) I believe diffinfo \nbeing false doesn't indicate 404 but actually a missing CGI parameter \n(as the error message states).\n\n>> \tif (!defined $ftype) {\n>> -\t\tdie_error(undef, \"Unknown type of object\");\n>> +\t\tdie_error(500, \"Unknown type of object\");\n> \n> Errr... shouldn't be '400 Bad Request' here, per convention?\n\nNope, we didn't get *anything* back, so something weird happened. 500.\n\n-- Lea\n"},{"id":"80360","messageId":"1213905801-2811-1-git-send-email-LeWiemann@gmail.com","threadId":"13968","inReplyTo":"485AAEB9.2080100@gmail.com","subject":"[PATCH v3] gitweb: standarize HTTP status codes","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-19T20:03:21Z","receivedAt":"2008-06-19T20:03:21Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Many error status codes simply default to 403 Forbidden, which is not\ncorrect in most cases. This patch makes gitweb return semantically\ncorrect status codes.\n\nFor convenience the die_error function now only takes the status code\nwithout reason as first parameter (e.g. 404 instead of \"404 Not\nFound\"), and it now defaults to 500 (Internal Server Error), even\nthough the default is not used anywhere.\n\nAlso documented status code conventions in die_error.\n\nSigned-off-by: Lea Wiemann \n---\n\nChanges since v2: die_error now adds the reason strings defined by RFC\n2616 to the HTTP status code; incorporated Jakub's other suggestions.\nDiff to v2 follows.\n\nI didn't use the HTTP_NOT_FOUND etc. suggestion because I found it too\nverbose and obtrusive.\n\nJust a friendly reminder, please remember that discussing fairly\ntrivial changes in-depth might be not a good use of all participants'\ntime; IOW, sending 340 lines of email about HTTP status codes that\ndon't *really* matter is probably overkill. (In case you're\ninterested, the reason why I wrote this patch in the first place is in\norder to be able to tell unforeseen errors from expected errors in the\ntest suite.) I recommend you read http://bikeshed.com/ if you haven't\ndone so.\n\nAlso, increasing the time between patch submission and acceptance\ncauses added overhead (effort) by itself (i.e. *in addition* to the\ntime spend on discussing and resending patches); that's because you\ncannot base anything on the code without risking merge conflicts.\nPlease think of that when replying to patches. ;-)\n\nBy the way, when you want to make trivial changes, it may be easier\nfor everyone if you simply implement them and send a follow-up patch\n(and it might actually be just as quick as suggesting the changes!).\nIn your follow-up patch you could provide a diff to the previous\nversion, and comment on the changes you made (or send it without\ncomments, even -- oftentimes they're obvious).\n\nAnyways, I hope everyone is happy with this version of the patch.\n\nDiff to v2:\n\nindex 3ca45fd..3bc8f69 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -2687,9 +2687,11 @@ sub die_error {\n \tmy $status = shift || 500;\n \tmy $error = shift || \"Internal server error\";\n \n-\t# Use a generic \"Error\" reason, e.g. \"404 Error\" instead of\n-\t# \"404 Not Found\". This is permissible by RFC 2616.\n-\tgit_header_html(\"$status Error\");\n+\tmy %http_responses = (400 => '400 Bad Request',\n+\t\t\t 403 => '403 Forbidden',\n+\t\t\t 404 => '404 Not Found',\n+\t\t\t 500 => '500 Internal Server Error');\n+\tgit_header_html($http_responses{$status});\n \tprint <\n

\n@@ -4131,11 +4133,11 @@ sub git_blame {\n \tmy $ftype;\n \n \tgitweb_check_feature('blame')\n-\t or die_error(403, \"Blame not allowed\");\n+\t or die_error(403, \"Blame view not allowed\");\n \n \tdie_error(400, \"No file name given\") unless $file_name;\n \t$hash_base ||= git_get_head_hash($project);\n-\tdie_error(400, \"Couldn't find base commit\") unless ($hash_base);\n+\tdie_error(404, \"Couldn't find base commit\") unless ($hash_base);\n \tmy %co = parse_commit($hash_base)\n \t\tor die_error(404, \"Commit not found\");\n \tif (!defined $hash) {\n@@ -4403,7 +4405,7 @@ sub git_tree {\n \topen my $fd, \"-|\", git_cmd(), \"ls-tree\", '-z', $hash\n \t\tor die_error(500, \"Open git-ls-tree failed\");\n \tmy @entries = map { chomp; $_ } <$fd>;\n-\tclose $fd or die_error(500, \"Reading tree failed\");\n+\tclose $fd or die_error(404, \"Reading tree failed\");\n \t$/ = \"\\n\";\n \n \tmy $refs = git_get_references();\n@@ -4641,7 +4643,7 @@ sub git_commit {\n \t\t$hash, \"--\"\n \t\tor die_error(500, \"Open git-diff-tree failed\");\n \t@difftree = map { chomp; $_ } <$fd>;\n-\tclose $fd or die_error(500, \"Reading git-diff-tree failed\");\n+\tclose $fd or die_error(404, \"Reading git-diff-tree failed\");\n \n \t# non-textual hash id's can be cached\n \tmy $expires;\n@@ -4788,7 +4790,7 @@ sub git_blobdiff {\n \t\t\t\tor die_error(500, \"Open git-diff-tree failed\");\n \t\t\t@difftree = map { chomp; $_ } <$fd>;\n \t\t\tclose $fd\n-\t\t\t\tor die_error(500, \"Reading git-diff-tree failed\");\n+\t\t\t\tor die_error(404, \"Reading git-diff-tree failed\");\n \t\t\t@difftree\n \t\t\t\tor die_error(404, \"Blob diff not found\");\n \n@@ -4806,7 +4808,7 @@ sub git_blobdiff {\n \t\t\t\tgrep { /^:[0-7]{6} [0-7]{6} [0-9a-fA-F]{40} $hash/ }\n \t\t\t\tmap { chomp; $_ } <$fd>;\n \t\t\tclose $fd\n-\t\t\t\tor die_error(500, \"Reading git-diff-tree failed\");\n+\t\t\t\tor die_error(404, \"Reading git-diff-tree failed\");\n \t\t\t@difftree\n \t\t\t\tor die_error(404, \"Blob diff not found\");\n \n\n\n\n\n\n\n\n gitweb/gitweb.perl | 211 ++++++++++++++++++++++++++--------------------------\n 1 files changed, 105 insertions(+), 106 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 3a7adae..3bc8f69 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -386,7 +386,7 @@ $projects_list ||= $projectroot;\n our $action = $cgi->param('a');\n if (defined $action) {\n \tif ($action =~ m/[^0-9a-zA-Z\\.\\-_]/) {\n-\t\tdie_error(undef, \"Invalid action parameter\");\n+\t\tdie_error(400, \"Invalid action parameter\");\n \t}\n }\n \n@@ -399,21 +399,21 @@ if (defined $project) {\n \t ($export_ok && !(-e \"$projectroot/$project/$export_ok\")) ||\n \t ($strict_export && !project_in_list($project))) {\n \t\tundef $project;\n-\t\tdie_error(undef, \"No such project\");\n+\t\tdie_error(404, \"No such project\");\n \t}\n }\n \n our $file_name = $cgi->param('f');\n if (defined $file_name) {\n \tif (!validate_pathname($file_name)) {\n-\t\tdie_error(undef, \"Invalid file parameter\");\n+\t\tdie_error(400, \"Invalid file parameter\");\n \t}\n }\n \n our $file_parent = $cgi->param('fp');\n if (defined $file_parent) {\n \tif (!validate_pathname($file_parent)) {\n-\t\tdie_error(undef, \"Invalid file parent parameter\");\n+\t\tdie_error(400, \"Invalid file parent parameter\");\n \t}\n }\n \n@@ -421,21 +421,21 @@ if (defined $file_parent) {\n our $hash = $cgi->param('h');\n if (defined $hash) {\n \tif (!validate_refname($hash)) {\n-\t\tdie_error(undef, \"Invalid hash parameter\");\n+\t\tdie_error(400, \"Invalid hash parameter\");\n \t}\n }\n \n our $hash_parent = $cgi->param('hp');\n if (defined $hash_parent) {\n \tif (!validate_refname($hash_parent)) {\n-\t\tdie_error(undef, \"Invalid hash parent parameter\");\n+\t\tdie_error(400, \"Invalid hash parent parameter\");\n \t}\n }\n \n our $hash_base = $cgi->param('hb');\n if (defined $hash_base) {\n \tif (!validate_refname($hash_base)) {\n-\t\tdie_error(undef, \"Invalid hash base parameter\");\n+\t\tdie_error(400, \"Invalid hash base parameter\");\n \t}\n }\n \n@@ -447,10 +447,10 @@ our @extra_options = $cgi->param('opt');\n if (defined @extra_options) {\n \tforeach my $opt (@extra_options) {\n \t\tif (not exists $allowed_options{$opt}) {\n-\t\t\tdie_error(undef, \"Invalid option parameter\");\n+\t\t\tdie_error(400, \"Invalid option parameter\");\n \t\t}\n \t\tif (not grep(/^$action$/, @{$allowed_options{$opt}})) {\n-\t\t\tdie_error(undef, \"Invalid option parameter for this action\");\n+\t\t\tdie_error(400, \"Invalid option parameter for this action\");\n \t\t}\n \t}\n }\n@@ -458,7 +458,7 @@ if (defined @extra_options) {\n our $hash_parent_base = $cgi->param('hpb');\n if (defined $hash_parent_base) {\n \tif (!validate_refname($hash_parent_base)) {\n-\t\tdie_error(undef, \"Invalid hash parent base parameter\");\n+\t\tdie_error(400, \"Invalid hash parent base parameter\");\n \t}\n }\n \n@@ -466,14 +466,14 @@ if (defined $hash_parent_base) {\n our $page = $cgi->param('pg');\n if (defined $page) {\n \tif ($page =~ m/[^0-9]/) {\n-\t\tdie_error(undef, \"Invalid page parameter\");\n+\t\tdie_error(400, \"Invalid page parameter\");\n \t}\n }\n \n our $searchtype = $cgi->param('st');\n if (defined $searchtype) {\n \tif ($searchtype =~ m/[^a-z]/) {\n-\t\tdie_error(undef, \"Invalid searchtype parameter\");\n+\t\tdie_error(400, \"Invalid searchtype parameter\");\n \t}\n }\n \n@@ -483,7 +483,7 @@ our $searchtext = $cgi->param('s');\n our $search_regexp;\n if (defined $searchtext) {\n \tif (length($searchtext) < 2) {\n-\t\tdie_error(undef, \"At least two characters are required for search parameter\");\n+\t\tdie_error(403, \"At least two characters are required for search parameter\");\n \t}\n \t$search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;\n }\n@@ -580,11 +580,11 @@ if (!defined $action) {\n \t}\n }\n if (!defined($actions{$action})) {\n-\tdie_error(undef, \"Unknown action\");\n+\tdie_error(400, \"Unknown action\");\n }\n if ($action !~ m/^(opml|project_list|project_index)$/ &&\n !$project) {\n-\tdie_error(undef, \"Project needed\");\n+\tdie_error(400, \"Project needed\");\n }\n $actions{$action}->();\n exit;\n@@ -1665,7 +1665,7 @@ sub git_get_hash_by_path {\n \t$path =~ s,/+$,,;\n \n \topen my $fd, \"-|\", git_cmd(), \"ls-tree\", $base, \"--\", $path\n-\t\tor die_error(undef, \"Open git-ls-tree failed\");\n+\t\tor die_error(500, \"Open git-ls-tree failed\");\n \tmy $line = <$fd>;\n \tclose $fd or return undef;\n \n@@ -2127,7 +2127,7 @@ sub parse_commit {\n \t\t\"--max-count=1\",\n \t\t$commit_id,\n \t\t\"--\",\n-\t\tor die_error(undef, \"Open git-rev-list failed\");\n+\t\tor die_error(500, \"Open git-rev-list failed\");\n \t%co = parse_commit_text(<$fd>, 1);\n \tclose $fd;\n \n@@ -2152,7 +2152,7 @@ sub parse_commits {\n \t\t$commit_id,\n \t\t\"--\",\n \t\t($filename ? ($filename) : ())\n-\t\tor die_error(undef, \"Open git-rev-list failed\");\n+\t\tor die_error(500, \"Open git-rev-list failed\");\n \twhile (my $line = <$fd>) {\n \t\tmy %co = parse_commit_text($line);\n \t\tpush @cos, \\%co;\n@@ -2672,11 +2672,26 @@ sub git_footer_html {\n \t \"\";\n }\n \n+# die_error(, )\n+# Example: die_error(404, 'Hash not found')\n+# By convention, use the following status codes (as defined in RFC 2616):\n+# 400: Invalid or missing CGI parameters, or\n+# requested object exists but has wrong type.\n+# 403: Requested feature (like \"pickaxe\" or \"snapshot\") not enabled on\n+# this server or project.\n+# 404: Requested object/revision/project doesn't exist.\n+# 500: The server isn't configured properly, or\n+# an internal error occurred (e.g. failed assertions caused by bugs), or\n+# an unknown error occurred (e.g. the git binary died unexpectedly).\n sub die_error {\n-\tmy $status = shift || \"403 Forbidden\";\n-\tmy $error = shift || \"Malformed query, file missing or permission denied\";\n-\n-\tgit_header_html($status);\n+\tmy $status = shift || 500;\n+\tmy $error = shift || \"Internal server error\";\n+\n+\tmy %http_responses = (400 => '400 Bad Request',\n+\t\t\t 403 => '403 Forbidden',\n+\t\t\t 404 => '404 Not Found',\n+\t\t\t 500 => '500 Internal Server Error');\n+\tgit_header_html($http_responses{$status});\n \tprint <\n

\n@@ -3924,12 +3939,12 @@ sub git_search_grep_body {\n sub git_project_list {\n \tmy $order = $cgi->param('o');\n \tif (defined $order && $order !~ m/none|project|descr|owner|age/) {\n-\t\tdie_error(undef, \"Unknown order parameter\");\n+\t\tdie_error(400, \"Unknown order parameter\");\n \t}\n \n \tmy @list = git_get_projects_list();\n \tif (!@list) {\n-\t\tdie_error(undef, \"No projects found\");\n+\t\tdie_error(404, \"No projects found\");\n \t}\n \n \tgit_header_html();\n@@ -3947,12 +3962,12 @@ sub git_project_list {\n sub git_forks {\n \tmy $order = $cgi->param('o');\n \tif (defined $order && $order !~ m/none|project|descr|owner|age/) {\n-\t\tdie_error(undef, \"Unknown order parameter\");\n+\t\tdie_error(400, \"Unknown order parameter\");\n \t}\n \n \tmy @list = git_get_projects_list($project);\n \tif (!@list) {\n-\t\tdie_error(undef, \"No forks found\");\n+\t\tdie_error(404, \"No forks found\");\n \t}\n \n \tgit_header_html();\n@@ -4081,7 +4096,7 @@ sub git_tag {\n \tmy %tag = parse_tag($hash);\n \n \tif (! %tag) {\n-\t\tdie_error(undef, \"Unknown tag object\");\n+\t\tdie_error(404, \"Unknown tag object\");\n \t}\n \n \tgit_print_header_div('commit', esc_html($tag{'name'}), $hash);\n@@ -4117,26 +4132,25 @@ sub git_blame {\n \tmy $fd;\n \tmy $ftype;\n \n-\tmy ($have_blame) = gitweb_check_feature('blame');\n-\tif (!$have_blame) {\n-\t\tdie_error('403 Permission denied', \"Permission denied\");\n-\t}\n-\tdie_error('404 Not Found', \"File name not defined\") if (!$file_name);\n+\tgitweb_check_feature('blame')\n+\t or die_error(403, \"Blame view not allowed\");\n+\n+\tdie_error(400, \"No file name given\") unless $file_name;\n \t$hash_base ||= git_get_head_hash($project);\n-\tdie_error(undef, \"Couldn't find base commit\") unless ($hash_base);\n+\tdie_error(404, \"Couldn't find base commit\") unless ($hash_base);\n \tmy %co = parse_commit($hash_base)\n-\t\tor die_error(undef, \"Reading commit failed\");\n+\t\tor die_error(404, \"Commit not found\");\n \tif (!defined $hash) {\n \t\t$hash = git_get_hash_by_path($hash_base, $file_name, \"blob\")\n-\t\t\tor die_error(undef, \"Error looking up file\");\n+\t\t\tor die_error(404, \"Error looking up file\");\n \t}\n \t$ftype = git_get_type($hash);\n \tif ($ftype !~ \"blob\") {\n-\t\tdie_error('400 Bad Request', \"Object is not a blob\");\n+\t\tdie_error(400, \"Object is not a blob\");\n \t}\n \topen ($fd, \"-|\", git_cmd(), \"blame\", '-p', '--',\n \t $file_name, $hash_base)\n-\t\tor die_error(undef, \"Open git-blame failed\");\n+\t\tor die_error(500, \"Open git-blame failed\");\n \tgit_header_html();\n \tmy $formats_nav =\n \t\t$cgi->a({-href => href(action=>\"blob\", -replay=>1)},\n@@ -4198,7 +4212,7 @@ HTML\n \t\t\tprint \"\\n\";\n \t\t}\n \t\topen (my $dd, \"-|\", git_cmd(), \"rev-parse\", \"$full_rev^\")\n-\t\t\tor die_error(undef, \"Open git-rev-parse failed\");\n+\t\t\tor die_error(500, \"Open git-rev-parse failed\");\n \t\tmy $parent_commit = <$dd>;\n \t\tclose $dd;\n \t\tchomp($parent_commit);\n@@ -4255,9 +4269,9 @@ sub git_blob_plain {\n \t\tif (defined $file_name) {\n \t\t\tmy $base = $hash_base || git_get_head_hash($project);\n \t\t\t$hash = git_get_hash_by_path($base, $file_name, \"blob\")\n-\t\t\t\tor die_error(undef, \"Error lookup file\");\n+\t\t\t\tor die_error(404, \"Cannot find file\");\n \t\t} else {\n-\t\t\tdie_error(undef, \"No file name defined\");\n+\t\t\tdie_error(400, \"No file name defined\");\n \t\t}\n \t} elsif ($hash =~ m/^[0-9a-fA-F]{40}$/) {\n \t\t# blobs defined by non-textual hash id's can be cached\n@@ -4265,7 +4279,7 @@ sub git_blob_plain {\n \t}\n \n \topen my $fd, \"-|\", git_cmd(), \"cat-file\", \"blob\", $hash\n-\t\tor die_error(undef, \"Open git-cat-file blob '$hash' failed\");\n+\t\tor die_error(500, \"Open git-cat-file blob '$hash' failed\");\n \n \t# content-type (can include charset)\n \t$type = blob_contenttype($fd, $file_name, $type);\n@@ -4297,9 +4311,9 @@ sub git_blob {\n \t\tif (defined $file_name) {\n \t\t\tmy $base = $hash_base || git_get_head_hash($project);\n \t\t\t$hash = git_get_hash_by_path($base, $file_name, \"blob\")\n-\t\t\t\tor die_error(undef, \"Error lookup file\");\n+\t\t\t\tor die_error(404, \"Cannot find file\");\n \t\t} else {\n-\t\t\tdie_error(undef, \"No file name defined\");\n+\t\t\tdie_error(400, \"No file name defined\");\n \t\t}\n \t} elsif ($hash =~ m/^[0-9a-fA-F]{40}$/) {\n \t\t# blobs defined by non-textual hash id's can be cached\n@@ -4308,7 +4322,7 @@ sub git_blob {\n \n \tmy ($have_blame) = gitweb_check_feature('blame');\n \topen my $fd, \"-|\", git_cmd(), \"cat-file\", \"blob\", $hash\n-\t\tor die_error(undef, \"Couldn't cat $file_name, $hash\");\n+\t\tor die_error(500, \"Couldn't cat $file_name, $hash\");\n \tmy $mimetype = blob_mimetype($fd, $file_name);\n \tif ($mimetype !~ m!^(?:text/|image/(?:gif|png|jpeg)$)! && -B $fd) {\n \t\tclose $fd;\n@@ -4389,9 +4403,9 @@ sub git_tree {\n \t}\n \t$/ = \"\\0\";\n \topen my $fd, \"-|\", git_cmd(), \"ls-tree\", '-z', $hash\n-\t\tor die_error(undef, \"Open git-ls-tree failed\");\n+\t\tor die_error(500, \"Open git-ls-tree failed\");\n \tmy @entries = map { chomp; $_ } <$fd>;\n-\tclose $fd or die_error(undef, \"Reading tree failed\");\n+\tclose $fd or die_error(404, \"Reading tree failed\");\n \t$/ = \"\\n\";\n \n \tmy $refs = git_get_references();\n@@ -4481,16 +4495,16 @@ sub git_snapshot {\n \n \tmy $format = $cgi->param('sf');\n \tif (!@supported_fmts) {\n-\t\tdie_error('403 Permission denied', \"Permission denied\");\n+\t\tdie_error(403, \"Snapshots not allowed\");\n \t}\n \t# default to first supported snapshot format\n \t$format ||= $supported_fmts[0];\n \tif ($format !~ m/^[a-z0-9]+$/) {\n-\t\tdie_error(undef, \"Invalid snapshot format parameter\");\n+\t\tdie_error(400, \"Invalid snapshot format parameter\");\n \t} elsif (!exists($known_snapshot_formats{$format})) {\n-\t\tdie_error(undef, \"Unknown snapshot format\");\n+\t\tdie_error(400, \"Unknown snapshot format\");\n \t} elsif (!grep($_ eq $format, @supported_fmts)) {\n-\t\tdie_error(undef, \"Unsupported snapshot format\");\n+\t\tdie_error(403, \"Unsupported snapshot format\");\n \t}\n \n \tif (!defined $hash) {\n@@ -4518,7 +4532,7 @@ sub git_snapshot {\n \t\t-status => '200 OK');\n \n \topen my $fd, \"-|\", $cmd\n-\t\tor die_error(undef, \"Execute git-archive failed\");\n+\t\tor die_error(500, \"Execute git-archive failed\");\n \tbinmode STDOUT, ':raw';\n \tprint <$fd>;\n \tbinmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi\n@@ -4586,10 +4600,8 @@ sub git_log {\n \n sub git_commit {\n \t$hash ||= $hash_base || \"HEAD\";\n-\tmy %co = parse_commit($hash);\n-\tif (!%co) {\n-\t\tdie_error(undef, \"Unknown commit object\");\n-\t}\n+\tmy %co = parse_commit($hash)\n+\t or die_error(404, \"Unknown commit object\");\n \tmy %ad = parse_date($co{'author_epoch'}, $co{'author_tz'});\n \tmy %cd = parse_date($co{'committer_epoch'}, $co{'committer_tz'});\n \n@@ -4629,9 +4641,9 @@ sub git_commit {\n \t\t@diff_opts,\n \t\t(@$parents <= 1 ? $parent : '-c'),\n \t\t$hash, \"--\"\n-\t\tor die_error(undef, \"Open git-diff-tree failed\");\n+\t\tor die_error(500, \"Open git-diff-tree failed\");\n \t@difftree = map { chomp; $_ } <$fd>;\n-\tclose $fd or die_error(undef, \"Reading git-diff-tree failed\");\n+\tclose $fd or die_error(404, \"Reading git-diff-tree failed\");\n \n \t# non-textual hash id's can be cached\n \tmy $expires;\n@@ -4724,33 +4736,33 @@ sub git_object {\n \n \t\topen my $fd, \"-|\", quote_command(\n \t\t\tgit_cmd(), 'cat-file', '-t', $object_id) . ' 2> /dev/null'\n-\t\t\tor die_error('404 Not Found', \"Object does not exist\");\n+\t\t\tor die_error(404, \"Object does not exist\");\n \t\t$type = <$fd>;\n \t\tchomp $type;\n \t\tclose $fd\n-\t\t\tor die_error('404 Not Found', \"Object does not exist\");\n+\t\t\tor die_error(404, \"Object does not exist\");\n \n \t# - hash_base and file_name\n \t} elsif ($hash_base && defined $file_name) {\n \t\t$file_name =~ s,/+$,,;\n \n \t\tsystem(git_cmd(), \"cat-file\", '-e', $hash_base) == 0\n-\t\t\tor die_error('404 Not Found', \"Base object does not exist\");\n+\t\t\tor die_error(404, \"Base object does not exist\");\n \n \t\t# here errors should not hapen\n \t\topen my $fd, \"-|\", git_cmd(), \"ls-tree\", $hash_base, \"--\", $file_name\n-\t\t\tor die_error(undef, \"Open git-ls-tree failed\");\n+\t\t\tor die_error(500, \"Open git-ls-tree failed\");\n \t\tmy $line = <$fd>;\n \t\tclose $fd;\n \n \t\t#'100644 blob 0fa3f3a66fb6a137f6ec2c19351ed4d807070ffa\tpanic.c'\n \t\tunless ($line && $line =~ m/^([0-9]+) (.+) ([0-9a-fA-F]{40})\\t/) {\n-\t\t\tdie_error('404 Not Found', \"File or directory for given base does not exist\");\n+\t\t\tdie_error(404, \"File or directory for given base does not exist\");\n \t\t}\n \t\t$type = $2;\n \t\t$hash = $3;\n \t} else {\n-\t\tdie_error('404 Not Found', \"Not enough information to find object\");\n+\t\tdie_error(400, \"Not enough information to find object\");\n \t}\n \n \tprint $cgi->redirect(-uri => href(action=>$type, -full=>1,\n@@ -4775,12 +4787,12 @@ sub git_blobdiff {\n \t\t\topen $fd, \"-|\", git_cmd(), \"diff-tree\", '-r', @diff_opts,\n \t\t\t\t$hash_parent_base, $hash_base,\n \t\t\t\t\"--\", (defined $file_parent ? $file_parent : ()), $file_name\n-\t\t\t\tor die_error(undef, \"Open git-diff-tree failed\");\n+\t\t\t\tor die_error(500, \"Open git-diff-tree failed\");\n \t\t\t@difftree = map { chomp; $_ } <$fd>;\n \t\t\tclose $fd\n-\t\t\t\tor die_error(undef, \"Reading git-diff-tree failed\");\n+\t\t\t\tor die_error(404, \"Reading git-diff-tree failed\");\n \t\t\t@difftree\n-\t\t\t\tor die_error('404 Not Found', \"Blob diff not found\");\n+\t\t\t\tor die_error(404, \"Blob diff not found\");\n \n \t\t} elsif (defined $hash &&\n \t\t $hash =~ /[0-9a-fA-F]{40}/) {\n@@ -4789,23 +4801,23 @@ sub git_blobdiff {\n \t\t\t# read filtered raw output\n \t\t\topen $fd, \"-|\", git_cmd(), \"diff-tree\", '-r', @diff_opts,\n \t\t\t\t$hash_parent_base, $hash_base, \"--\"\n-\t\t\t\tor die_error(undef, \"Open git-diff-tree failed\");\n+\t\t\t\tor die_error(500, \"Open git-diff-tree failed\");\n \t\t\t@difftree =\n \t\t\t\t# ':100644 100644 03b21826... 3b93d5e7... M\tls-files.c'\n \t\t\t\t# $hash == to_id\n \t\t\t\tgrep { /^:[0-7]{6} [0-7]{6} [0-9a-fA-F]{40} $hash/ }\n \t\t\t\tmap { chomp; $_ } <$fd>;\n \t\t\tclose $fd\n-\t\t\t\tor die_error(undef, \"Reading git-diff-tree failed\");\n+\t\t\t\tor die_error(404, \"Reading git-diff-tree failed\");\n \t\t\t@difftree\n-\t\t\t\tor die_error('404 Not Found', \"Blob diff not found\");\n+\t\t\t\tor die_error(404, \"Blob diff not found\");\n \n \t\t} else {\n-\t\t\tdie_error('404 Not Found', \"Missing one of the blob diff parameters\");\n+\t\t\tdie_error(400, \"Missing one of the blob diff parameters\");\n \t\t}\n \n \t\tif (@difftree > 1) {\n-\t\t\tdie_error('404 Not Found', \"Ambiguous blob diff specification\");\n+\t\t\tdie_error(400, \"Ambiguous blob diff specification\");\n \t\t}\n \n \t\t%diffinfo = parse_difftree_raw_line($difftree[0]);\n@@ -4826,7 +4838,7 @@ sub git_blobdiff {\n \t\t\t'-p', ($format eq 'html' ? \"--full-index\" : ()),\n \t\t\t$hash_parent_base, $hash_base,\n \t\t\t\"--\", (defined $file_parent ? $file_parent : ()), $file_name\n-\t\t\tor die_error(undef, \"Open git-diff-tree failed\");\n+\t\t\tor die_error(500, \"Open git-diff-tree failed\");\n \t}\n \n \t# old/legacy style URI\n@@ -4862,9 +4874,9 @@ sub git_blobdiff {\n \t\topen $fd, \"-|\", git_cmd(), \"diff\", @diff_opts,\n \t\t\t'-p', ($format eq 'html' ? \"--full-index\" : ()),\n \t\t\t$hash_parent, $hash, \"--\"\n-\t\t\tor die_error(undef, \"Open git-diff failed\");\n+\t\t\tor die_error(500, \"Open git-diff failed\");\n \t} else {\n-\t\tdie_error('404 Not Found', \"Missing one of the blob diff parameters\")\n+\t\tdie_error(400, \"Missing one of the blob diff parameters\")\n \t\t\tunless %diffinfo;\n \t}\n \n@@ -4897,7 +4909,7 @@ sub git_blobdiff {\n \t\tprint \"X-Git-Url: \" . $cgi->self_url() . \"\\n\\n\";\n \n \t} else {\n-\t\tdie_error(undef, \"Unknown blobdiff format\");\n+\t\tdie_error(400, \"Unknown blobdiff format\");\n \t}\n \n \t# patch\n@@ -4932,10 +4944,8 @@ sub git_blobdiff_plain {\n sub git_commitdiff {\n \tmy $format = shift || 'html';\n \t$hash ||= $hash_base || \"HEAD\";\n-\tmy %co = parse_commit($hash);\n-\tif (!%co) {\n-\t\tdie_error(undef, \"Unknown commit object\");\n-\t}\n+\tmy %co = parse_commit($hash)\n+\t or die_error(404, \"Unknown commit object\");\n \n \t# choose format for commitdiff for merge\n \tif (! defined $hash_parent && @{$co{'parents'}} > 1) {\n@@ -5017,7 +5027,7 @@ sub git_commitdiff {\n \t\topen $fd, \"-|\", git_cmd(), \"diff-tree\", '-r', @diff_opts,\n \t\t\t\"--no-commit-id\", \"--patch-with-raw\", \"--full-index\",\n \t\t\t$hash_parent_param, $hash, \"--\"\n-\t\t\tor die_error(undef, \"Open git-diff-tree failed\");\n+\t\t\tor die_error(500, \"Open git-diff-tree failed\");\n \n \t\twhile (my $line = <$fd>) {\n \t\t\tchomp $line;\n@@ -5029,10 +5039,10 @@ sub git_commitdiff {\n \t} elsif ($format eq 'plain') {\n \t\topen $fd, \"-|\", git_cmd(), \"diff-tree\", '-r', @diff_opts,\n \t\t\t'-p', $hash_parent_param, $hash, \"--\"\n-\t\t\tor die_error(undef, \"Open git-diff-tree failed\");\n+\t\t\tor die_error(500, \"Open git-diff-tree failed\");\n \n \t} else {\n-\t\tdie_error(undef, \"Unknown commitdiff format\");\n+\t\tdie_error(400, \"Unknown commitdiff format\");\n \t}\n \n \t# non-textual hash id's can be cached\n@@ -5115,19 +5125,15 @@ sub git_history {\n \t\t$page = 0;\n \t}\n \tmy $ftype;\n-\tmy %co = parse_commit($hash_base);\n-\tif (!%co) {\n-\t\tdie_error(undef, \"Unknown commit object\");\n-\t}\n+\tmy %co = parse_commit($hash_base)\n+\t or die_error(404, \"Unknown commit object\");\n \n \tmy $refs = git_get_references();\n \tmy $limit = sprintf(\"--max-count=%i\", (100 * ($page+1)));\n \n \tmy @commitlist = parse_commits($hash_base, 101, (100 * $page),\n-\t $file_name, \"--full-history\");\n-\tif (!@commitlist) {\n-\t\tdie_error('404 Not Found', \"No such file or directory on given branch\");\n-\t}\n+\t $file_name, \"--full-history\")\n+\t or die_error(404, \"No such file or directory on given branch\");\n \n \tif (!defined $hash && defined $file_name) {\n \t\t# some commits could have deleted file in question,\n@@ -5141,7 +5147,7 @@ sub git_history {\n \t\t$ftype = git_get_type($hash);\n \t}\n \tif (!defined $ftype) {\n-\t\tdie_error(undef, \"Unknown type of object\");\n+\t\tdie_error(500, \"Unknown type of object\");\n \t}\n \n \tmy $paging_nav = '';\n@@ -5179,19 +5185,16 @@ sub git_history {\n }\n \n sub git_search {\n-\tmy ($have_search) = gitweb_check_feature('search');\n-\tif (!$have_search) {\n-\t\tdie_error('403 Permission denied', \"Permission denied\");\n-\t}\n+\tgitweb_check_feature('search') or die_error(403, \"Search is disabled\");\n \tif (!defined $searchtext) {\n-\t\tdie_error(undef, \"Text field empty\");\n+\t\tdie_error(400, \"Text field is empty\");\n \t}\n \tif (!defined $hash) {\n \t\t$hash = git_get_head_hash($project);\n \t}\n \tmy %co = parse_commit($hash);\n \tif (!%co) {\n-\t\tdie_error(undef, \"Unknown commit object\");\n+\t\tdie_error(404, \"Unknown commit object\");\n \t}\n \tif (!defined $page) {\n \t\t$page = 0;\n@@ -5201,16 +5204,12 @@ sub git_search {\n \tif ($searchtype eq 'pickaxe') {\n \t\t# pickaxe may take all resources of your box and run for several minutes\n \t\t# with every query - so decide by yourself how public you make this feature\n-\t\tmy ($have_pickaxe) = gitweb_check_feature('pickaxe');\n-\t\tif (!$have_pickaxe) {\n-\t\t\tdie_error('403 Permission denied', \"Permission denied\");\n-\t\t}\n+\t\tgitweb_check_feature('pickaxe')\n+\t\t or die_error(403, \"Pickaxe is disabled\");\n \t}\n \tif ($searchtype eq 'grep') {\n-\t\tmy ($have_grep) = gitweb_check_feature('grep');\n-\t\tif (!$have_grep) {\n-\t\t\tdie_error('403 Permission denied', \"Permission denied\");\n-\t\t}\n+\t\tgitweb_check_feature('grep')\n+\t\t or die_error(403, \"Grep is disabled\");\n \t}\n \n \tgit_header_html();\n@@ -5484,7 +5483,7 @@ sub git_feed {\n \t# Atom: http://www.atomenabled.org/developers/syndication/\n \t# RSS: http://www.notestips.com/80256B3A007F2692/1/NAMO5P9UPQ\n \tif ($format ne 'rss' && $format ne 'atom') {\n-\t\tdie_error(undef, \"Unknown web feed format\");\n+\t\tdie_error(400, \"Unknown web feed format\");\n \t}\n \n \t# log/feed of current (HEAD) branch, log of given branch, history of file/directory\n-- \n1.5.6.149.g06c04.dirty\n"},{"id":"80362","messageId":"1213907110-5080-1-git-send-email-LeWiemann@gmail.com","threadId":"13968","inReplyTo":"1213905801-2811-1-git-send-email-LeWiemann@gmail.com","subject":"[PATCH v3] gitweb: standarize HTTP status codes","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-06-19T20:25:10Z","receivedAt":"2008-06-19T20:25:10Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Many error status codes simply default to 403 Forbidden, which is not\ncorrect in most cases. This patch makes gitweb return semantically\ncorrect status codes.\n\nFor convenience the die_error function now only takes the status code\nwithout reason as first parameter (e.g. 404 instead of \"404 Not\nFound\"), and it now defaults to 500 (Internal Server Error), even\nthough the default is not used anywhere.\n\nAlso documented status code conventions in die_error.\n\nSigned-off-by: Lea Wiemann \n---\n[Resent with fixed diff to v2 so git-am doesn't get confused. ;-)]\n\nChanges since v2: die_error now adds the reason strings defined by RFC\n2616 to the HTTP status code; incorporated Jakub's other suggestions.\nDiff to v2 follows.\n\nI didn't use the HTTP_NOT_FOUND etc. suggestion because I found it too\nverbose and obtrusive.\n\nJust a friendly reminder, please remember that discussing fairly\ntrivial changes in-depth might be not a good use of all participants'\ntime; IOW, sending 340 lines of email about HTTP status codes that\ndon't *really* matter is probably overkill. (In case you're\ninterested, the reason why I wrote this patch in the first place is in\norder to be able to tell unforeseen errors from expected errors in the\ntest suite.) I recommend you read http://bikeshed.com/ if you haven't\ndone so.\n\nAlso, increasing the time between patch submission and acceptance\ncauses added overhead (effort) by itself (i.e. *in addition* to the\ntime spend on discussing and resending patches); that's because you\ncannot base anything on the code without risking merge conflicts.\nPlease think of that when replying to patches. ;-)\n\nBy the way, when you want to make trivial changes, it may be easier\nfor everyone if you simply implement them and send a follow-up patch\n(and it might actually be just as quick as suggesting the changes!).\nIn your follow-up patch you could provide a diff to the previous\nversion, and comment on the changes you made (or send it without\ncomments, even -- oftentimes they're obvious).\n\nAnyways, I hope everyone is happy with this version of the patch.\n\nDiff to v2:\n\n index 3ca45fd..3bc8f69 100755\n --- a/gitweb/gitweb.perl\n +++ b/gitweb/gitweb.perl\n @@ -2687,9 +2687,11 @@ sub die_error {\n my $status = shift || 500;\n my $error = shift || \"Internal server error\";\n\n -\t# Use a generic \"Error\" reason, e.g. \"404 Error\" instead of\n -\t# \"404 Not Found\". This is permissible by RFC 2616.\n -\tgit_header_html(\"$status Error\");\n +\tmy %http_responses = (400 => '400 Bad Request',\n +\t\t\t 403 => '403 Forbidden',\n +\t\t\t 404 => '404 Not Found',\n +\t\t\t 500 => '500 Internal Server Error');\n +\tgit_header_html($http_responses{$status});\n print <\n

\n @@ -4131,11 +4133,11 @@ sub git_blame {\n my $ftype;\n\n gitweb_check_feature('blame')\n -\t or die_error(403, \"Blame not allowed\");\n +\t or die_error(403, \"Blame view not allowed\");\n\n die_error(400, \"No file name given\") unless $file_name;\n $hash_base ||= git_get_head_hash($project);\n -\tdie_error(400, \"Couldn't find base commit\") unless ($hash_base);\n +\tdie_error(404, \"Couldn't find base commit\") unless ($hash_base);\n my %co = parse_commit($hash_base)\n or die_error(404, \"Commit not found\");\n if (!defined $hash) {\n @@ -4403,7 +4405,7 @@ sub git_tree {\n open my $fd, \"-|\", git_cmd(), \"ls-tree\", '-z', $hash\n or die_error(500, \"Open git-ls-tree failed\");\n my @entries = map { chomp; $_ } <$fd>;\n -\tclose $fd or die_error(500, \"Reading tree failed\");\n +\tclose $fd or die_error(404, \"Reading tree failed\");\n $/ = \"\\n\";\n\n my $refs = git_get_references();\n @@ -4641,7 +4643,7 @@ sub git_commit {\n $hash, \"--\"\n or die_error(500, \"Open git-diff-tree failed\");\n @difftree = map { chomp; $_ } <$fd>;\n -\tclose $fd or die_error(500, \"Reading git-diff-tree failed\");\n +\tclose $fd or die_error(404, \"Reading git-diff-tree failed\");\n\n # non-textual hash id's can be cached\n my $expires;\n @@ -4788,7 +4790,7 @@ sub git_blobdiff {\n or die_error(500, \"Open git-diff-tree failed\");\n @difftree = map { chomp; $_ } <$fd>;\n close $fd\n -\t\t\t\tor die_error(500, \"Reading git-diff-tree failed\");\n +\t\t\t\tor die_error(404, \"Reading git-diff-tree failed\");\n @difftree\n or die_error(404, \"Blob diff not found\");\n\n @@ -4806,7 +4808,7 @@ sub git_blobdiff {\n grep { /^:[0-7]{6} [0-7]{6} [0-9a-fA-F]{40} $hash/ }\n map { chomp; $_ } <$fd>;\n close $fd\n -\t\t\t\tor die_error(500, \"Reading git-diff-tree failed\");\n +\t\t\t\tor die_error(404, \"Reading git-diff-tree failed\");\n @difftree\n or die_error(404, \"Blob diff not found\");\n\n\n\n gitweb/gitweb.perl | 211 ++++++++++++++++++++++++++--------------------------\n 1 files changed, 105 insertions(+), 106 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 3a7adae..3bc8f69 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -386,7 +386,7 @@ $projects_list ||= $projectroot;\n our $action = $cgi->param('a');\n if (defined $action) {\n \tif ($action =~ m/[^0-9a-zA-Z\\.\\-_]/) {\n-\t\tdie_error(undef, \"Invalid action parameter\");\n+\t\tdie_error(400, \"Invalid action parameter\");\n \t}\n }\n \n@@ -399,21 +399,21 @@ if (defined $project) {\n \t ($export_ok && !(-e \"$projectroot/$project/$export_ok\")) ||\n \t ($strict_export && !project_in_list($project))) {\n \t\tundef $project;\n-\t\tdie_error(undef, \"No such project\");\n+\t\tdie_error(404, \"No such project\");\n \t}\n }\n \n our $file_name = $cgi->param('f');\n if (defined $file_name) {\n \tif (!validate_pathname($file_name)) {\n-\t\tdie_error(undef, \"Invalid file parameter\");\n+\t\tdie_error(400, \"Invalid file parameter\");\n \t}\n }\n \n our $file_parent = $cgi->param('fp');\n if (defined $file_parent) {\n \tif (!validate_pathname($file_parent)) {\n-\t\tdie_error(undef, \"Invalid file parent parameter\");\n+\t\tdie_error(400, \"Invalid file parent parameter\");\n \t}\n }\n \n@@ -421,21 +421,21 @@ if (defined $file_parent) {\n our $hash = $cgi->param('h');\n if (defined $hash) {\n \tif (!validate_refname($hash)) {\n-\t\tdie_error(undef, \"Invalid hash parameter\");\n+\t\tdie_error(400, \"Invalid hash parameter\");\n \t}\n }\n \n our $hash_parent = $cgi->param('hp');\n if (defined $hash_parent) {\n \tif (!validate_refname($hash_parent)) {\n-\t\tdie_error(undef, \"Invalid hash parent parameter\");\n+\t\tdie_error(400, \"Invalid hash parent parameter\");\n \t}\n }\n \n our $hash_base = $cgi->param('hb');\n if (defined $hash_base) {\n \tif (!validate_refname($hash_base)) {\n-\t\tdie_error(undef, \"Invalid hash base parameter\");\n+\t\tdie_error(400, \"Invalid hash base parameter\");\n \t}\n }\n \n@@ -447,10 +447,10 @@ our @extra_options = $cgi->param('opt');\n if (defined @extra_options) {\n \tforeach my $opt (@extra_options) {\n \t\tif (not exists $allowed_options{$opt}) {\n-\t\t\tdie_error(undef, \"Invalid option parameter\");\n+\t\t\tdie_error(400, \"Invalid option parameter\");\n \t\t}\n \t\tif (not grep(/^$action$/, @{$allowed_options{$opt}})) {\n-\t\t\tdie_error(undef, \"Invalid option parameter for this action\");\n+\t\t\tdie_error(400, \"Invalid option parameter for this action\");\n \t\t}\n \t}\n }\n@@ -458,7 +458,7 @@ if (defined @extra_options) {\n our $hash_parent_base = $cgi->param('hpb');\n if (defined $hash_parent_base) {\n \tif (!validate_refname($hash_parent_base)) {\n-\t\tdie_error(undef, \"Invalid hash parent base parameter\");\n+\t\tdie_error(400, \"Invalid hash parent base parameter\");\n \t}\n }\n \n@@ -466,14 +466,14 @@ if (defined $hash_parent_base) {\n our $page = $cgi->param('pg');\n if (defined $page) {\n \tif ($page =~ m/[^0-9]/) {\n-\t\tdie_error(undef, \"Invalid page parameter\");\n+\t\tdie_error(400, \"Invalid page parameter\");\n \t}\n }\n \n our $searchtype = $cgi->param('st');\n if (defined $searchtype) {\n \tif ($searchtype =~ m/[^a-z]/) {\n-\t\tdie_error(undef, \"Invalid searchtype parameter\");\n+\t\tdie_error(400, \"Invalid searchtype parameter\");\n \t}\n }\n \n@@ -483,7 +483,7 @@ our $searchtext = $cgi->param('s');\n our $search_regexp;\n if (defined $searchtext) {\n \tif (length($searchtext) < 2) {\n-\t\tdie_error(undef, \"At least two characters are required for search parameter\");\n+\t\tdie_error(403, \"At least two characters are required for search parameter\");\n \t}\n \t$search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;\n }\n@@ -580,11 +580,11 @@ if (!defined $action) {\n \t}\n }\n if (!defined($actions{$action})) {\n-\tdie_error(undef, \"Unknown action\");\n+\tdie_error(400, \"Unknown action\");\n }\n if ($action !~ m/^(opml|project_list|project_index)$/ &&\n !$project) {\n-\tdie_error(undef, \"Project needed\");\n+\tdie_error(400, \"Project needed\");\n }\n $actions{$action}->();\n exit;\n@@ -1665,7 +1665,7 @@ sub git_get_hash_by_path {\n \t$path =~ s,/+$,,;\n \n \topen my $fd, \"-|\", git_cmd(), \"ls-tree\", $base, \"--\", $path\n-\t\tor die_error(undef, \"Open git-ls-tree failed\");\n+\t\tor die_error(500, \"Open git-ls-tree failed\");\n \tmy $line = <$fd>;\n \tclose $fd or return undef;\n \n@@ -2127,7 +2127,7 @@ sub parse_commit {\n \t\t\"--max-count=1\",\n \t\t$commit_id,\n \t\t\"--\",\n-\t\tor die_error(undef, \"Open git-rev-list failed\");\n+\t\tor die_error(500, \"Open git-rev-list failed\");\n \t%co = parse_commit_text(<$fd>, 1);\n \tclose $fd;\n \n@@ -2152,7 +2152,7 @@ sub parse_commits {\n \t\t$commit_id,\n \t\t\"--\",\n \t\t($filename ? ($filename) : ())\n-\t\tor die_error(undef, \"Open git-rev-list failed\");\n+\t\tor die_error(500, \"Open git-rev-list failed\");\n \twhile (my $line = <$fd>) {\n \t\tmy %co = parse_commit_text($line);\n \t\tpush @cos, \\%co;\n@@ -2672,11 +2672,26 @@ sub git_footer_html {\n \t \"\";\n }\n \n+# die_error(, )\n+# Example: die_error(404, 'Hash not found')\n+# By convention, use the following status codes (as defined in RFC 2616):\n+# 400: Invalid or missing CGI parameters, or\n+# requested object exists but has wrong type.\n+# 403: Requested feature (like \"pickaxe\" or \"snapshot\") not enabled on\n+# this server or project.\n+# 404: Requested object/revision/project doesn't exist.\n+# 500: The server isn't configured properly, or\n+# an internal error occurred (e.g. failed assertions caused by bugs), or\n+# an unknown error occurred (e.g. the git binary died unexpectedly).\n sub die_error {\n-\tmy $status = shift || \"403 Forbidden\";\n-\tmy $error = shift || \"Malformed query, file missing or permission denied\";\n-\n-\tgit_header_html($status);\n+\tmy $status = shift || 500;\n+\tmy $error = shift || \"Internal server error\";\n+\n+\tmy %http_responses = (400 => '400 Bad Request',\n+\t\t\t 403 => '403 Forbidden',\n+\t\t\t 404 => '404 Not Found',\n+\t\t\t 500 => '500 Internal Server Error');\n+\tgit_header_html($http_responses{$status});\n \tprint <\n

\n@@ -3924,12 +3939,12 @@ sub git_search_grep_body {\n sub git_project_list {\n \tmy $order = $cgi->param('o');\n \tif (defined $order && $order !~ m/none|project|descr|owner|age/) {\n-\t\tdie_error(undef, \"Unknown order parameter\");\n+\t\tdie_error(400, \"Unknown order parameter\");\n \t}\n \n \tmy @list = git_get_projects_list();\n \tif (!@list) {\n-\t\tdie_error(undef, \"No projects found\");\n+\t\tdie_error(404, \"No projects found\");\n \t}\n \n \tgit_header_html();\n@@ -3947,12 +3962,12 @@ sub git_project_list {\n sub git_forks {\n \tmy $order = $cgi->param('o');\n \tif (defined $order && $order !~ m/none|project|descr|owner|age/) {\n-\t\tdie_error(undef, \"Unknown order parameter\");\n+\t\tdie_error(400, \"Unknown order parameter\");\n \t}\n \n \tmy @list = git_get_projects_list($project);\n \tif (!@list) {\n-\t\tdie_error(undef, \"No forks found\");\n+\t\tdie_error(404, \"No forks found\");\n \t}\n \n \tgit_header_html();\n@@ -4081,7 +4096,7 @@ sub git_tag {\n \tmy %tag = parse_tag($hash);\n \n \tif (! %tag) {\n-\t\tdie_error(undef, \"Unknown tag object\");\n+\t\tdie_error(404, \"Unknown tag object\");\n \t}\n \n \tgit_print_header_div('commit', esc_html($tag{'name'}), $hash);\n@@ -4117,26 +4132,25 @@ sub git_blame {\n \tmy $fd;\n \tmy $ftype;\n \n-\tmy ($have_blame) = gitweb_check_feature('blame');\n-\tif (!$have_blame) {\n-\t\tdie_error('403 Permission denied', \"Permission denied\");\n-\t}\n-\tdie_error('404 Not Found', \"File name not defined\") if (!$file_name);\n+\tgitweb_check_feature('blame')\n+\t or die_error(403, \"Blame view not allowed\");\n+\n+\tdie_error(400, \"No file name given\") unless $file_name;\n \t$hash_base ||= git_get_head_hash($project);\n-\tdie_error(undef, \"Couldn't find base commit\") unless ($hash_base);\n+\tdie_error(404, \"Couldn't find base commit\") unless ($hash_base);\n \tmy %co = parse_commit($hash_base)\n-\t\tor die_error(undef, \"Reading commit failed\");\n+\t\tor die_error(404, \"Commit not found\");\n \tif (!defined $hash) {\n \t\t$hash = git_get_hash_by_path($hash_base, $file_name, \"blob\")\n-\t\t\tor die_error(undef, \"Error looking up file\");\n+\t\t\tor die_error(404, \"Error looking up file\");\n \t}\n \t$ftype = git_get_type($hash);\n \tif ($ftype !~ \"blob\") {\n-\t\tdie_error('400 Bad Request', \"Object is not a blob\");\n+\t\tdie_error(400, \"Object is not a blob\");\n \t}\n \topen ($fd, \"-|\", git_cmd(), \"blame\", '-p', '--',\n \t $file_name, $hash_base)\n-\t\tor die_error(undef, \"Open git-blame failed\");\n+\t\tor die_error(500, \"Open git-blame failed\");\n \tgit_header_html();\n \tmy $formats_nav =\n \t\t$cgi->a({-href => href(action=>\"blob\", -replay=>1)},\n@@ -4198,7 +4212,7 @@ HTML\n \t\t\tprint \"\\n\";\n \t\t}\n \t\topen (my $dd, \"-|\", git_cmd(), \"rev-parse\", \"$full_rev^\")\n-\t\t\tor die_error(undef, \"Open git-rev-parse failed\");\n+\t\t\tor die_error(500, \"Open git-rev-parse failed\");\n \t\tmy $parent_commit = <$dd>;\n \t\tclose $dd;\n \t\tchomp($parent_commit);\n@@ -4255,9 +4269,9 @@ sub git_blob_plain {\n \t\tif (defined $file_name) {\n \t\t\tmy $base = $hash_base || git_get_head_hash($project);\n \t\t\t$hash = git_get_hash_by_path($base, $file_name, \"blob\")\n-\t\t\t\tor die_error(undef, \"Error lookup file\");\n+\t\t\t\tor die_error(404, \"Cannot find file\");\n \t\t} else {\n-\t\t\tdie_error(undef, \"No file name defined\");\n+\t\t\tdie_error(400, \"No file name defined\");\n \t\t}\n \t} elsif ($hash =~ m/^[0-9a-fA-F]{40}$/) {\n \t\t# blobs defined by non-textual hash id's can be cached\n@@ -4265,7 +4279,7 @@ sub git_blob_plain {\n \t}\n \n \topen my $fd, \"-|\", git_cmd(), \"cat-file\", \"blob\", $hash\n-\t\tor die_error(undef, \"Open git-cat-file blob '$hash' failed\");\n+\t\tor die_error(500, \"Open git-cat-file blob '$hash' failed\");\n \n \t# content-type (can include charset)\n \t$type = blob_contenttype($fd, $file_name, $type);\n@@ -4297,9 +4311,9 @@ sub git_blob {\n \t\tif (defined $file_name) {\n \t\t\tmy $base = $hash_base || git_get_head_hash($project);\n \t\t\t$hash = git_get_hash_by_path($base, $file_name, \"blob\")\n-\t\t\t\tor die_error(undef, \"Error lookup file\");\n+\t\t\t\tor die_error(404, \"Cannot find file\");\n \t\t} else {\n-\t\t\tdie_error(undef, \"No file name defined\");\n+\t\t\tdie_error(400, \"No file name defined\");\n \t\t}\n \t} elsif ($hash =~ m/^[0-9a-fA-F]{40}$/) {\n \t\t# blobs defined by non-textual hash id's can be cached\n@@ -4308,7 +4322,7 @@ sub git_blob {\n \n \tmy ($have_blame) = gitweb_check_feature('blame');\n \topen my $fd, \"-|\", git_cmd(), \"cat-file\", \"blob\", $hash\n-\t\tor die_error(undef, \"Couldn't cat $file_name, $hash\");\n+\t\tor die_error(500, \"Couldn't cat $file_name, $hash\");\n \tmy $mimetype = blob_mimetype($fd, $file_name);\n \tif ($mimetype !~ m!^(?:text/|image/(?:gif|png|jpeg)$)! && -B $fd) {\n \t\tclose $fd;\n@@ -4389,9 +4403,9 @@ sub git_tree {\n \t}\n \t$/ = \"\\0\";\n \topen my $fd, \"-|\", git_cmd(), \"ls-tree\", '-z', $hash\n-\t\tor die_error(undef, \"Open git-ls-tree failed\");\n+\t\tor die_error(500, \"Open git-ls-tree failed\");\n \tmy @entries = map { chomp; $_ } <$fd>;\n-\tclose $fd or die_error(undef, \"Reading tree failed\");\n+\tclose $fd or die_error(404, \"Reading tree failed\");\n \t$/ = \"\\n\";\n \n \tmy $refs = git_get_references();\n@@ -4481,16 +4495,16 @@ sub git_snapshot {\n \n \tmy $format = $cgi->param('sf');\n \tif (!@supported_fmts) {\n-\t\tdie_error('403 Permission denied', \"Permission denied\");\n+\t\tdie_error(403, \"Snapshots not allowed\");\n \t}\n \t# default to first supported snapshot format\n \t$format ||= $supported_fmts[0];\n \tif ($format !~ m/^[a-z0-9]+$/) {\n-\t\tdie_error(undef, \"Invalid snapshot format parameter\");\n+\t\tdie_error(400, \"Invalid snapshot format parameter\");\n \t} elsif (!exists($known_snapshot_formats{$format})) {\n-\t\tdie_error(undef, \"Unknown snapshot format\");\n+\t\tdie_error(400, \"Unknown snapshot format\");\n \t} elsif (!grep($_ eq $format, @supported_fmts)) {\n-\t\tdie_error(undef, \"Unsupported snapshot format\");\n+\t\tdie_error(403, \"Unsupported snapshot format\");\n \t}\n \n \tif (!defined $hash) {\n@@ -4518,7 +4532,7 @@ sub git_snapshot {\n \t\t-status => '200 OK');\n \n \topen my $fd, \"-|\", $cmd\n-\t\tor die_error(undef, \"Execute git-archive failed\");\n+\t\tor die_error(500, \"Execute git-archive failed\");\n \tbinmode STDOUT, ':raw';\n \tprint <$fd>;\n \tbinmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi\n@@ -4586,10 +4600,8 @@ sub git_log {\n \n sub git_commit {\n \t$hash ||= $hash_base || \"HEAD\";\n-\tmy %co = parse_commit($hash);\n-\tif (!%co) {\n-\t\tdie_error(undef, \"Unknown commit object\");\n-\t}\n+\tmy %co = parse_commit($hash)\n+\t or die_error(404, \"Unknown commit object\");\n \tmy %ad = parse_date($co{'author_epoch'}, $co{'author_tz'});\n \tmy %cd = parse_date($co{'committer_epoch'}, $co{'committer_tz'});\n \n@@ -4629,9 +4641,9 @@ sub git_commit {\n \t\t@diff_opts,\n \t\t(@$parents <= 1 ? $parent : '-c'),\n \t\t$hash, \"--\"\n-\t\tor die_error(undef, \"Open git-diff-tree failed\");\n+\t\tor die_error(500, \"Open git-diff-tree failed\");\n \t@difftree = map { chomp; $_ } <$fd>;\n-\tclose $fd or die_error(undef, \"Reading git-diff-tree failed\");\n+\tclose $fd or die_error(404, \"Reading git-diff-tree failed\");\n \n \t# non-textual hash id's can be cached\n \tmy $expires;\n@@ -4724,33 +4736,33 @@ sub git_object {\n \n \t\topen my $fd, \"-|\", quote_command(\n \t\t\tgit_cmd(), 'cat-file', '-t', $object_id) . ' 2> /dev/null'\n-\t\t\tor die_error('404 Not Found', \"Object does not exist\");\n+\t\t\tor die_error(404, \"Object does not exist\");\n \t\t$type = <$fd>;\n \t\tchomp $type;\n \t\tclose $fd\n-\t\t\tor die_error('404 Not Found', \"Object does not exist\");\n+\t\t\tor die_error(404, \"Object does not exist\");\n \n \t# - hash_base and file_name\n \t} elsif ($hash_base && defined $file_name) {\n \t\t$file_name =~ s,/+$,,;\n \n \t\tsystem(git_cmd(), \"cat-file\", '-e', $hash_base) == 0\n-\t\t\tor die_error('404 Not Found', \"Base object does not exist\");\n+\t\t\tor die_error(404, \"Base object does not exist\");\n \n \t\t# here errors should not hapen\n \t\topen my $fd, \"-|\", git_cmd(), \"ls-tree\", $hash_base, \"--\", $file_name\n-\t\t\tor die_error(undef, \"Open git-ls-tree failed\");\n+\t\t\tor die_error(500, \"Open git-ls-tree failed\");\n \t\tmy $line = <$fd>;\n \t\tclose $fd;\n \n \t\t#'100644 blob 0fa3f3a66fb6a137f6ec2c19351ed4d807070ffa\tpanic.c'\n \t\tunless ($line && $line =~ m/^([0-9]+) (.+) ([0-9a-fA-F]{40})\\t/) {\n-\t\t\tdie_error('404 Not Found', \"File or directory for given base does not exist\");\n+\t\t\tdie_error(404, \"File or directory for given base does not exist\");\n \t\t}\n \t\t$type = $2;\n \t\t$hash = $3;\n \t} else {\n-\t\tdie_error('404 Not Found', \"Not enough information to find object\");\n+\t\tdie_error(400, \"Not enough information to find object\");\n \t}\n \n \tprint $cgi->redirect(-uri => href(action=>$type, -full=>1,\n@@ -4775,12 +4787,12 @@ sub git_blobdiff {\n \t\t\topen $fd, \"-|\", git_cmd(), \"diff-tree\", '-r', @diff_opts,\n \t\t\t\t$hash_parent_base, $hash_base,\n \t\t\t\t\"--\", (defined $file_parent ? $file_parent : ()), $file_name\n-\t\t\t\tor die_error(undef, \"Open git-diff-tree failed\");\n+\t\t\t\tor die_error(500, \"Open git-diff-tree failed\");\n \t\t\t@difftree = map { chomp; $_ } <$fd>;\n \t\t\tclose $fd\n-\t\t\t\tor die_error(undef, \"Reading git-diff-tree failed\");\n+\t\t\t\tor die_error(404, \"Reading git-diff-tree failed\");\n \t\t\t@difftree\n-\t\t\t\tor die_error('404 Not Found', \"Blob diff not found\");\n+\t\t\t\tor die_error(404, \"Blob diff not found\");\n \n \t\t} elsif (defined $hash &&\n \t\t $hash =~ /[0-9a-fA-F]{40}/) {\n@@ -4789,23 +4801,23 @@ sub git_blobdiff {\n \t\t\t# read filtered raw output\n \t\t\topen $fd, \"-|\", git_cmd(), \"diff-tree\", '-r', @diff_opts,\n \t\t\t\t$hash_parent_base, $hash_base, \"--\"\n-\t\t\t\tor die_error(undef, \"Open git-diff-tree failed\");\n+\t\t\t\tor die_error(500, \"Open git-diff-tree failed\");\n \t\t\t@difftree =\n \t\t\t\t# ':100644 100644 03b21826... 3b93d5e7... M\tls-files.c'\n \t\t\t\t# $hash == to_id\n \t\t\t\tgrep { /^:[0-7]{6} [0-7]{6} [0-9a-fA-F]{40} $hash/ }\n \t\t\t\tmap { chomp; $_ } <$fd>;\n \t\t\tclose $fd\n-\t\t\t\tor die_error(undef, \"Reading git-diff-tree failed\");\n+\t\t\t\tor die_error(404, \"Reading git-diff-tree failed\");\n \t\t\t@difftree\n-\t\t\t\tor die_error('404 Not Found', \"Blob diff not found\");\n+\t\t\t\tor die_error(404, \"Blob diff not found\");\n \n \t\t} else {\n-\t\t\tdie_error('404 Not Found', \"Missing one of the blob diff parameters\");\n+\t\t\tdie_error(400, \"Missing one of the blob diff parameters\");\n \t\t}\n \n \t\tif (@difftree > 1) {\n-\t\t\tdie_error('404 Not Found', \"Ambiguous blob diff specification\");\n+\t\t\tdie_error(400, \"Ambiguous blob diff specification\");\n \t\t}\n \n \t\t%diffinfo = parse_difftree_raw_line($difftree[0]);\n@@ -4826,7 +4838,7 @@ sub git_blobdiff {\n \t\t\t'-p', ($format eq 'html' ? \"--full-index\" : ()),\n \t\t\t$hash_parent_base, $hash_base,\n \t\t\t\"--\", (defined $file_parent ? $file_parent : ()), $file_name\n-\t\t\tor die_error(undef, \"Open git-diff-tree failed\");\n+\t\t\tor die_error(500, \"Open git-diff-tree failed\");\n \t}\n \n \t# old/legacy style URI\n@@ -4862,9 +4874,9 @@ sub git_blobdiff {\n \t\topen $fd, \"-|\", git_cmd(), \"diff\", @diff_opts,\n \t\t\t'-p', ($format eq 'html' ? \"--full-index\" : ()),\n \t\t\t$hash_parent, $hash, \"--\"\n-\t\t\tor die_error(undef, \"Open git-diff failed\");\n+\t\t\tor die_error(500, \"Open git-diff failed\");\n \t} else {\n-\t\tdie_error('404 Not Found', \"Missing one of the blob diff parameters\")\n+\t\tdie_error(400, \"Missing one of the blob diff parameters\")\n \t\t\tunless %diffinfo;\n \t}\n \n@@ -4897,7 +4909,7 @@ sub git_blobdiff {\n \t\tprint \"X-Git-Url: \" . $cgi->self_url() . \"\\n\\n\";\n \n \t} else {\n-\t\tdie_error(undef, \"Unknown blobdiff format\");\n+\t\tdie_error(400, \"Unknown blobdiff format\");\n \t}\n \n \t# patch\n@@ -4932,10 +4944,8 @@ sub git_blobdiff_plain {\n sub git_commitdiff {\n \tmy $format = shift || 'html';\n \t$hash ||= $hash_base || \"HEAD\";\n-\tmy %co = parse_commit($hash);\n-\tif (!%co) {\n-\t\tdie_error(undef, \"Unknown commit object\");\n-\t}\n+\tmy %co = parse_commit($hash)\n+\t or die_error(404, \"Unknown commit object\");\n \n \t# choose format for commitdiff for merge\n \tif (! defined $hash_parent && @{$co{'parents'}} > 1) {\n@@ -5017,7 +5027,7 @@ sub git_commitdiff {\n \t\topen $fd, \"-|\", git_cmd(), \"diff-tree\", '-r', @diff_opts,\n \t\t\t\"--no-commit-id\", \"--patch-with-raw\", \"--full-index\",\n \t\t\t$hash_parent_param, $hash, \"--\"\n-\t\t\tor die_error(undef, \"Open git-diff-tree failed\");\n+\t\t\tor die_error(500, \"Open git-diff-tree failed\");\n \n \t\twhile (my $line = <$fd>) {\n \t\t\tchomp $line;\n@@ -5029,10 +5039,10 @@ sub git_commitdiff {\n \t} elsif ($format eq 'plain') {\n \t\topen $fd, \"-|\", git_cmd(), \"diff-tree\", '-r', @diff_opts,\n \t\t\t'-p', $hash_parent_param, $hash, \"--\"\n-\t\t\tor die_error(undef, \"Open git-diff-tree failed\");\n+\t\t\tor die_error(500, \"Open git-diff-tree failed\");\n \n \t} else {\n-\t\tdie_error(undef, \"Unknown commitdiff format\");\n+\t\tdie_error(400, \"Unknown commitdiff format\");\n \t}\n \n \t# non-textual hash id's can be cached\n@@ -5115,19 +5125,15 @@ sub git_history {\n \t\t$page = 0;\n \t}\n \tmy $ftype;\n-\tmy %co = parse_commit($hash_base);\n-\tif (!%co) {\n-\t\tdie_error(undef, \"Unknown commit object\");\n-\t}\n+\tmy %co = parse_commit($hash_base)\n+\t or die_error(404, \"Unknown commit object\");\n \n \tmy $refs = git_get_references();\n \tmy $limit = sprintf(\"--max-count=%i\", (100 * ($page+1)));\n \n \tmy @commitlist = parse_commits($hash_base, 101, (100 * $page),\n-\t $file_name, \"--full-history\");\n-\tif (!@commitlist) {\n-\t\tdie_error('404 Not Found', \"No such file or directory on given branch\");\n-\t}\n+\t $file_name, \"--full-history\")\n+\t or die_error(404, \"No such file or directory on given branch\");\n \n \tif (!defined $hash && defined $file_name) {\n \t\t# some commits could have deleted file in question,\n@@ -5141,7 +5147,7 @@ sub git_history {\n \t\t$ftype = git_get_type($hash);\n \t}\n \tif (!defined $ftype) {\n-\t\tdie_error(undef, \"Unknown type of object\");\n+\t\tdie_error(500, \"Unknown type of object\");\n \t}\n \n \tmy $paging_nav = '';\n@@ -5179,19 +5185,16 @@ sub git_history {\n }\n \n sub git_search {\n-\tmy ($have_search) = gitweb_check_feature('search');\n-\tif (!$have_search) {\n-\t\tdie_error('403 Permission denied', \"Permission denied\");\n-\t}\n+\tgitweb_check_feature('search') or die_error(403, \"Search is disabled\");\n \tif (!defined $searchtext) {\n-\t\tdie_error(undef, \"Text field empty\");\n+\t\tdie_error(400, \"Text field is empty\");\n \t}\n \tif (!defined $hash) {\n \t\t$hash = git_get_head_hash($project);\n \t}\n \tmy %co = parse_commit($hash);\n \tif (!%co) {\n-\t\tdie_error(undef, \"Unknown commit object\");\n+\t\tdie_error(404, \"Unknown commit object\");\n \t}\n \tif (!defined $page) {\n \t\t$page = 0;\n@@ -5201,16 +5204,12 @@ sub git_search {\n \tif ($searchtype eq 'pickaxe') {\n \t\t# pickaxe may take all resources of your box and run for several minutes\n \t\t# with every query - so decide by yourself how public you make this feature\n-\t\tmy ($have_pickaxe) = gitweb_check_feature('pickaxe');\n-\t\tif (!$have_pickaxe) {\n-\t\t\tdie_error('403 Permission denied', \"Permission denied\");\n-\t\t}\n+\t\tgitweb_check_feature('pickaxe')\n+\t\t or die_error(403, \"Pickaxe is disabled\");\n \t}\n \tif ($searchtype eq 'grep') {\n-\t\tmy ($have_grep) = gitweb_check_feature('grep');\n-\t\tif (!$have_grep) {\n-\t\t\tdie_error('403 Permission denied', \"Permission denied\");\n-\t\t}\n+\t\tgitweb_check_feature('grep')\n+\t\t or die_error(403, \"Grep is disabled\");\n \t}\n \n \tgit_header_html();\n@@ -5484,7 +5483,7 @@ sub git_feed {\n \t# Atom: http://www.atomenabled.org/developers/syndication/\n \t# RSS: http://www.notestips.com/80256B3A007F2692/1/NAMO5P9UPQ\n \tif ($format ne 'rss' && $format ne 'atom') {\n-\t\tdie_error(undef, \"Unknown web feed format\");\n+\t\tdie_error(400, \"Unknown web feed format\");\n \t}\n \n \t# log/feed of current (HEAD) branch, log of given branch, history of file/directory\n-- \n1.5.6.149.g06c04.dirty\n"},{"id":"80389","messageId":"200806200022.39685.jnareb@gmail.com","threadId":"13968","inReplyTo":"485AAEB9.2080100@gmail.com","subject":"Re: [PATCH v2] gitweb: standarize HTTP status codes","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-19T22:22:38Z","receivedAt":"2008-06-19T22:22:38Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Lea Wiemann wrote:\n> Jakub Narebski wrote:\n>> Lea Wiemann writes:\n>>>\n>>> For convenience the die_error function now only takes the status code\n>>> without reason as first parameter (e.g. 404 instead of \"404 Not Found\")\n>> \n>> _Whose_ convenience?\n> \n> The developer's convenience of course. It's plain redundant.\n\nRedundancy isn't always bad.\n\nMoreover I think that \"convenience of developer\" here is a bit matter\nof taste, and the fact if one is web developer, or \"accidental\" gitweb\ndeveloper.\n \n>> * I don't think that RFC 2616 allows blanket replacing reason phrase\n>> by generic \"Error\",\n>> * Test::WWW::Mechanize displays both HTTP error status code and\n>> reason phrase when get_ok(...) fails:\n>> * From the point of view of someone examinimg gitweb.perl code, 400,\n>> 403, 404, 500 are _magic numbers_;\n> \n> I think we're really arguing about the color of the bikeshed here. IMO \n> we're not stretching RFC 2616 too much by putting \"Error\" there (since \n> reason codes don't matter on a technical level), and the status codes \n> make enough sense to me (and I'm not even a web developer) that I'm not \n> concerned about readability.\n\nWell, I didn't know what 400 code meant, and I had to check RFC 2616\nfor that. '400 Bad Request' is more readable.\n\nBut it is a bit bikeshedding. This patch consist of two things: using\nbetter HTTP error status codes (for example getting rid of \n403 Forbidden as default catch-all code and using 500 Internal Server\nError for cases where an error _is_ serious server error), and\nchanging die_error(...) signature / calling convention (meant for\nconvenience). I agree wholeheartly with first part (modulo using\n404 Not Found for errors which usually happens because of user error).\nSecond part might wait when code stabilizes and there is lull in the\ngitweb development (changes shouldn't conflict anyway, but applying\npatches might fail because of changed context)...\n\n...but as I can see you have send PATCH v3, in the form I can agree\nwith.\n \n> I don't think your constants a la HTTP_INVALID are a good idea (I \n> remember the status codes in a year, but maybe not the constants); \n\nI can agree with that.\n\n> die_error could figure out the right reason code using a hash. (...)\n\nGood idea. I see it is done this way in PATCH v3.\n \n>>> -\t\tdie_error(undef, \"At least two characters are required for search parameter\");\n>>> +\t\tdie_error(403, \"At least two characters are required for search parameter\");\n>> \n>> Should gitweb use there '403 Forbidden', or '400 Bad Request'?\n>> This is failing static validation of CGI parameters, not a matter of\n>> some permissions...\n> \n> I used 403 in the sense of \"sorry, we don't have shorter search strings \n> activated for performance reasons\". The '2' could even become \n> configurable. 400 is fine too, though, I don't care.\n\nI had in mind using '403 Forbidden' for \"permission denied\" errors,\ni.e. for cases where different _configuration_ could result in access.\n \n>>> -\tclose $fd or die_error(undef, \"Reading tree failed\");\n>>> +\tclose $fd or die_error(500, \"Reading tree failed\");\n>> \n>> Not O.K. Barring errors in gitweb code this might happen when\n>> [X Y Z]. All those are clearly 4xx _client_ errors,\n> \n> I haven't verified that, so until we have better error handling I prefer \n> 500, but I really won't bother objecting to 404.\n\nI'd rather have '404 Not Found' here; in most cases this is client\nerror, and one should examine URL not mail webmaster.\n\n> FWIW I'm \n> assuming that once gitweb uses the new API, that error handling code \n> will go away anyway.\n\nI hope that performance impact for non-caching case would be negligible,\nand cleaner code would overweigth this concern.\n \n>>> \tif (!defined $ftype) {\n>>> -\t\tdie_error(undef, \"Unknown type of object\");\n>>> +\t\tdie_error(500, \"Unknown type of object\");\n>> \n>> Errr... shouldn't be '400 Bad Request' here, per convention?\n> \n> Nope, we didn't get *anything* back, so something weird happened. 500.\n\nOhhh... right. I didn't get that from seeing only this part.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"80392","messageId":"m3hcbpm4pe.fsf@localhost.localdomain","threadId":"13968","inReplyTo":"1213907110-5080-1-git-send-email-LeWiemann@gmail.com","subject":"Re: [PATCH v3] gitweb: standarize HTTP status codes","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-19T22:37:35Z","receivedAt":"2008-06-19T22:37:35Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Lea Wiemann writes:\n\n> Changes since v2: die_error now adds the reason strings defined by RFC\n> 2616 to the HTTP status code; incorporated Jakub's other suggestions.\n> Diff to v2 follows.\n\nThis address both of my concerns: first, that for someone examining\nMechanize-based gitweb test number like 403, or 500 would be magical\nnumber without explanation (reason phrase) other than 'Error'.\n\nSecond, that for casual / accidental gitweb developer who has to add\nor modify a bit of code with die_error(...) wouldn't know which of\n\"magic number\" to use, if the case didn't fail into described\nsituation. Now it is enough to example die_error(...) in addition to\nsimilar code...\n \n> I didn't use the HTTP_NOT_FOUND etc. suggestion because I found it too\n> verbose and obtrusive.\n\nI can agree with that.\n\n> Just a friendly reminder, please remember that discussing fairly\n> trivial changes in-depth might be not a good use of all participants'\n> time [...]\n\nWell, this was what I though was patch revies... :-/\n\n> Anyways, I hope everyone is happy with this version of the patch.\n\nFWIW:\n\nAcked-by: Jakub Narebski \n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"80423","messageId":"7viqw5q6d7.fsf@gitster.siamese.dyndns.org","threadId":"13968","inReplyTo":"1213905801-2811-1-git-send-email-LeWiemann@gmail.com","subject":"Re: [PATCH v3] gitweb: standarize HTTP status codes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-20T00:48:04Z","receivedAt":"2008-06-20T00:48:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks. This will be part of 'next' as of tonight.\n"}]}