* [PATCH] gitweb: return correct HTTP status codes
@ 2008-06-15 21:15 Lea Wiemann
2008-06-15 22:48 ` Jakub Narebski
2008-06-18 0:15 ` [PATCH] gitweb: standarize " Lea Wiemann
0 siblings, 2 replies; 25+ messages in thread
From: Lea Wiemann @ 2008-06-15 21:15 UTC (permalink / raw)
To: git; +Cc: Lea Wiemann
Many error status codes simply default to 403 Forbidden, which is not
correct in most cases. This patch makes gitweb return semantically
correct status codes.
For convenience the die_error function now only takes the status code
without reason as first parameter (e.g. 404 instead of "404 Not
Found"), and it now defaults to 500 (Internal Server Error), even
though the default is not used anywhere.
Signed-off-by: Lea Wiemann <LeWiemann@gmail.com>
---
I recommend that you apply this patch beforehand:
[PATCH] gitweb: remove git_blame and rename git_blame2 to git_blame
http://article.gmane.org/gmane.comp.version-control.git/84036/raw
In some cases I've simply trusted the existing message (so when the
message said "not found", I would make it a 404 error without checking
whether the message is accurate). Also, in some cases an overly
generic 500 Internal Server Error is returned, e.g. in several cases
like this one ...
open my $fd, "-|", git_cmd(), "ls-tree", $base, "--", $path
- or die_error(undef, "Open git-ls-tree failed");
+ or die_error(500, "Open git-ls-tree failed");
... I suspect that the error could be triggered by non-existent hashes
as well, in which case the more specific 404 would be more appropriate
-- however, the current implementation oftentimes doesn't allow for
more fine-granulared checking, and I don't want to add actual code as
long as we don't have tests. So we'll go with catch-all 500 errors in
the meantime; it's not a regression, at least.
Jakub: Let's make this patch a prerequisite for the Mechanize tests so
we can test for exact status codes rather than [45][0-9][0-9].
gitweb/gitweb.perl | 196 ++++++++++++++++++++++++----------------------------
1 files changed, 91 insertions(+), 105 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 7b1b076..07e64da 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -386,7 +386,7 @@ $projects_list ||= $projectroot;
our $action = $cgi->param('a');
if (defined $action) {
if ($action =~ m/[^0-9a-zA-Z\.\-_]/) {
- die_error(undef, "Invalid action parameter");
+ die_error(400, "Invalid action parameter");
}
}
@@ -399,21 +399,21 @@ if (defined $project) {
($export_ok && !(-e "$projectroot/$project/$export_ok")) ||
($strict_export && !project_in_list($project))) {
undef $project;
- die_error(undef, "No such project");
+ die_error(404, "No such project");
}
}
our $file_name = $cgi->param('f');
if (defined $file_name) {
if (!validate_pathname($file_name)) {
- die_error(undef, "Invalid file parameter");
+ die_error(400, "Invalid file parameter");
}
}
our $file_parent = $cgi->param('fp');
if (defined $file_parent) {
if (!validate_pathname($file_parent)) {
- die_error(undef, "Invalid file parent parameter");
+ die_error(400, "Invalid file parent parameter");
}
}
@@ -421,21 +421,21 @@ if (defined $file_parent) {
our $hash = $cgi->param('h');
if (defined $hash) {
if (!validate_refname($hash)) {
- die_error(undef, "Invalid hash parameter");
+ die_error(400, "Invalid hash parameter");
}
}
our $hash_parent = $cgi->param('hp');
if (defined $hash_parent) {
if (!validate_refname($hash_parent)) {
- die_error(undef, "Invalid hash parent parameter");
+ die_error(400, "Invalid hash parent parameter");
}
}
our $hash_base = $cgi->param('hb');
if (defined $hash_base) {
if (!validate_refname($hash_base)) {
- die_error(undef, "Invalid hash base parameter");
+ die_error(400, "Invalid hash base parameter");
}
}
@@ -447,10 +447,10 @@ our @extra_options = $cgi->param('opt');
if (defined @extra_options) {
foreach my $opt (@extra_options) {
if (not exists $allowed_options{$opt}) {
- die_error(undef, "Invalid option parameter");
+ die_error(400, "Invalid option parameter");
}
if (not grep(/^$action$/, @{$allowed_options{$opt}})) {
- die_error(undef, "Invalid option parameter for this action");
+ die_error(400, "Invalid option parameter for this action");
}
}
}
@@ -458,7 +458,7 @@ if (defined @extra_options) {
our $hash_parent_base = $cgi->param('hpb');
if (defined $hash_parent_base) {
if (!validate_refname($hash_parent_base)) {
- die_error(undef, "Invalid hash parent base parameter");
+ die_error(400, "Invalid hash parent base parameter");
}
}
@@ -466,14 +466,14 @@ if (defined $hash_parent_base) {
our $page = $cgi->param('pg');
if (defined $page) {
if ($page =~ m/[^0-9]/) {
- die_error(undef, "Invalid page parameter");
+ die_error(400, "Invalid page parameter");
}
}
our $searchtype = $cgi->param('st');
if (defined $searchtype) {
if ($searchtype =~ m/[^a-z]/) {
- die_error(undef, "Invalid searchtype parameter");
+ die_error(400, "Invalid searchtype parameter");
}
}
@@ -483,7 +483,7 @@ our $searchtext = $cgi->param('s');
our $search_regexp;
if (defined $searchtext) {
if (length($searchtext) < 2) {
- die_error(undef, "At least two characters are required for search parameter");
+ die_error(403, "At least two characters are required for search parameter");
}
$search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;
}
@@ -580,11 +580,11 @@ if (!defined $action) {
}
}
if (!defined($actions{$action})) {
- die_error(undef, "Unknown action");
+ die_error(400, "Unknown action");
}
if ($action !~ m/^(opml|project_list|project_index)$/ &&
!$project) {
- die_error(undef, "Project needed");
+ die_error(400, "Project needed");
}
$actions{$action}->();
exit;
@@ -1661,7 +1661,7 @@ sub git_get_hash_by_path {
$path =~ s,/+$,,;
open my $fd, "-|", git_cmd(), "ls-tree", $base, "--", $path
- or die_error(undef, "Open git-ls-tree failed");
+ or die_error(500, "Open git-ls-tree failed");
my $line = <$fd>;
close $fd or return undef;
@@ -2123,7 +2123,7 @@ sub parse_commit {
"--max-count=1",
$commit_id,
"--",
- or die_error(undef, "Open git-rev-list failed");
+ or die_error(500, "Open git-rev-list failed");
%co = parse_commit_text(<$fd>, 1);
close $fd;
@@ -2148,7 +2148,7 @@ sub parse_commits {
$commit_id,
"--",
($filename ? ($filename) : ())
- or die_error(undef, "Open git-rev-list failed");
+ or die_error(500, "Open git-rev-list failed");
while (my $line = <$fd>) {
my %co = parse_commit_text($line);
push @cos, \%co;
@@ -2669,10 +2669,12 @@ sub git_footer_html {
}
sub die_error {
- my $status = shift || "403 Forbidden";
- my $error = shift || "Malformed query, file missing or permission denied";
+ my $status = shift || 500;
+ my $error = shift || "Internal server error";
- git_header_html($status);
+ # Use a generic "Error" reason, e.g. "404 Error" instead of
+ # "404 Not Found". This is permissible by RFC 2616.
+ git_header_html("$status Error");
print <<EOF;
<div class="page_body">
<br /><br />
@@ -3920,12 +3922,12 @@ sub git_search_grep_body {
sub git_project_list {
my $order = $cgi->param('o');
if (defined $order && $order !~ m/none|project|descr|owner|age/) {
- die_error(undef, "Unknown order parameter");
+ die_error(400, "Unknown order parameter");
}
my @list = git_get_projects_list();
if (!@list) {
- die_error(undef, "No projects found");
+ die_error(404, "No projects found");
}
git_header_html();
@@ -3943,12 +3945,12 @@ sub git_project_list {
sub git_forks {
my $order = $cgi->param('o');
if (defined $order && $order !~ m/none|project|descr|owner|age/) {
- die_error(undef, "Unknown order parameter");
+ die_error(400, "Unknown order parameter");
}
my @list = git_get_projects_list($project);
if (!@list) {
- die_error(undef, "No forks found");
+ die_error(404, "No forks found");
}
git_header_html();
@@ -4077,7 +4079,7 @@ sub git_tag {
my %tag = parse_tag($hash);
if (! %tag) {
- die_error(undef, "Unknown tag object");
+ die_error(404, "Unknown tag object");
}
git_print_header_div('commit', esc_html($tag{'name'}), $hash);
@@ -4113,26 +4115,25 @@ sub git_blame {
my $fd;
my $ftype;
- my ($have_blame) = gitweb_check_feature('blame');
- if (!$have_blame) {
- die_error('403 Permission denied', "Permission denied");
- }
- die_error('404 Not Found', "File name not defined") if (!$file_name);
+ gitweb_check_feature('blame')
+ or die_error(403, "Blame not allowed");
+
+ die_error(400, "No file name given") unless $file_name;
$hash_base ||= git_get_head_hash($project);
- die_error(undef, "Couldn't find base commit") unless ($hash_base);
+ die_error(400, "Couldn't find base commit") unless ($hash_base);
my %co = parse_commit($hash_base)
- or die_error(undef, "Reading commit failed");
+ or die_error(404, "Commit not found");
if (!defined $hash) {
$hash = git_get_hash_by_path($hash_base, $file_name, "blob")
- or die_error(undef, "Error looking up file");
+ or die_error(404, "Error looking up file");
}
$ftype = git_get_type($hash);
if ($ftype !~ "blob") {
- die_error('400 Bad Request', "Object is not a blob");
+ die_error(400, "Object is not a blob");
}
open ($fd, "-|", git_cmd(), "blame", '-p', '--',
$file_name, $hash_base)
- or die_error(undef, "Open git-blame failed");
+ or die_error(500, "Open git-blame failed");
git_header_html();
my $formats_nav =
$cgi->a({-href => href(action=>"blob", -replay=>1)},
@@ -4194,7 +4195,7 @@ HTML
print "</td>\n";
}
open (my $dd, "-|", git_cmd(), "rev-parse", "$full_rev^")
- or die_error(undef, "Open git-rev-parse failed");
+ or die_error(500, "Open git-rev-parse failed");
my $parent_commit = <$dd>;
close $dd;
chomp($parent_commit);
@@ -4251,9 +4252,9 @@ sub git_blob_plain {
if (defined $file_name) {
my $base = $hash_base || git_get_head_hash($project);
$hash = git_get_hash_by_path($base, $file_name, "blob")
- or die_error(undef, "Error lookup file");
+ or die_error(404, "Cannot find file");
} else {
- die_error(undef, "No file name defined");
+ die_error(400, "No file name defined");
}
} elsif ($hash =~ m/^[0-9a-fA-F]{40}$/) {
# blobs defined by non-textual hash id's can be cached
@@ -4261,7 +4262,7 @@ sub git_blob_plain {
}
open my $fd, "-|", git_cmd(), "cat-file", "blob", $hash
- or die_error(undef, "Open git-cat-file blob '$hash' failed");
+ or die_error(500, "Open git-cat-file blob '$hash' failed");
# content-type (can include charset)
$type = blob_contenttype($fd, $file_name, $type);
@@ -4293,9 +4294,9 @@ sub git_blob {
if (defined $file_name) {
my $base = $hash_base || git_get_head_hash($project);
$hash = git_get_hash_by_path($base, $file_name, "blob")
- or die_error(undef, "Error lookup file");
+ or die_error(404, "Cannot find file");
} else {
- die_error(undef, "No file name defined");
+ die_error(400, "No file name defined");
}
} elsif ($hash =~ m/^[0-9a-fA-F]{40}$/) {
# blobs defined by non-textual hash id's can be cached
@@ -4304,7 +4305,7 @@ sub git_blob {
my ($have_blame) = gitweb_check_feature('blame');
open my $fd, "-|", git_cmd(), "cat-file", "blob", $hash
- or die_error(undef, "Couldn't cat $file_name, $hash");
+ or die_error(500, "Couldn't cat $file_name, $hash");
my $mimetype = blob_mimetype($fd, $file_name);
if ($mimetype !~ m!^(?:text/|image/(?:gif|png|jpeg)$)! && -B $fd) {
close $fd;
@@ -4385,9 +4386,9 @@ sub git_tree {
}
$/ = "\0";
open my $fd, "-|", git_cmd(), "ls-tree", '-z', $hash
- or die_error(undef, "Open git-ls-tree failed");
+ or die_error(500, "Open git-ls-tree failed");
my @entries = map { chomp; $_ } <$fd>;
- close $fd or die_error(undef, "Reading tree failed");
+ close $fd or die_error(500, "Reading tree failed");
$/ = "\n";
my $refs = git_get_references();
@@ -4477,16 +4478,16 @@ sub git_snapshot {
my $format = $cgi->param('sf');
if (!@supported_fmts) {
- die_error('403 Permission denied', "Permission denied");
+ die_error(403, "Snapshots not allowed");
}
# default to first supported snapshot format
$format ||= $supported_fmts[0];
if ($format !~ m/^[a-z0-9]+$/) {
- die_error(undef, "Invalid snapshot format parameter");
+ die_error(400, "Invalid snapshot format parameter");
} elsif (!exists($known_snapshot_formats{$format})) {
- die_error(undef, "Unknown snapshot format");
+ die_error(400, "Unknown snapshot format");
} elsif (!grep($_ eq $format, @supported_fmts)) {
- die_error(undef, "Unsupported snapshot format");
+ die_error(403, "Unsupported snapshot format");
}
if (!defined $hash) {
@@ -4514,7 +4515,7 @@ sub git_snapshot {
-status => '200 OK');
open my $fd, "-|", $cmd
- or die_error(undef, "Execute git-archive failed");
+ or die_error(500, "Execute git-archive failed");
binmode STDOUT, ':raw';
print <$fd>;
binmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi
@@ -4582,10 +4583,8 @@ sub git_log {
sub git_commit {
$hash ||= $hash_base || "HEAD";
- my %co = parse_commit($hash);
- if (!%co) {
- die_error(undef, "Unknown commit object");
- }
+ my %co = parse_commit($hash)
+ or die_error(404, "Unknown commit object");
my %ad = parse_date($co{'author_epoch'}, $co{'author_tz'});
my %cd = parse_date($co{'committer_epoch'}, $co{'committer_tz'});
@@ -4625,9 +4624,9 @@ sub git_commit {
@diff_opts,
(@$parents <= 1 ? $parent : '-c'),
$hash, "--"
- or die_error(undef, "Open git-diff-tree failed");
+ or die_error(500, "Open git-diff-tree failed");
@difftree = map { chomp; $_ } <$fd>;
- close $fd or die_error(undef, "Reading git-diff-tree failed");
+ close $fd or die_error(500, "Reading git-diff-tree failed");
# non-textual hash id's can be cached
my $expires;
@@ -4720,33 +4719,33 @@ sub git_object {
my $git_command = git_cmd_str();
open my $fd, "-|", "$git_command cat-file -t $object_id 2>/dev/null"
- or die_error('404 Not Found', "Object does not exist");
+ or die_error(404, "Object does not exist");
$type = <$fd>;
chomp $type;
close $fd
- or die_error('404 Not Found', "Object does not exist");
+ or die_error(404, "Object does not exist");
# - hash_base and file_name
} elsif ($hash_base && defined $file_name) {
$file_name =~ s,/+$,,;
system(git_cmd(), "cat-file", '-e', $hash_base) == 0
- or die_error('404 Not Found', "Base object does not exist");
+ or die_error(404, "Base object does not exist");
# here errors should not hapen
open my $fd, "-|", git_cmd(), "ls-tree", $hash_base, "--", $file_name
- or die_error(undef, "Open git-ls-tree failed");
+ or die_error(500, "Open git-ls-tree failed");
my $line = <$fd>;
close $fd;
#'100644 blob 0fa3f3a66fb6a137f6ec2c19351ed4d807070ffa panic.c'
unless ($line && $line =~ m/^([0-9]+) (.+) ([0-9a-fA-F]{40})\t/) {
- die_error('404 Not Found', "File or directory for given base does not exist");
+ die_error(404, "File or directory for given base does not exist");
}
$type = $2;
$hash = $3;
} else {
- die_error('404 Not Found', "Not enough information to find object");
+ die_error(400, "Not enough information to find object");
}
print $cgi->redirect(-uri => href(action=>$type, -full=>1,
@@ -4771,12 +4770,12 @@ sub git_blobdiff {
open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
$hash_parent_base, $hash_base,
"--", (defined $file_parent ? $file_parent : ()), $file_name
- or die_error(undef, "Open git-diff-tree failed");
+ or die_error(500, "Open git-diff-tree failed");
@difftree = map { chomp; $_ } <$fd>;
close $fd
- or die_error(undef, "Reading git-diff-tree failed");
+ or die_error(500, "Reading git-diff-tree failed");
@difftree
- or die_error('404 Not Found', "Blob diff not found");
+ or die_error(404, "Blob diff not found");
} elsif (defined $hash &&
$hash =~ /[0-9a-fA-F]{40}/) {
@@ -4785,23 +4784,23 @@ sub git_blobdiff {
# read filtered raw output
open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
$hash_parent_base, $hash_base, "--"
- or die_error(undef, "Open git-diff-tree failed");
+ or die_error(500, "Open git-diff-tree failed");
@difftree =
# ':100644 100644 03b21826... 3b93d5e7... M ls-files.c'
# $hash == to_id
grep { /^:[0-7]{6} [0-7]{6} [0-9a-fA-F]{40} $hash/ }
map { chomp; $_ } <$fd>;
close $fd
- or die_error(undef, "Reading git-diff-tree failed");
+ or die_error(500, "Reading git-diff-tree failed");
@difftree
- or die_error('404 Not Found', "Blob diff not found");
+ or die_error(404, "Blob diff not found");
} else {
- die_error('404 Not Found', "Missing one of the blob diff parameters");
+ die_error(400, "Missing one of the blob diff parameters");
}
if (@difftree > 1) {
- die_error('404 Not Found', "Ambiguous blob diff specification");
+ die_error(400, "Ambiguous blob diff specification");
}
%diffinfo = parse_difftree_raw_line($difftree[0]);
@@ -4822,7 +4821,7 @@ sub git_blobdiff {
'-p', ($format eq 'html' ? "--full-index" : ()),
$hash_parent_base, $hash_base,
"--", (defined $file_parent ? $file_parent : ()), $file_name
- or die_error(undef, "Open git-diff-tree failed");
+ or die_error(500, "Open git-diff-tree failed");
}
# old/legacy style URI
@@ -4858,9 +4857,9 @@ sub git_blobdiff {
open $fd, "-|", git_cmd(), "diff", @diff_opts,
'-p', ($format eq 'html' ? "--full-index" : ()),
$hash_parent, $hash, "--"
- or die_error(undef, "Open git-diff failed");
+ or die_error(500, "Open git-diff failed");
} else {
- die_error('404 Not Found', "Missing one of the blob diff parameters")
+ die_error(400, "Missing one of the blob diff parameters")
unless %diffinfo;
}
@@ -4893,7 +4892,7 @@ sub git_blobdiff {
print "X-Git-Url: " . $cgi->self_url() . "\n\n";
} else {
- die_error(undef, "Unknown blobdiff format");
+ die_error(400, "Unknown blobdiff format");
}
# patch
@@ -4928,10 +4927,8 @@ sub git_blobdiff_plain {
sub git_commitdiff {
my $format = shift || 'html';
$hash ||= $hash_base || "HEAD";
- my %co = parse_commit($hash);
- if (!%co) {
- die_error(undef, "Unknown commit object");
- }
+ my %co = parse_commit($hash)
+ or die_error(404, "Unknown commit object");
# choose format for commitdiff for merge
if (! defined $hash_parent && @{$co{'parents'}} > 1) {
@@ -5013,7 +5010,7 @@ sub git_commitdiff {
open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
"--no-commit-id", "--patch-with-raw", "--full-index",
$hash_parent_param, $hash, "--"
- or die_error(undef, "Open git-diff-tree failed");
+ or die_error(500, "Open git-diff-tree failed");
while (my $line = <$fd>) {
chomp $line;
@@ -5025,10 +5022,10 @@ sub git_commitdiff {
} elsif ($format eq 'plain') {
open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
'-p', $hash_parent_param, $hash, "--"
- or die_error(undef, "Open git-diff-tree failed");
+ or die_error(500, "Open git-diff-tree failed");
} else {
- die_error(undef, "Unknown commitdiff format");
+ die_error(400, "Unknown commitdiff format");
}
# non-textual hash id's can be cached
@@ -5111,19 +5108,15 @@ sub git_history {
$page = 0;
}
my $ftype;
- my %co = parse_commit($hash_base);
- if (!%co) {
- die_error(undef, "Unknown commit object");
- }
+ my %co = parse_commit($hash_base)
+ or die_error(404, "Unknown commit object");
my $refs = git_get_references();
my $limit = sprintf("--max-count=%i", (100 * ($page+1)));
my @commitlist = parse_commits($hash_base, 101, (100 * $page),
- $file_name, "--full-history");
- if (!@commitlist) {
- die_error('404 Not Found', "No such file or directory on given branch");
- }
+ $file_name, "--full-history")
+ or die_error(404, "No such file or directory on given branch");
if (!defined $hash && defined $file_name) {
# some commits could have deleted file in question,
@@ -5137,7 +5130,7 @@ sub git_history {
$ftype = git_get_type($hash);
}
if (!defined $ftype) {
- die_error(undef, "Unknown type of object");
+ die_error(500, "Unknown type of object");
}
my $paging_nav = '';
@@ -5175,19 +5168,16 @@ sub git_history {
}
sub git_search {
- my ($have_search) = gitweb_check_feature('search');
- if (!$have_search) {
- die_error('403 Permission denied', "Permission denied");
- }
+ gitweb_check_feature('search') or die_error(403, "Search is disabled");
if (!defined $searchtext) {
- die_error(undef, "Text field empty");
+ die_error(400, "Text field is empty");
}
if (!defined $hash) {
$hash = git_get_head_hash($project);
}
my %co = parse_commit($hash);
if (!%co) {
- die_error(undef, "Unknown commit object");
+ die_error(404, "Unknown commit object");
}
if (!defined $page) {
$page = 0;
@@ -5197,16 +5187,12 @@ sub git_search {
if ($searchtype eq 'pickaxe') {
# pickaxe may take all resources of your box and run for several minutes
# with every query - so decide by yourself how public you make this feature
- my ($have_pickaxe) = gitweb_check_feature('pickaxe');
- if (!$have_pickaxe) {
- die_error('403 Permission denied', "Permission denied");
- }
+ gitweb_check_feature('pickaxe')
+ or die_error(403, "Pickaxe is disabled");
}
if ($searchtype eq 'grep') {
- my ($have_grep) = gitweb_check_feature('grep');
- if (!$have_grep) {
- die_error('403 Permission denied', "Permission denied");
- }
+ gitweb_check_feature('grep')
+ or die_error(403, "Grep is disabled");
}
git_header_html();
@@ -5480,7 +5466,7 @@ sub git_feed {
# Atom: http://www.atomenabled.org/developers/syndication/
# RSS: http://www.notestips.com/80256B3A007F2692/1/NAMO5P9UPQ
if ($format ne 'rss' && $format ne 'atom') {
- die_error(undef, "Unknown web feed format");
+ die_error(400, "Unknown web feed format");
}
# log/feed of current (HEAD) branch, log of given branch, history of file/directory
--
1.5.6.rc3.7.ged9620
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH] gitweb: return correct HTTP status codes
2008-06-15 21:15 [PATCH] gitweb: return correct HTTP status codes Lea Wiemann
@ 2008-06-15 22:48 ` Jakub Narebski
2008-06-16 15:57 ` Lea Wiemann
2008-06-18 0:15 ` [PATCH] gitweb: standarize " Lea Wiemann
1 sibling, 1 reply; 25+ messages in thread
From: Jakub Narebski @ 2008-06-15 22:48 UTC (permalink / raw)
To: Lea Wiemann; +Cc: git, Lea Wiemann
Lea Wiemann <lewiemann@gmail.com> writes:
I would perhaps choose "gitweb: standarize HTTP status codes"
as the subject, but the one you chosen is also good.
> Many error status codes simply default to 403 Forbidden, which is not
> correct in most cases. This patch makes gitweb return semantically
> correct status codes.
I'm not sure if "403 Forbidden" is not correct error code in the
absence of further information.
I'd like to have reasoning when, and why, we use which responses (HTTP
status codes) in which situations. Link to appropriate RFC you used
(and perhaps to some other information) wouldn't be out of the
question. See also below.
> For convenience the die_error function now only takes the status code
> without reason as first parameter (e.g. 404 instead of "404 Not
> Found"), and it now defaults to 500 (Internal Server Error), even
> though the default is not used anywhere.
Well, gitweb sometimes used different wording (different explanation)
for the same error message / HTTP status code, for example it is
generic "403 Forbidden", but when trying to access page which is not
available due to some restrictions (like feature being disabled) it is
"403 Permission Denied".
> Signed-off-by: Lea Wiemann <LeWiemann@gmail.com>
> ---
> I recommend that you apply this patch beforehand:
> [PATCH] gitweb: remove git_blame and rename git_blame2 to git_blame
> http://article.gmane.org/gmane.comp.version-control.git/84036/raw
First, why?
Second, I think it is not possible, as IIRC removal of git-annotate
based git_blame got already applied.
> In some cases I've simply trusted the existing message (so when the
> message said "not found", I would make it a 404 error without checking
> whether the message is accurate). Also, in some cases an overly
> generic 500 Internal Server Error is returned, e.g. in several cases
> like this one ...
>
> open my $fd, "-|", git_cmd(), "ls-tree", $base, "--", $path
> - or die_error(undef, "Open git-ls-tree failed");
> + or die_error(500, "Open git-ls-tree failed");
Should we really use "500 Internal Server Error" here? Usually this
would be not an error at all, but wrong parameters to git command,
i.e. it is _user_ who erred, not some error on _server_ side. I would
use "500 Internal Server Error" if for example git binary could not be
found...
> ... I suspect that the error could be triggered by non-existent hashes
> as well, in which case the more specific 404 would be more appropriate
> -- however, the current implementation oftentimes doesn't allow for
> more fine-granulared checking, and I don't want to add actual code as
> long as we don't have tests. So we'll go with catch-all 500 errors in
> the meantime; it's not a regression, at least.
...that is why I'd rather have "403 Forbidden" as catch-all error, as
it was done in gitweb. But that also seems not very good idea.
It seems like this is a matter of taste.
> Jakub: Let's make this patch a prerequisite for the Mechanize tests so
> we can test for exact status codes rather than [45][0-9][0-9].
Could you please Cc me if you address me in email? I am not
subscribed to git mailing list, I read it via GMane NNTP (Usenet news)
interface; and even if I had been subscribed it is better to Cc people
who might be interested (in the case of gitweb caching it would be
probably me, Petr Baudis, John Hawley, perhaps Luben Tuikov).
[...]
> sub die_error {
> - my $status = shift || "403 Forbidden";
> - my $error = shift || "Malformed query, file missing or permission denied";
> + my $status = shift || 500;
> + my $error = shift || "Internal server error";
Errr, I think "Malformed query, file missing or permission denied" is
actually closer to truth, and better error message (it is displayed in
body of message)
> - git_header_html($status);
> + # Use a generic "Error" reason, e.g. "404 Error" instead of
> + # "404 Not Found". This is permissible by RFC 2616.
> + git_header_html("$status Error");
> print <<EOF;
Even if it is _permissible_ by RFC 2616, I don't think it is
_recommended_ practice. IIRC it is recommnded for some (all?) HTTP
error status codes to give more detailed explanation in the body of
message.
==================================================================
Proposed usage of HTTP server/client error status codes in gitweb:
1. Basic CGI parameter validation; this catches for example invalid
action names and invalid order names, pathnames which doesn't look
like good pathnames, hash* parameters which doesn't look like
refnames, extra option parameters not on allowed list, page which
is not a number, unknown action, etc.
These should probably all return "400 Bad Request", or perhaps
"404 Not Found" as catch-all, not "403 Forbidden".
2. Gitweb cannot find requested resource; this includes requesting
project which does not exists (and, for security reasons, also
those excluded from gitweb by different means), tag does not exists
etc. I'm not sure if include there situations where for example
filename is missing for a 'blob' view request.
Here "404 Not Found" would be most valid.
3. Some required parameters are missing, for example action other than
projects list (in any format) and no project provided,
I think that here "400 Bad Request" is probably best solution.
4. Project list is requested, but there are no projects (or no forks)
to be displayed by gitweb.
This is a strange situation indeed, and I'm not sure what proper
HTTP code should be used. I don't think that "404 Not Found" would
be good solution...
5. When some feature is requested that is not enabled, but with
different gitweb configuration would be available.
Authorization wouldn't change the response, so I guess best would
be "403 Forbidden" or "403 Permission Denied".
6. Request is made for a wrong type of object, for example 'blame' on
object which is not a blob.
Here I guess "400 Bad Request" would be good solution (if not 404).
7. When the git command failed, and we don't know the reason...
--
Jakub Narebski
Poland
ShadeHawk on #git
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] gitweb: return correct HTTP status codes
2008-06-15 22:48 ` Jakub Narebski
@ 2008-06-16 15:57 ` Lea Wiemann
2008-06-16 16:43 ` Jakub Narebski
` (2 more replies)
0 siblings, 3 replies; 25+ messages in thread
From: Lea Wiemann @ 2008-06-16 15:57 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
Jakub Narebski wrote:
> [Subject:] I would perhaps choose "gitweb: standarize HTTP status codes"
Will fix if there's a follow-up patch.
> I'd like to have reasoning when, and why, we use which responses (HTTP
> status codes) in which situations. Link to appropriate RFC you used
> (and perhaps to some other information)
RFC 2616 has clear descriptions of the codes we use. Maybe a really
short list of conventions though, like you listed below? If we have
agreement I'll happily put it in.
>> For convenience the die_error function now only takes the status code
>> without reason as first parameter
>
> Well, gitweb sometimes used different wording
The 'reason' only appeared in the HTTP response header -- see below.
>> I recommend that you apply [PATCH] gitweb: remove git_blame
>
> I think it is not possible, as IIRC [it] got already applied.
It isn't applied on master (at the time I'm writing this), only on next.
Should I be working on the next branch?
>> open my $fd, "-|", git_cmd(), "ls-tree", $base, "--", $path
>> - or die_error(undef, "Open git-ls-tree failed");
>> + or die_error(500, "Open git-ls-tree failed");
>
> Should we really use "500 Internal Server Error" here? Usually this
> would be not an error at all, but wrong parameters to git command,
> i.e. it is _user_ who erred, not some error on _server_ side.
You cannot tell for sure -- all you know here is that the command
somehow failed when it shouldn't have, and so all you can give is 500;
see below. I don't think we should apply reasoning like "most commonly
it's a wrong hash, so let's return 404" -- we don't know, and we
shouldn't assume.
>> ... I suspect that the error could be triggered by non-existent hashes
>> as well, in which case the more specific 404 would be more appropriate
>> -- however, the current implementation oftentimes doesn't allow for
>> more fine-granulared checking
>
> ...that is why I'd rather have "403 Forbidden" as catch-all error, as
> it was done in gitweb. But that also seems not very good idea.
[IANA HTTP expert, but:] All we have in cases like the one above is "I
cannot deliver the content you requested, but I'm so confused that I
don't even know why" -- which sounds like 500 to me (definitely not
403). 403 would be used for some deactivated feature (commonly for
disabled directory listings I think) or something that shouldn't be
accessible to the public.
>> sub die_error {
>> - my $status = shift || "403 Forbidden";
>> - my $error = shift || "Malformed query, file missing or permission denied";
>> + my $status = shift || 500;
>> + my $error = shift || "Internal server error";
>
> Errr, I think "Malformed query, file missing or permission denied" is
> actually closer to truth, and better error message
This default is intended for expressions like "some_action(...) or
die_error" (without parameters), in which case you really can't tell
what triggered the error, so "Internal server error" is as specific as
you can get.
> (it is displayed in body of message)
Just to make sure we're on the same boat, the $error variable above *is*
what's displayed in the body.
>> + # Use a generic "Error" reason, e.g. "404 Error" instead of
>> + # "404 Not Found". This is permissible by RFC 2616.
>> + git_header_html("$status Error");
>
> I don't think it is _recommended_ practice. IIRC it is recommnded
> [...] to give more detailed explanation in the body of message.
The code above prints the HTTP response headers, not the body -- the
body contains the detailed error message passed to the die_error
function. Here's a stripped down version of gitweb's error response
(X'ed out against spam filter):
HTXTP/1.0 404 Error <------------ "Error" here
CoXntent-Type: text/htXml
... 404 - Hash not found ...
I don't know of *any* browser or tool that actually displays the reason
given in the HTTP response header, so defaulting to Error is fine here
(especially since RFC 261 explicitly allow it).
(I don't want to put the $error passed to die_error into the HTTP
response header, since badly interpolated strings might invalidate the
response. So using "Error" makes sure we always die gracefully.)
> Proposed HTTP server/client error status codes [heavily snipped]:
>
> 1. Basic CGI parameter validation; 400 or perhaps 404 as catch-all
Yes, 400 is appropriate here; I don't think there are any catch-all
cases to catch. :)
> 2. Gitweb cannot find requested resource: 404 Not Found
> 3. Some required parameters are missing: 400 Bad Request
> 5. When some feature is requested that is not enabled, but with
> different gitweb configuration would be available: 403
Yup, that's what I used.
> 4. Project list is requested, but there are no projects (or no forks)
> to be displayed by gitweb: not sure [...]. I don't think that
> "404 Not Found" would be good solution...
I see three possibilities:
404: "I couldn't find any projects/forks."
500: "The admin didn't set this up properly" (doesn't apply to forks)
200: "Here's the list your requested (it's empty though)."
Since 500 doesn't work for forks, and since 200 requires additional code
in order to not display an error page (which should be done in a
separate patch, with tests), 404 seemed like a reasonable default to me.
It's better than the previous 403, so it's not a regression.
> 6. Request is made for a wrong type of object, for example 'blame' on
> object which is not a blob: 400 (if not 404).
Yup. I wasn't sure, but I used 400 since it seemed a little more
appropriate ("you asked me for an invalid action on this object").
> 7. When the git command failed, and we don't know the reason...
... 500 (see above). :)
> Could you please Cc me if you address me in email?
Will do.
> probably me, Petr Baudis, John Hawley, perhaps Luben Tuikov
I wouldn't want to Cc people if I don't address them personally -- e.g.
neither Petr nor John are currently working on gitweb, so flooding their
mailboxes might seem a little rude; if they're interested they can
always filter for subjects. (Unless someone requests to always be CC'ed
of course.)
-- Lea
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] gitweb: return correct HTTP status codes
2008-06-16 15:57 ` Lea Wiemann
@ 2008-06-16 16:43 ` Jakub Narebski
2008-06-16 21:49 ` Lea Wiemann
2008-06-16 22:38 ` Junio C Hamano
2008-06-16 23:37 ` Jakub Narebski
2 siblings, 1 reply; 25+ messages in thread
From: Jakub Narebski @ 2008-06-16 16:43 UTC (permalink / raw)
To: Lea Wiemann; +Cc: git, Kay Sievers
[Fast reply, I will reply more in depth later]
Lea Wiemann wrote:
> Jakub Narebski wrote:
>> Lea Wiemann wrote:
>>> open my $fd, "-|", git_cmd(), "ls-tree", $base, "--", $path
>>> - or die_error(undef, "Open git-ls-tree failed");
>>> + or die_error(500, "Open git-ls-tree failed");
>>
>> Should we really use "500 Internal Server Error" here? Usually this
>> would be not an error at all, but wrong parameters to git command,
>> i.e. it is _user_ who erred, not some error on _server_ side.
>
> You cannot tell for sure -- all you know here is that the command
> somehow failed when it shouldn't have, and so all you can give is 500;
> see below. I don't think we should apply reasoning like "most commonly
> it's a wrong hash, so let's return 404" -- we don't know, and we
> shouldn't assume.
Well, we could, perhaps, examine stderr (or redirect it to stdout and
examine upon error) to check what was the error. Or when/if gitweb
start to use Git.pm methods, examine catched Error (for example
"fatal: bad revision '$hash'" would mean "404 Not Found" revision).
But I think in all, or almost all cases, the source is wrong parameters
in URL. Now, returning 5xx _server_ error would make me want to email
webmaster about error on his/her server, while 4xx _user_ error would
make me examine my input, i.e. URL I have entered, or handcrafted, or
magled. That is IMVHO *very* important difference, and why I am
against using "500 Internal Server Error" as catch-all; I can agree
that "403 Forbidden" (which is from the times where gitweb was developed
as separate project by Kay Sievers, old, old times of v056) is better
left for disabled features[*1*], and "404 Not Found" is better
catch-all.
Kay, do you remember why "403 Forbidden" was used as default catch-all
gitweb HTTP error status code?
> > probably me, Petr Baudis, John Hawley, perhaps Luben Tuikov
>
> I wouldn't want to Cc people if I don't address them personally -- e.g.
> neither Petr nor John are currently working on gitweb, so flooding their
> mailboxes might seem a little rude; if they're interested they can
> always filter for subjects. (Unless someone requests to always be CC'ed
> of course.)
O.K. although I have though that as John is your GSoC mentor, he might
be interested gitweb caching related posts. But this is something
better made agree with him.
BTW. I got three copies of this email: was it you fighting VGER
anti-spam filter?
--
Jakub Narebski
Poland
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] gitweb: return correct HTTP status codes
2008-06-16 16:43 ` Jakub Narebski
@ 2008-06-16 21:49 ` Lea Wiemann
2008-06-16 22:34 ` Jakub Narebski
0 siblings, 1 reply; 25+ messages in thread
From: Lea Wiemann @ 2008-06-16 21:49 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git, Kay Sievers
Jakub Narebski wrote:
> Well, we could, perhaps, examine stderr (or redirect it to stdout and
> examine upon error) to check what was the error.
We don't have to -- gitweb's current (suboptimal) error checking is
because it doesn't interface with git very well. The API I'm writing
will fix this (i.e. provide proper feedback in all cases) so we'll have
more specific status codes. IOW, we'll be able to differentiate between
500 and 404. Trust me on this one. ;-)
> But I think in all, or almost all cases, the source is wrong parameters
> in URL. Now, returning 5xx _server_ error would make me want to email
> webmaster about error on his/her server, while 4xx _user_ error would
> make me examine my input
Since the status codes will get better (more accurate) anyway, I care
more about correctness than practicalities right now (and I'm convinced
that only 500 is correct in the cases we're talking about). That said,
if you really want 404s in there, go ahead and send a follow-up patch, I
won't object.
> BTW. I got three copies of this email: was it you fighting VGER
> anti-spam filter?
Yup. Apparently it simply greps for Content-TypXe: text/hXtml. *shakes
head* :-)
-- Lea
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] gitweb: return correct HTTP status codes
2008-06-16 21:49 ` Lea Wiemann
@ 2008-06-16 22:34 ` Jakub Narebski
2008-06-17 13:53 ` Lea Wiemann
0 siblings, 1 reply; 25+ messages in thread
From: Jakub Narebski @ 2008-06-16 22:34 UTC (permalink / raw)
To: Lea Wiemann; +Cc: git
On Mon, 16 Jun 2008, Lea Wiemann wrote:
> Jakub Narebski wrote:
>>
>> Well, we could, perhaps, examine stderr (or redirect it to stdout and
>> examine upon error) to check what was the error.
>
> We don't have to -- gitweb's current (suboptimal) error checking is
> because it doesn't interface with git very well.
The "examine stderr" was a bit tongue-in-cheek, i.e. solution which
would require least changes... but I guess very impractical.
> The API I'm writing
> will fix this (i.e. provide proper feedback in all cases) so we'll have
> more specific status codes. IOW, we'll be able to differentiate between
> 500 and 404. Trust me on this one. ;-)
I have thought that Git.pm API together with catching (and examining)
Error, perhaps with redirecting STDERR somewhere (but it would be best
if it would not be needed) would be enough.
>> But I think in all, or almost all cases, the source is wrong parameters
>> in URL. Now, returning 5xx _server_ error would make me want to email
>> webmaster about error on his/her server, while 4xx _user_ error would
>> make me examine my input
>
> Since the status codes will get better (more accurate) anyway, I care
> more about correctness than practicalities right now (and I'm convinced
> that only 500 is correct in the cases we're talking about). That said,
> if you really want 404s in there, go ahead and send a follow-up patch, I
> won't object.
If the source of error is some misconfiguration on server, then 5xx is
appropriate (for example git binary is not found, something which
perhaps we should check upfront at the beginning). But I think it
should be very, very rare, and result of misconfigured gitweb, or error
installing git... or corrupted repository.
If source of error is mistake in URL, I would certainly want 4xx error.
So the user knows that he/she has to look at the URL.
That said, perhaps I am worrying over nothing, and
or die_error(undef, "Open <git command> failed");
can happen only on some serious server error (like corrupted
repository).
>From RFC 2616 (http://tools.ietf.org/html/rfc2616)
10.4 Client Error 4xx
The 4xx class of status code is intended for cases in which the
client seems to have erred.
[...]
10.5 Server Error 5xx
Response status codes beginning with the digit "5" indicate cases in
which the server is aware that it has erred or is incapable of
performing the request.
>> BTW. I got three copies of this email: was it you fighting VGER
>> anti-spam filter?
>
> Yup. Apparently it simply greps for Content-TypXe: text/hXtml. *shakes
> head* :-)
It is unfortunately very simple pattern based filter, not Markovian,
spam/ham Bayesian, or even simple Bayesian.
--
Jakub Narebski
Poland
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] gitweb: return correct HTTP status codes
2008-06-16 15:57 ` Lea Wiemann
2008-06-16 16:43 ` Jakub Narebski
@ 2008-06-16 22:38 ` Junio C Hamano
2008-06-17 14:04 ` Lea Wiemann
2008-06-16 23:37 ` Jakub Narebski
2 siblings, 1 reply; 25+ messages in thread
From: Junio C Hamano @ 2008-06-16 22:38 UTC (permalink / raw)
To: Lea Wiemann; +Cc: Jakub Narebski, git
Lea Wiemann <lewiemann@gmail.com> writes:
> Jakub Narebski wrote:
>...
>>> I recommend that you apply [PATCH] gitweb: remove git_blame
>>
>> I think it is not possible, as IIRC [it] got already applied.
>
> It isn't applied on master (at the time I'm writing this), only on next.
> Should I be working on the next branch?
I think it was sensible for you to mention the patch dependency, and if
you said the same thing with a different phrasing "this depends on remove
git_blame patch" you wouldn't have opened yourself up to such a nitpick
;-)
Since you are working on something that is outside the current 1.5.6
freeze, and removal of the unused "other" blame sub was something nobody
seemed to have objected to, I think it is Ok to assume that the removal
will happen when this new code is ready and after 1.5.6 ships.
It's up to you to work on 'next' or 'master with extra patches'. Just
state what you are basing your development on and with what extra patches
if there is a risk of confusion among your readers.
>>> open my $fd, "-|", git_cmd(), "ls-tree", $base, "--", $path
>>> - or die_error(undef, "Open git-ls-tree failed");
>>> + or die_error(500, "Open git-ls-tree failed");
>>
>> Should we really use "500 Internal Server Error" here? Usually this
>> would be not an error at all, but wrong parameters to git command,
>> i.e. it is _user_ who erred, not some error on _server_ side.
>
> You cannot tell for sure -- all you know here is that the command
> somehow failed when it shouldn't have, and so all you can give is 500;
> see below. I don't think we should apply reasoning like "most commonly
> it's a wrong hash, so let's return 404" -- we don't know, and we
> shouldn't assume.
>
>>> ... I suspect that the error could be triggered by non-existent hashes
>>> as well, in which case the more specific 404 would be more appropriate
>>> -- however, the current implementation oftentimes doesn't allow for
>>> more fine-granulared checking
>>
>> ...that is why I'd rather have "403 Forbidden" as catch-all error, as
>> it was done in gitweb. But that also seems not very good idea.
>
> [IANA HTTP expert, but:] All we have in cases like the one above is "I
> cannot deliver the content you requested, but I'm so confused that I
> don't even know why" -- which sounds like 500 to me (definitely not
> 403). 403 would be used for some deactivated feature (commonly for
> disabled directory listings I think) or something that shouldn't be
> accessible to the public.
Hmph... if we cared deeply enough about this issue, isn't it possible for
you to unconfuse yourself in a slow path and figure out exactly why it
failed?
unless (open $fd, '-|', ls-tree $base -- $path) {
# Oh, an error? why?
if (!object_exists($base)) {
die_error(404, "No such object $base");
} elsif (!is_a_treeish($base)) {
die_error(404, "No such tree-ish $base");
} else {
die_error();
}
}
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] gitweb: return correct HTTP status codes
2008-06-16 15:57 ` Lea Wiemann
2008-06-16 16:43 ` Jakub Narebski
2008-06-16 22:38 ` Junio C Hamano
@ 2008-06-16 23:37 ` Jakub Narebski
2 siblings, 0 replies; 25+ messages in thread
From: Jakub Narebski @ 2008-06-16 23:37 UTC (permalink / raw)
To: Lea Wiemann; +Cc: git
On Mon, 16 Jun 2008, Lea Wiemann wrote:
> Jakub Narebski wrote:
>>
>> I'd like to have reasoning when, and why, we use which responses (HTTP
>> status codes) in which situations. Link to appropriate RFC you used
>> (and perhaps to some other information)
>
> RFC 2616 has clear descriptions of the codes we use.
The RFC number was not mentioned in commit message, if I remember
correctly. URL like http://tools.ietf.org/html/rfc2616 would also
be nice (especially when/if gitweb finally acquires committags
support).
> Maybe a really short list of conventions though, like you listed
> below?
That would be nice.
Perhaps it should be organized differently, i.e. start with different
HTTP error status codes, and when they are used (situations), and not
with categories of situations and poposed HTTP codes.
> If we have agreement I'll happily put it in.
Even if there is no agreement, it is something that can be discussed
when talking about given patch sent.
>>> For convenience the die_error function now only takes the status code
>>> without reason as first parameter
>>
>> Well, gitweb sometimes used different wording
>
> The 'reason' only appeared in the HTTP response header -- see below.
I'd rather have explanation in HTTP resonse header, not only pure
number. For example Test::WWW::Mechanize::CGI shows both code
and _description_ when there is error (for get_ok).
>>> open my $fd, "-|", git_cmd(), "ls-tree", $base, "--", $path
>>> - or die_error(undef, "Open git-ls-tree failed");
>>> + or die_error(500, "Open git-ls-tree failed");
>>
>> Should we really use "500 Internal Server Error" here? Usually this
>> would be not an error at all, but wrong parameters to git command,
>> i.e. it is _user_ who erred, not some error on _server_ side.
>
> You cannot tell for sure -- all you know here is that the command
> somehow failed when it shouldn't have, and so all you can give is 500;
> see below. I don't think we should apply reasoning like "most commonly
> it's a wrong hash, so let's return 404" -- we don't know, and we
> shouldn't assume.
I think that in this situation, "open or die_error(...)", if error can
happen at all, it can happen only because of some serious error, like
git not installed[*1*], or corrupted repository, or something.
We would have to examine "close or die_error(...)" to check (perhaps
like Junio proposed examining details in a slow path, perhaps catching
and examining Error from Git.pm methods, perhaps examining stderr)
whether this is user error with 4xx HTTP status code, or server error
with 5xx HTTP status code.
[*1*] Perhaps we should have upfront in gitweb something like
unless (-x "$GIT")
die_error("500 Internal Server Error", 'Git binary not found');
or something like that. Here it is _server_ error, and you should mail
webmaster / web page admin, and not examine your URL as in case of
_user_ error of 4xx variety.
>>> ... I suspect that the error could be triggered by non-existent hashes
>>> as well, in which case the more specific 404 would be more appropriate
>>> -- however, the current implementation oftentimes doesn't allow for
>>> more fine-granulared checking
>>
>> ...that is why I'd rather have "403 Forbidden" as catch-all error, as
>> it was done in gitweb. But that also seems not very good idea.
>
> [IANA HTTP expert, but:] All we have in cases like the one above is "I
> cannot deliver the content you requested, but I'm so confused that I
> don't even know why" -- which sounds like 500 to me (definitely not
> 403). 403 would be used for some deactivated feature (commonly for
> disabled directory listings I think) or something that shouldn't be
> accessible to the public.
I think that "404 Not Found" would be better catch-all than
"500 Internal Server Error"... unless we are sure or almost sure that
it _is_ server error.
>>> sub die_error {
>>> - my $status = shift || "403 Forbidden";
>>> - my $error = shift || "Malformed query, file missing or permission denied";
>>> + my $status = shift || 500;
>>> + my $error = shift || "Internal server error";
>>
>> Errr, I think "Malformed query, file missing or permission denied" is
>> actually closer to truth, and better error message
>
> This default is intended for expressions like "some_action(...) or
> die_error" (without parameters), in which case you really can't tell
> what triggered the error, so "Internal server error" is as specific as
> you can get.
I think that most errors are of the above... but I guess that it is
a moot point, because even if status sometimes is taken default,
I don't think there is callsite that makes use of default for $error,
i.e. second argument of die_error() is always set explicitely.
>> (it is displayed in body of message)
>
> Just to make sure we're on the same boat, the $error variable above *is*
> what's displayed in the body.
True.
>>> + # Use a generic "Error" reason, e.g. "404 Error" instead of
>>> + # "404 Not Found". This is permissible by RFC 2616.
>>> + git_header_html("$status Error");
>>
>> I don't think it is _recommended_ practice. IIRC it is recommnded
>> [...] to give more detailed explanation in the body of message.
>
> The code above prints the HTTP response headers, not the body -- the
> body contains the detailed error message passed to the die_error
> function. Here's a stripped down version of gitweb's error response
> (X'ed out against spam filter):
>
> HTXTP/1.0 404 Error <------------ "Error" here
> CoXntent-Type: text/htXml
>
> ... 404 - Hash not found ...
>
> I don't know of *any* browser or tool that actually displays the reason
> given in the HTTP response header, so defaulting to Error is fine here
Test::WWW::Mechanize displays both numeric HTTP response *and* reason
given in HTTP response header when get_ok(...) fails.
Besides we would want some explanation also on HEAD reuests (supported
now only for web feeds, IIRC).
> (especially since RFC 2616 explicitly allow it).
Could you point me to where it is? And no, I don't think that
"The reason phrases listed here are only recommendations -- they MAY
be replaced by local equivalents without affecting the protocol."
is it.
> (I don't want to put the $error passed to die_error into the HTTP
> response header, since badly interpolated strings might invalidate the
> response. So using "Error" makes sure we always die gracefully.)
Errr... I think we can trust gitweb programmers to not screw up.
Perhaps there should be comment that first argument should be plain
text, while second can be HTML fragment.
>> Proposed HTTP server/client error status codes [heavily snipped]:
Let us put it in different order.
1. 4xx: Client Error - The request contains bad syntax or cannot
be fulfilled
1.1. 400 Bad Request - The request could not be understood by the
server due to malformed syntax.
Here I think we should put all the cases where we know that URL
is invalid even without examining any data, after purely
syntaxical checking. This means params with values outside their
domain, like for example $page ('pg') parameter which is not
a number.
It *might* be also used for some unreasonable discrepancies
in parameters which need access to git repository to check, like
asking for blame view of non-blob... although this could also be
"404 Not Found".
1.2. 403 Forbidden - The server understood the request, but is refusing
to fulfill it. Authorization will not help (...)
(Sometimes as "403 Permission Denied").
Here I think we should put all the cases where access is denied
because of gitweb or repo configuration; this I think does not
include non-exported projects. This usually means some 'feature'
disabled (not enabled).
1.3. 404 Not Found - The server has not found anything matching the
Request-URI. No indication is given of whether the condition
is temporary or permanent. (...) This status code is commonly
used when the server does not wish to reveal exactly why the
request has been refused, or when no other response is
applicable.
Here we should put all the situations where we ask for commit, and
does not get it, as for a project, and such project does not exist,
ask for a path, and there is no such path in specified tree-ish,
asking for a tag while there is nothing under given name, etc.
It could also be a reasonable catch-all.
2. 5xx: Server Error - The server failed to fulfill an apparently
valid request
2.1. 500 Internal Server Error - The server encountered an unexpected
condition which prevented it from fulfilling the request.
Here I think we can put problems with git binaries, corrupted
repositories, some Perl error etc. Something you should mail
webmaster (web admin) about.
All "open or die_error(...)" should I guess go there, although
I'm not sure when/if that can happen. About "close or die...":
those ones would need carefull examination if they should result
in 404 or in 500 error code zone.
2.2. 503 Service Unavailable - The server is currently unable to handle
the request due to a temporary overloading or maintenance of
the server. The implication is that this is a temporary
condition which will be alleviated after some delay.
Perhaps during longer regenerating cache this should be
response...
--
Jakub Narebski
ShadeHawk on #git
Poland
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] gitweb: return correct HTTP status codes
2008-06-16 22:34 ` Jakub Narebski
@ 2008-06-17 13:53 ` Lea Wiemann
0 siblings, 0 replies; 25+ messages in thread
From: Lea Wiemann @ 2008-06-17 13:53 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
Jakub Narebski wrote:
> I have thought that Git.pm API together with catching (and examining)
> Error, perhaps with redirecting STDERR somewhere (but it would be best
> if it would not be needed) would be enough.
(Just to clarify, I'm not touching Git.pm with my current work; I'm
adding a few Git::* modules instead, and they work differently from
Git.pm. So there's no Error, and there won't be any STDERR to catch at
all.)
-- Lea
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] gitweb: return correct HTTP status codes
2008-06-16 22:38 ` Junio C Hamano
@ 2008-06-17 14:04 ` Lea Wiemann
2008-06-17 14:33 ` Jakub Narebski
0 siblings, 1 reply; 25+ messages in thread
From: Lea Wiemann @ 2008-06-17 14:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jakub Narebski, git
Junio C Hamano wrote:
> It's up to you to work on 'next' or 'master with extra patches'.
Okay; I guess I'll use 'next with extra patches'. ;-)
> [Gitweb's error handling:] isn't it possible for you to unconfuse
> yourself in a slow path and figure out exactly why it failed?
>
> unless (open $fd, '-|', ls-tree $base -- $path) {
> # Oh, an error? why?
> if (!object_exists($base)) { [...]
> } elsif (!is_a_treeish($base)) { [...]
That's possible, but the API I'm writing is designed the other way
round: First, get the hash & type of $base; if it fails or the type is
wrong, die accordingly. Then pass the hash you got into whatever call
to git, and if that fails, you can quite safely assume that something
serious went wrong. (The example above has an additional $path to deal
with, but you get the idea.)
IOW, the strategy is "don't let the git binaries resolve any object
names for you", which should make both error handling and caching a lot
easier.
-- Lea
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] gitweb: return correct HTTP status codes
2008-06-17 14:04 ` Lea Wiemann
@ 2008-06-17 14:33 ` Jakub Narebski
2008-06-17 22:28 ` Lea Wiemann
0 siblings, 1 reply; 25+ messages in thread
From: Jakub Narebski @ 2008-06-17 14:33 UTC (permalink / raw)
To: Lea Wiemann; +Cc: Junio C Hamano, git
Lea Wiemann wrote:
> Junio C Hamano wrote:
> > [Gitweb's error handling:] isn't it possible for you to unconfuse
> > yourself in a slow path and figure out exactly why it failed?
> >
> > unless (open $fd, '-|', ls-tree $base -- $path) {
> > # Oh, an error? why?
> > if (!object_exists($base)) { [...]
> > } elsif (!is_a_treeish($base)) { [...]
BTW. you can catch such errors on close(), not on open(), my mistake.
On open you can catch only fairly fatal errors (5xx category I think).
> That's possible, but the API I'm writing is designed the other way
> round: First, get the hash & type of $base; if it fails or the type is
> wrong, die accordingly. Then pass the hash you got into whatever call
> to git, and if that fails, you can quite safely assume that something
> serious went wrong. (The example above has an additional $path to deal
> with, but you get the idea.)
>
> IOW, the strategy is "don't let the git binaries resolve any object
> names for you", which should make both error handling and caching a lot
> easier.
But that means checking arguments in the "fast path", which means
additional calls to git commands in the _common_ case, not only in
the case of errors. I think that even with caching it is not a good
idea.
I'll try to come with example implementation using Error.pm and Git.pm
--
Jakub Narebski
Poland
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] gitweb: return correct HTTP status codes
2008-06-17 14:33 ` Jakub Narebski
@ 2008-06-17 22:28 ` Lea Wiemann
2008-06-17 22:54 ` Jakub Narebski
0 siblings, 1 reply; 25+ messages in thread
From: Lea Wiemann @ 2008-06-17 22:28 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Junio C Hamano, git
Jakub Narebski wrote:
> But that means checking arguments in the "fast path", which means
> additional calls to git commands in the _common_ case, not only in
> the case of errors.
No, it doesn't, it just pipes stuff into cat-file --batch-check, which
has to be opened on virtually any call to gitweb. Before telling me
about the performance of my code, can you please (a) read it and (b)
benchmark it? We lose a lot of time on pointless discussion otherwise.
> I'll try to come with example implementation using Error.pm and Git.pm
I don't think it's worth the time; I think I understand your point
without an example implementation. :)
Unrelatedly, I thought we had agreement not to use Error.pm as of
<http://thread.gmane.org/gmane.comp.version-control.git/83267/focus=83348>
(though Git.pm still does use Error.pm, so it can still be used as long
as there's no patch that removes all instances of it). I got *really*
weird unreproduceable errors when throwing exceptions with Error.pm in
gitweb, and I suspect that Error.pm (perhaps in interaction with some
other module) was to blame. Just a warning. ;-)
-- Lea
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] gitweb: return correct HTTP status codes
2008-06-17 22:28 ` Lea Wiemann
@ 2008-06-17 22:54 ` Jakub Narebski
2008-06-17 23:47 ` Lea Wiemann
0 siblings, 1 reply; 25+ messages in thread
From: Jakub Narebski @ 2008-06-17 22:54 UTC (permalink / raw)
To: Lea Wiemann; +Cc: Junio C Hamano, git
Lea Wiemann wrote:
> Jakub Narebski wrote:
> >
> > But that means checking arguments in the "fast path", which means
> > additional calls to git commands in the _common_ case, not only in
> > the case of errors.
>
> No, it doesn't, it just pipes stuff into cat-file --batch-check,
Ah, O.K., it does add additional call to git command, which should not
matter much performance wise on sane operating systems; it would matter
on OS with slow fork, like MS Windows, if gitweb ran on Windows
(perhaps it can, but certainly not with ActiveState Perl, IIRC).
But what are arguments for "check params; run command" vs "run command;
check params if error" proposed by Junio? Why do you want to check
parameters upfront? Does it make code much, much easier?
> which has to be opened on virtually any call to gitweb.
IIRC it is for checking parameters? Even then, using it only on "slow
patch", i.e. in preence of error might be better solution.
Or is it needed for something else too?
> Before telling me about the performance of my code, can you please
> (a) read it [...]
By the way, would you be sending your current WIP for review?
P.S. I wanted to ask in another subthread if adding object oriented
interface (wrapper) to git repositories (similar to the one used by
StGIT / git-python perhaps?) is really needed for implementing gitweb
caching?; I still think that you have to chose which spots to cache,
and do it from gitweb, and not cache everything. But if it is needed
for sane error reporting, and perhaps better ETags...
--
Jakub Narebski
Poland
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] gitweb: return correct HTTP status codes
2008-06-17 22:54 ` Jakub Narebski
@ 2008-06-17 23:47 ` Lea Wiemann
2008-06-18 0:12 ` Jakub Narebski
0 siblings, 1 reply; 25+ messages in thread
From: Lea Wiemann @ 2008-06-17 23:47 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Junio C Hamano, git
Jakub Narebski wrote:
> But what are arguments for "check params; run command" vs "run command;
> check params if error" proposed by Junio? Why do you want to check
> parameters upfront?
It's actually not checking, it's resolving. Instead of ...
get_commit($symbol)
... (with $symbol = 'HEAD' for instance), you do (pseudocode):
$hash = get_hash($symbol, 'commit'); # 'commit' to resolve tags
check that $hash is defined
get_commit($hash)
(And get_commit won't even accept anything but 40-byte hashes.) This is
for two reasons:
1. Caching: Resolving symbols first gives you some (very few) cache
entries that need to be expired (namely, get_hash results for symbols
that are not SHA1 hashes already), but most cache entries (like the
get_commit) are infinitely valid.
2. Besides being a little more straightforward to implement, it ensures
that you have well-defined failure points. IOW, first you resolve the
symbols you're getting from the user and check them for existence (and
correct type). From then on, any hashes you pass around are guaranteed
to be valid, so failures indicate that something serious went wrong.
Apart from making things easier for the developer by reminding them of
the points where they *need* to check for errors, it means that you can
very easily check the error-handling code for completeness (by only
reading the first few lines that resolve symbols).
> By the way, would you be sending your current WIP for review?
Will do in the next 2 days after some cleanup.
> [is] adding object oriented interface (wrapper) to git repositories
> really needed for implementing gitweb caching?
It makes things better to read for sure, and it means that you can have
a plain API without caching, and implement the caching layer as a
subclass (which overrides the methods with cacheable results).
-- Lea
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] gitweb: return correct HTTP status codes
2008-06-17 23:47 ` Lea Wiemann
@ 2008-06-18 0:12 ` Jakub Narebski
2008-06-18 1:25 ` Lea Wiemann
0 siblings, 1 reply; 25+ messages in thread
From: Jakub Narebski @ 2008-06-18 0:12 UTC (permalink / raw)
To: Lea Wiemann; +Cc: Junio C Hamano, git
Lea Wiemann wrote:
> Jakub Narebski wrote:
> >
> > But what are arguments for "check params; run command" vs "run command;
> > check params if error" proposed by Junio? Why do you want to check
> > parameters upfront?
>
> It's actually not checking, it's resolving. Instead of ...
>
> get_commit($symbol)
>
> ... (with $symbol = 'HEAD' for instance), you do (pseudocode):
>
> $hash = get_hash($symbol, 'commit'); # 'commit' to resolve tags
Errr... is there equivalent to ^{}, i.e. resolve to non-tag?
> check that $hash is defined
> get_commit($hash)
Note that you would have to examine gitweb sources to check if it
uses href(..., -replay=>1) when it should, so the links from "volatile"
link, e.g. HEAD version URL, also lead to appropriate version,
e.g. "tree" from current / HEAD version.
> (And get_commit won't even accept anything but 40-byte hashes.) This is
> for two reasons:
>
> 1. Caching: Resolving symbols first gives you some (very few) cache
> entries that need to be expired (namely, get_hash results for symbols
> that are not SHA1 hashes already), but most cache entries (like the
> get_commit) are infinitely valid.
BTW. one of earliest idea was to fully resolve hashes, add missing
parameters if possible (like 'h', 'hp', 'f') and convert hashes to
sha-1. One of intended uses was (weak) ETag for simple HTTP caching.
But it was before git-cat-file --batch-check, and before
href(...,-replay=>1).
[...]
Thanks for explanation. It makes clear why you have chosen this
solution (I think that (1) is deciding factor here).
> > By the way, would you be sending your current WIP for review?
>
> Will do in the next 2 days after some cleanup.
Thanks.
I have read your current code, but I'd rather not comment on it
(like no_plan and skipping tests, or elaborate wrapping fields
vs. perltie) until it is ready, i.e. sent for peer review.
> > [is] adding object oriented interface (wrapper) to git repositories
> > really needed for implementing gitweb caching?
>
> It makes things better to read for sure, and it means that you can have
> a plain API without caching, and implement the caching layer as a
> subclass (which overrides the methods with cacheable results).
All the time I think that caching _everything_ is a bad solution.
Besides sometimes you need to cache information specific for gitweb,
or information _aggregated_ by gitweb, which doesn't have place as
single entity in Git::Repo.
So while I think that Git::Repo and making gitweb use it is
a worthwhile goal, I think caching should be explicitely in gitweb.
See CGI::Cache for good solution of inobtrusive output caching, and
CHI (or other in recommended thread) for inobtrusive data caching
using callbacks (and Cache::None engine). That not said that we
should use such non-standard Perl modules!
<probably should not send email this late>
--
Jakub Narebski
Poland
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH] gitweb: standarize HTTP status codes
2008-06-15 21:15 [PATCH] gitweb: return correct HTTP status codes Lea Wiemann
2008-06-15 22:48 ` Jakub Narebski
@ 2008-06-18 0:15 ` Lea Wiemann
2008-06-19 0:51 ` [PATCH v2] " Jakub Narebski
1 sibling, 1 reply; 25+ messages in thread
From: Lea Wiemann @ 2008-06-18 0:15 UTC (permalink / raw)
To: git; +Cc: Lea Wiemann
Many error status codes simply default to 403 Forbidden, which is not
correct in most cases. This patch makes gitweb return semantically
correct status codes.
For convenience the die_error function now only takes the status code
without reason as first parameter (e.g. 404 instead of "404 Not
Found"), and it now defaults to 500 (Internal Server Error), even
though the default is not used anywhere.
Also documented status code conventions in die_error.
Signed-off-by: Lea Wiemann <LeWiemann@gmail.com>
---
Changes since v1:
- Changed subject per Jakub's suggestion.
- Resolved merge conflict caused by "[PATCH v2] gitweb: quote commands
properly when calling the shell".
- Added the following documentation to die_error (I know Jakub might
disagree with me on the last line ;-)):
# die_error(<http_status_code>, <error_message>)
# Example: die_error(404, 'Hash not found')
# By convention, use the following status codes (as defined in RFC 2616):
# 400: Invalid or missing CGI parameters, or
# requested object exists but has wrong type.
# 403: Requested feature (like "pickaxe" or "snapshot") not enabled on
# this server or project.
# 404: Requested object/revision/project doesn't exist.
# 500: The server isn't configured properly, or
# an internal error occurred (e.g. failed assertions caused by bugs), or
# an unknown error occurred (e.g. the git binary died unexpectedly).
gitweb/gitweb.perl | 207 ++++++++++++++++++++++++++--------------------------
1 files changed, 102 insertions(+), 105 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 3a7adae..3ca45fd 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -386,7 +386,7 @@ $projects_list ||= $projectroot;
our $action = $cgi->param('a');
if (defined $action) {
if ($action =~ m/[^0-9a-zA-Z\.\-_]/) {
- die_error(undef, "Invalid action parameter");
+ die_error(400, "Invalid action parameter");
}
}
@@ -399,21 +399,21 @@ if (defined $project) {
($export_ok && !(-e "$projectroot/$project/$export_ok")) ||
($strict_export && !project_in_list($project))) {
undef $project;
- die_error(undef, "No such project");
+ die_error(404, "No such project");
}
}
our $file_name = $cgi->param('f');
if (defined $file_name) {
if (!validate_pathname($file_name)) {
- die_error(undef, "Invalid file parameter");
+ die_error(400, "Invalid file parameter");
}
}
our $file_parent = $cgi->param('fp');
if (defined $file_parent) {
if (!validate_pathname($file_parent)) {
- die_error(undef, "Invalid file parent parameter");
+ die_error(400, "Invalid file parent parameter");
}
}
@@ -421,21 +421,21 @@ if (defined $file_parent) {
our $hash = $cgi->param('h');
if (defined $hash) {
if (!validate_refname($hash)) {
- die_error(undef, "Invalid hash parameter");
+ die_error(400, "Invalid hash parameter");
}
}
our $hash_parent = $cgi->param('hp');
if (defined $hash_parent) {
if (!validate_refname($hash_parent)) {
- die_error(undef, "Invalid hash parent parameter");
+ die_error(400, "Invalid hash parent parameter");
}
}
our $hash_base = $cgi->param('hb');
if (defined $hash_base) {
if (!validate_refname($hash_base)) {
- die_error(undef, "Invalid hash base parameter");
+ die_error(400, "Invalid hash base parameter");
}
}
@@ -447,10 +447,10 @@ our @extra_options = $cgi->param('opt');
if (defined @extra_options) {
foreach my $opt (@extra_options) {
if (not exists $allowed_options{$opt}) {
- die_error(undef, "Invalid option parameter");
+ die_error(400, "Invalid option parameter");
}
if (not grep(/^$action$/, @{$allowed_options{$opt}})) {
- die_error(undef, "Invalid option parameter for this action");
+ die_error(400, "Invalid option parameter for this action");
}
}
}
@@ -458,7 +458,7 @@ if (defined @extra_options) {
our $hash_parent_base = $cgi->param('hpb');
if (defined $hash_parent_base) {
if (!validate_refname($hash_parent_base)) {
- die_error(undef, "Invalid hash parent base parameter");
+ die_error(400, "Invalid hash parent base parameter");
}
}
@@ -466,14 +466,14 @@ if (defined $hash_parent_base) {
our $page = $cgi->param('pg');
if (defined $page) {
if ($page =~ m/[^0-9]/) {
- die_error(undef, "Invalid page parameter");
+ die_error(400, "Invalid page parameter");
}
}
our $searchtype = $cgi->param('st');
if (defined $searchtype) {
if ($searchtype =~ m/[^a-z]/) {
- die_error(undef, "Invalid searchtype parameter");
+ die_error(400, "Invalid searchtype parameter");
}
}
@@ -483,7 +483,7 @@ our $searchtext = $cgi->param('s');
our $search_regexp;
if (defined $searchtext) {
if (length($searchtext) < 2) {
- die_error(undef, "At least two characters are required for search parameter");
+ die_error(403, "At least two characters are required for search parameter");
}
$search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;
}
@@ -580,11 +580,11 @@ if (!defined $action) {
}
}
if (!defined($actions{$action})) {
- die_error(undef, "Unknown action");
+ die_error(400, "Unknown action");
}
if ($action !~ m/^(opml|project_list|project_index)$/ &&
!$project) {
- die_error(undef, "Project needed");
+ die_error(400, "Project needed");
}
$actions{$action}->();
exit;
@@ -1665,7 +1665,7 @@ sub git_get_hash_by_path {
$path =~ s,/+$,,;
open my $fd, "-|", git_cmd(), "ls-tree", $base, "--", $path
- or die_error(undef, "Open git-ls-tree failed");
+ or die_error(500, "Open git-ls-tree failed");
my $line = <$fd>;
close $fd or return undef;
@@ -2127,7 +2127,7 @@ sub parse_commit {
"--max-count=1",
$commit_id,
"--",
- or die_error(undef, "Open git-rev-list failed");
+ or die_error(500, "Open git-rev-list failed");
%co = parse_commit_text(<$fd>, 1);
close $fd;
@@ -2152,7 +2152,7 @@ sub parse_commits {
$commit_id,
"--",
($filename ? ($filename) : ())
- or die_error(undef, "Open git-rev-list failed");
+ or die_error(500, "Open git-rev-list failed");
while (my $line = <$fd>) {
my %co = parse_commit_text($line);
push @cos, \%co;
@@ -2672,11 +2672,24 @@ sub git_footer_html {
"</html>";
}
+# die_error(<http_status_code>, <error_message>)
+# Example: die_error(404, 'Hash not found')
+# By convention, use the following status codes (as defined in RFC 2616):
+# 400: Invalid or missing CGI parameters, or
+# requested object exists but has wrong type.
+# 403: Requested feature (like "pickaxe" or "snapshot") not enabled on
+# this server or project.
+# 404: Requested object/revision/project doesn't exist.
+# 500: The server isn't configured properly, or
+# an internal error occurred (e.g. failed assertions caused by bugs), or
+# an unknown error occurred (e.g. the git binary died unexpectedly).
sub die_error {
- my $status = shift || "403 Forbidden";
- my $error = shift || "Malformed query, file missing or permission denied";
+ my $status = shift || 500;
+ my $error = shift || "Internal server error";
- git_header_html($status);
+ # Use a generic "Error" reason, e.g. "404 Error" instead of
+ # "404 Not Found". This is permissible by RFC 2616.
+ git_header_html("$status Error");
print <<EOF;
<div class="page_body">
<br /><br />
@@ -3924,12 +3937,12 @@ sub git_search_grep_body {
sub git_project_list {
my $order = $cgi->param('o');
if (defined $order && $order !~ m/none|project|descr|owner|age/) {
- die_error(undef, "Unknown order parameter");
+ die_error(400, "Unknown order parameter");
}
my @list = git_get_projects_list();
if (!@list) {
- die_error(undef, "No projects found");
+ die_error(404, "No projects found");
}
git_header_html();
@@ -3947,12 +3960,12 @@ sub git_project_list {
sub git_forks {
my $order = $cgi->param('o');
if (defined $order && $order !~ m/none|project|descr|owner|age/) {
- die_error(undef, "Unknown order parameter");
+ die_error(400, "Unknown order parameter");
}
my @list = git_get_projects_list($project);
if (!@list) {
- die_error(undef, "No forks found");
+ die_error(404, "No forks found");
}
git_header_html();
@@ -4081,7 +4094,7 @@ sub git_tag {
my %tag = parse_tag($hash);
if (! %tag) {
- die_error(undef, "Unknown tag object");
+ die_error(404, "Unknown tag object");
}
git_print_header_div('commit', esc_html($tag{'name'}), $hash);
@@ -4117,26 +4130,25 @@ sub git_blame {
my $fd;
my $ftype;
- my ($have_blame) = gitweb_check_feature('blame');
- if (!$have_blame) {
- die_error('403 Permission denied', "Permission denied");
- }
- die_error('404 Not Found', "File name not defined") if (!$file_name);
+ gitweb_check_feature('blame')
+ or die_error(403, "Blame not allowed");
+
+ die_error(400, "No file name given") unless $file_name;
$hash_base ||= git_get_head_hash($project);
- die_error(undef, "Couldn't find base commit") unless ($hash_base);
+ die_error(400, "Couldn't find base commit") unless ($hash_base);
my %co = parse_commit($hash_base)
- or die_error(undef, "Reading commit failed");
+ or die_error(404, "Commit not found");
if (!defined $hash) {
$hash = git_get_hash_by_path($hash_base, $file_name, "blob")
- or die_error(undef, "Error looking up file");
+ or die_error(404, "Error looking up file");
}
$ftype = git_get_type($hash);
if ($ftype !~ "blob") {
- die_error('400 Bad Request', "Object is not a blob");
+ die_error(400, "Object is not a blob");
}
open ($fd, "-|", git_cmd(), "blame", '-p', '--',
$file_name, $hash_base)
- or die_error(undef, "Open git-blame failed");
+ or die_error(500, "Open git-blame failed");
git_header_html();
my $formats_nav =
$cgi->a({-href => href(action=>"blob", -replay=>1)},
@@ -4198,7 +4210,7 @@ HTML
print "</td>\n";
}
open (my $dd, "-|", git_cmd(), "rev-parse", "$full_rev^")
- or die_error(undef, "Open git-rev-parse failed");
+ or die_error(500, "Open git-rev-parse failed");
my $parent_commit = <$dd>;
close $dd;
chomp($parent_commit);
@@ -4255,9 +4267,9 @@ sub git_blob_plain {
if (defined $file_name) {
my $base = $hash_base || git_get_head_hash($project);
$hash = git_get_hash_by_path($base, $file_name, "blob")
- or die_error(undef, "Error lookup file");
+ or die_error(404, "Cannot find file");
} else {
- die_error(undef, "No file name defined");
+ die_error(400, "No file name defined");
}
} elsif ($hash =~ m/^[0-9a-fA-F]{40}$/) {
# blobs defined by non-textual hash id's can be cached
@@ -4265,7 +4277,7 @@ sub git_blob_plain {
}
open my $fd, "-|", git_cmd(), "cat-file", "blob", $hash
- or die_error(undef, "Open git-cat-file blob '$hash' failed");
+ or die_error(500, "Open git-cat-file blob '$hash' failed");
# content-type (can include charset)
$type = blob_contenttype($fd, $file_name, $type);
@@ -4297,9 +4309,9 @@ sub git_blob {
if (defined $file_name) {
my $base = $hash_base || git_get_head_hash($project);
$hash = git_get_hash_by_path($base, $file_name, "blob")
- or die_error(undef, "Error lookup file");
+ or die_error(404, "Cannot find file");
} else {
- die_error(undef, "No file name defined");
+ die_error(400, "No file name defined");
}
} elsif ($hash =~ m/^[0-9a-fA-F]{40}$/) {
# blobs defined by non-textual hash id's can be cached
@@ -4308,7 +4320,7 @@ sub git_blob {
my ($have_blame) = gitweb_check_feature('blame');
open my $fd, "-|", git_cmd(), "cat-file", "blob", $hash
- or die_error(undef, "Couldn't cat $file_name, $hash");
+ or die_error(500, "Couldn't cat $file_name, $hash");
my $mimetype = blob_mimetype($fd, $file_name);
if ($mimetype !~ m!^(?:text/|image/(?:gif|png|jpeg)$)! && -B $fd) {
close $fd;
@@ -4389,9 +4401,9 @@ sub git_tree {
}
$/ = "\0";
open my $fd, "-|", git_cmd(), "ls-tree", '-z', $hash
- or die_error(undef, "Open git-ls-tree failed");
+ or die_error(500, "Open git-ls-tree failed");
my @entries = map { chomp; $_ } <$fd>;
- close $fd or die_error(undef, "Reading tree failed");
+ close $fd or die_error(500, "Reading tree failed");
$/ = "\n";
my $refs = git_get_references();
@@ -4481,16 +4493,16 @@ sub git_snapshot {
my $format = $cgi->param('sf');
if (!@supported_fmts) {
- die_error('403 Permission denied', "Permission denied");
+ die_error(403, "Snapshots not allowed");
}
# default to first supported snapshot format
$format ||= $supported_fmts[0];
if ($format !~ m/^[a-z0-9]+$/) {
- die_error(undef, "Invalid snapshot format parameter");
+ die_error(400, "Invalid snapshot format parameter");
} elsif (!exists($known_snapshot_formats{$format})) {
- die_error(undef, "Unknown snapshot format");
+ die_error(400, "Unknown snapshot format");
} elsif (!grep($_ eq $format, @supported_fmts)) {
- die_error(undef, "Unsupported snapshot format");
+ die_error(403, "Unsupported snapshot format");
}
if (!defined $hash) {
@@ -4518,7 +4530,7 @@ sub git_snapshot {
-status => '200 OK');
open my $fd, "-|", $cmd
- or die_error(undef, "Execute git-archive failed");
+ or die_error(500, "Execute git-archive failed");
binmode STDOUT, ':raw';
print <$fd>;
binmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi
@@ -4586,10 +4598,8 @@ sub git_log {
sub git_commit {
$hash ||= $hash_base || "HEAD";
- my %co = parse_commit($hash);
- if (!%co) {
- die_error(undef, "Unknown commit object");
- }
+ my %co = parse_commit($hash)
+ or die_error(404, "Unknown commit object");
my %ad = parse_date($co{'author_epoch'}, $co{'author_tz'});
my %cd = parse_date($co{'committer_epoch'}, $co{'committer_tz'});
@@ -4629,9 +4639,9 @@ sub git_commit {
@diff_opts,
(@$parents <= 1 ? $parent : '-c'),
$hash, "--"
- or die_error(undef, "Open git-diff-tree failed");
+ or die_error(500, "Open git-diff-tree failed");
@difftree = map { chomp; $_ } <$fd>;
- close $fd or die_error(undef, "Reading git-diff-tree failed");
+ close $fd or die_error(500, "Reading git-diff-tree failed");
# non-textual hash id's can be cached
my $expires;
@@ -4724,33 +4734,33 @@ sub git_object {
open my $fd, "-|", quote_command(
git_cmd(), 'cat-file', '-t', $object_id) . ' 2> /dev/null'
- or die_error('404 Not Found', "Object does not exist");
+ or die_error(404, "Object does not exist");
$type = <$fd>;
chomp $type;
close $fd
- or die_error('404 Not Found', "Object does not exist");
+ or die_error(404, "Object does not exist");
# - hash_base and file_name
} elsif ($hash_base && defined $file_name) {
$file_name =~ s,/+$,,;
system(git_cmd(), "cat-file", '-e', $hash_base) == 0
- or die_error('404 Not Found', "Base object does not exist");
+ or die_error(404, "Base object does not exist");
# here errors should not hapen
open my $fd, "-|", git_cmd(), "ls-tree", $hash_base, "--", $file_name
- or die_error(undef, "Open git-ls-tree failed");
+ or die_error(500, "Open git-ls-tree failed");
my $line = <$fd>;
close $fd;
#'100644 blob 0fa3f3a66fb6a137f6ec2c19351ed4d807070ffa panic.c'
unless ($line && $line =~ m/^([0-9]+) (.+) ([0-9a-fA-F]{40})\t/) {
- die_error('404 Not Found', "File or directory for given base does not exist");
+ die_error(404, "File or directory for given base does not exist");
}
$type = $2;
$hash = $3;
} else {
- die_error('404 Not Found', "Not enough information to find object");
+ die_error(400, "Not enough information to find object");
}
print $cgi->redirect(-uri => href(action=>$type, -full=>1,
@@ -4775,12 +4785,12 @@ sub git_blobdiff {
open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
$hash_parent_base, $hash_base,
"--", (defined $file_parent ? $file_parent : ()), $file_name
- or die_error(undef, "Open git-diff-tree failed");
+ or die_error(500, "Open git-diff-tree failed");
@difftree = map { chomp; $_ } <$fd>;
close $fd
- or die_error(undef, "Reading git-diff-tree failed");
+ or die_error(500, "Reading git-diff-tree failed");
@difftree
- or die_error('404 Not Found', "Blob diff not found");
+ or die_error(404, "Blob diff not found");
} elsif (defined $hash &&
$hash =~ /[0-9a-fA-F]{40}/) {
@@ -4789,23 +4799,23 @@ sub git_blobdiff {
# read filtered raw output
open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
$hash_parent_base, $hash_base, "--"
- or die_error(undef, "Open git-diff-tree failed");
+ or die_error(500, "Open git-diff-tree failed");
@difftree =
# ':100644 100644 03b21826... 3b93d5e7... M ls-files.c'
# $hash == to_id
grep { /^:[0-7]{6} [0-7]{6} [0-9a-fA-F]{40} $hash/ }
map { chomp; $_ } <$fd>;
close $fd
- or die_error(undef, "Reading git-diff-tree failed");
+ or die_error(500, "Reading git-diff-tree failed");
@difftree
- or die_error('404 Not Found', "Blob diff not found");
+ or die_error(404, "Blob diff not found");
} else {
- die_error('404 Not Found', "Missing one of the blob diff parameters");
+ die_error(400, "Missing one of the blob diff parameters");
}
if (@difftree > 1) {
- die_error('404 Not Found', "Ambiguous blob diff specification");
+ die_error(400, "Ambiguous blob diff specification");
}
%diffinfo = parse_difftree_raw_line($difftree[0]);
@@ -4826,7 +4836,7 @@ sub git_blobdiff {
'-p', ($format eq 'html' ? "--full-index" : ()),
$hash_parent_base, $hash_base,
"--", (defined $file_parent ? $file_parent : ()), $file_name
- or die_error(undef, "Open git-diff-tree failed");
+ or die_error(500, "Open git-diff-tree failed");
}
# old/legacy style URI
@@ -4862,9 +4872,9 @@ sub git_blobdiff {
open $fd, "-|", git_cmd(), "diff", @diff_opts,
'-p', ($format eq 'html' ? "--full-index" : ()),
$hash_parent, $hash, "--"
- or die_error(undef, "Open git-diff failed");
+ or die_error(500, "Open git-diff failed");
} else {
- die_error('404 Not Found', "Missing one of the blob diff parameters")
+ die_error(400, "Missing one of the blob diff parameters")
unless %diffinfo;
}
@@ -4897,7 +4907,7 @@ sub git_blobdiff {
print "X-Git-Url: " . $cgi->self_url() . "\n\n";
} else {
- die_error(undef, "Unknown blobdiff format");
+ die_error(400, "Unknown blobdiff format");
}
# patch
@@ -4932,10 +4942,8 @@ sub git_blobdiff_plain {
sub git_commitdiff {
my $format = shift || 'html';
$hash ||= $hash_base || "HEAD";
- my %co = parse_commit($hash);
- if (!%co) {
- die_error(undef, "Unknown commit object");
- }
+ my %co = parse_commit($hash)
+ or die_error(404, "Unknown commit object");
# choose format for commitdiff for merge
if (! defined $hash_parent && @{$co{'parents'}} > 1) {
@@ -5017,7 +5025,7 @@ sub git_commitdiff {
open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
"--no-commit-id", "--patch-with-raw", "--full-index",
$hash_parent_param, $hash, "--"
- or die_error(undef, "Open git-diff-tree failed");
+ or die_error(500, "Open git-diff-tree failed");
while (my $line = <$fd>) {
chomp $line;
@@ -5029,10 +5037,10 @@ sub git_commitdiff {
} elsif ($format eq 'plain') {
open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
'-p', $hash_parent_param, $hash, "--"
- or die_error(undef, "Open git-diff-tree failed");
+ or die_error(500, "Open git-diff-tree failed");
} else {
- die_error(undef, "Unknown commitdiff format");
+ die_error(400, "Unknown commitdiff format");
}
# non-textual hash id's can be cached
@@ -5115,19 +5123,15 @@ sub git_history {
$page = 0;
}
my $ftype;
- my %co = parse_commit($hash_base);
- if (!%co) {
- die_error(undef, "Unknown commit object");
- }
+ my %co = parse_commit($hash_base)
+ or die_error(404, "Unknown commit object");
my $refs = git_get_references();
my $limit = sprintf("--max-count=%i", (100 * ($page+1)));
my @commitlist = parse_commits($hash_base, 101, (100 * $page),
- $file_name, "--full-history");
- if (!@commitlist) {
- die_error('404 Not Found', "No such file or directory on given branch");
- }
+ $file_name, "--full-history")
+ or die_error(404, "No such file or directory on given branch");
if (!defined $hash && defined $file_name) {
# some commits could have deleted file in question,
@@ -5141,7 +5145,7 @@ sub git_history {
$ftype = git_get_type($hash);
}
if (!defined $ftype) {
- die_error(undef, "Unknown type of object");
+ die_error(500, "Unknown type of object");
}
my $paging_nav = '';
@@ -5179,19 +5183,16 @@ sub git_history {
}
sub git_search {
- my ($have_search) = gitweb_check_feature('search');
- if (!$have_search) {
- die_error('403 Permission denied', "Permission denied");
- }
+ gitweb_check_feature('search') or die_error(403, "Search is disabled");
if (!defined $searchtext) {
- die_error(undef, "Text field empty");
+ die_error(400, "Text field is empty");
}
if (!defined $hash) {
$hash = git_get_head_hash($project);
}
my %co = parse_commit($hash);
if (!%co) {
- die_error(undef, "Unknown commit object");
+ die_error(404, "Unknown commit object");
}
if (!defined $page) {
$page = 0;
@@ -5201,16 +5202,12 @@ sub git_search {
if ($searchtype eq 'pickaxe') {
# pickaxe may take all resources of your box and run for several minutes
# with every query - so decide by yourself how public you make this feature
- my ($have_pickaxe) = gitweb_check_feature('pickaxe');
- if (!$have_pickaxe) {
- die_error('403 Permission denied', "Permission denied");
- }
+ gitweb_check_feature('pickaxe')
+ or die_error(403, "Pickaxe is disabled");
}
if ($searchtype eq 'grep') {
- my ($have_grep) = gitweb_check_feature('grep');
- if (!$have_grep) {
- die_error('403 Permission denied', "Permission denied");
- }
+ gitweb_check_feature('grep')
+ or die_error(403, "Grep is disabled");
}
git_header_html();
@@ -5484,7 +5481,7 @@ sub git_feed {
# Atom: http://www.atomenabled.org/developers/syndication/
# RSS: http://www.notestips.com/80256B3A007F2692/1/NAMO5P9UPQ
if ($format ne 'rss' && $format ne 'atom') {
- die_error(undef, "Unknown web feed format");
+ die_error(400, "Unknown web feed format");
}
# log/feed of current (HEAD) branch, log of given branch, history of file/directory
--
1.5.6.rc3.7.ged9620
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH] gitweb: return correct HTTP status codes
2008-06-18 0:12 ` Jakub Narebski
@ 2008-06-18 1:25 ` Lea Wiemann
2008-06-18 7:35 ` Jakub Narebski
0 siblings, 1 reply; 25+ messages in thread
From: Lea Wiemann @ 2008-06-18 1:25 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Junio C Hamano, git
Jakub Narebski wrote:
> Lea Wiemann wrote:
>> $hash = get_hash($symbol, 'commit'); # 'commit' to resolve tags
>
> Errr... is there equivalent to ^{}, i.e. resolve to non-tag?
Yup. Haven't quite decided whether to simply use "$symbol^{type}" or
make type a separate parameter.
> Note that you would have to examine gitweb sources to check if it
> uses href(..., -replay=>1) when it should,
Good point, will do.
> BTW. one of earliest idea was to fully resolve hashes, add missing
> parameters if possible (like 'h', 'hp', 'f') and convert hashes to
> sha-1. One of intended uses was (weak) ETag for simple HTTP caching.
Interesting. Something to keep in mind is that using name-rev still can
wreck with this since it has the unique property of taking hashes but
still depending on the current refs. Gitweb isn't using name-rev a lot
right now, but that might change of course (e.g. I think that it would
be convenient to always display names along with any commit hashes).
> All the time I think that caching _everything_ is a bad solution.
So? We can easily add an option to the cache; e.g. no_cache =>
['get_blob', 'ls_tree']. I doubt that it will be needed, but if it
does, it's easy to add it. Don't worry about it, really.
> CHI (or other in recommended thread) for inobtrusive data caching
Thanks for the pointer! On the one hand CHI is very recent and not even
in Debian, on the other hand it provides things like busy_lock on top of
Memcached (AFAICS), at fairly little cost. I'll look into it.
-- Lea
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] gitweb: return correct HTTP status codes
2008-06-18 1:25 ` Lea Wiemann
@ 2008-06-18 7:35 ` Jakub Narebski
0 siblings, 0 replies; 25+ messages in thread
From: Jakub Narebski @ 2008-06-18 7:35 UTC (permalink / raw)
To: Lea Wiemann; +Cc: Junio C Hamano, git
Lea Wiemann wrote:
> Jakub Narebski wrote:
>> Lea Wiemann wrote:
>>>
>>> $hash = get_hash($symbol, 'commit'); # 'commit' to resolve tags
>>
>> Errr... is there equivalent to ^{}, i.e. resolve to non-tag?
>
> Yup. Haven't quite decided whether to simply use "$symbol^{type}" or
> make type a separate parameter.
Although I think this won't be necessary for gitweb, the ability
should (I guess) be in generic object-oriented interface like
Git::Repo module.
>> Note that you would have to examine gitweb sources to check if it
>> uses href(..., -replay=>1) when it should,
>
> Good point, will do.
It should, but I might have missed something.
>> BTW. one of earliest idea was to fully resolve hashes, add missing
>> parameters if possible (like 'h', 'hp', 'f') and convert hashes to
>> sha-1. One of intended uses was (weak) ETag for simple HTTP caching.
>
> Interesting. Something to keep in mind is that using name-rev still can
> wreck with this since it has the unique property of taking hashes but
> still depending on the current refs. Gitweb isn't using name-rev a lot
> right now, but that might change of course (e.g. I think that it would
> be convenient to always display names along with any commit hashes).
Well, you will have two level cache: first from refnames[*1*] to hashes
(by the way I think you can treat tag names as valid indifinitely,
and use them in the place of full sha-1 hashes; thanks to heads<->tags
ambiguity gitweb now spells tags in full as refs/heads/<tag>), second
from "normalized" URLs to content, or rather from "normalized" URL
derivative to data used to generate content.
So I think you can use first-level cache to calculate equivalent
of 'name-rev', or keep cached 'name-rev' there.
Currently gitweb uses name-rev only in (I think) rarely used 'raw'
(text/plain) version of 'commit' and 'commitdiff' views... and I think
it is here to stay thanks to gitk like displaying refs marks in
log-like views (I think result of git_get_references() should also
be cached...).
[*1*] Well, project name (repository path) + refname.
We will see if this two-tier cache solution is good idea...
>> All the time I think that caching _everything_ is a bad solution.
>
> So? We can easily add an option to the cache; e.g. no_cache =>
> ['get_blob', 'ls_tree']. I doubt that it will be needed, but if it
> does, it's easy to add it. Don't worry about it, really.
Well, on the other hand from what I understand kernel.org gitweb caches
unconditionally (but adaptively) every page, same with CGit (used for
example on freedesktop.org).
It would be nice to have some stats of gitweb access (e.g. from
repo.or.cz and kernel.org).
>> CHI (or other in recommended thread) for inobtrusive data caching
>
> Thanks for the pointer! On the one hand CHI is very recent and not even
> in Debian, on the other hand it provides things like busy_lock on top of
> Memcached (AFAICS), at fairly little cost. I'll look into it.
I was not thinking about using CHI, or any Perl module metioned in
"[RFD] Gitweb caching, part 3: examining Perl modules for caching (long)"
http://thread.gmane.org/gmane.comp.version-control.git/77529/focus=77527
(perhaps with exception of Cache::Cache and its submodules/engines), but
rather about emulating best of its interfaces (well, perhaps also
"borrowing" some of its code, if license permits).
--
Jakub Narebski
Poland
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2] gitweb: standarize HTTP status codes
2008-06-18 0:15 ` [PATCH] gitweb: standarize " Lea Wiemann
@ 2008-06-19 0:51 ` Jakub Narebski
2008-06-19 19:08 ` Lea Wiemann
0 siblings, 1 reply; 25+ messages in thread
From: Jakub Narebski @ 2008-06-19 0:51 UTC (permalink / raw)
To: Lea Wiemann; +Cc: git
Lea Wiemann <lewiemann@gmail.com> writes:
> Many error status codes simply default to 403 Forbidden, which is not
> correct in most cases. This patch makes gitweb return semantically
> correct status codes.
O.K.
> For convenience the die_error function now only takes the status code
> without reason as first parameter (e.g. 404 instead of "404 Not
> Found"), and it now defaults to 500 (Internal Server Error), even
> though the default is not used anywhere.
_Whose_ convenience?
IMHO using "bare" status code is bad idea for the following reasons:
* I don't think that RFC 2616 allows blanket replacing reason phrase
by generic "Error", as I don't think that for example "Error" is
'local equivalent' of "Not Found" for 404 client error. And even
if it would allow it, I don't think it is best / recommended
practice.
If the fragment cited below isn't what you mean by "RFC 2616 allows
this" please point me out to the fragment that states this.
> + # Use a generic "Error" reason, e.g. "404 Error" instead of
> + # "404 Not Found". This is permissible by RFC 2616.
> + git_header_html("$status Error");
* Test::WWW::Mechanize displays both HTTP error status code and
reason phrase when get_ok(...) fails:
not ok 18 - GET http://localhost/?p=non-existent.git
# Failed test 'GET http://localhost/?p=non-existent.git
# at ../t9503/test.pl line 105.
# 403
# Forbidden
It does not display error _page_, so it helps if reason phrase
explains the error.
* From the point of view of someone examinimg gitweb.perl code, 400,
403, 404, 500 are _magic numbers_; one would have to take a look at
RFC 2616 (or Wikipedia) to check what given number means. It is
much more readable to have e.g. '404 Not Found' there.
Posible solution would be to use constants similar to the ones in
HTTP::Status (but best without requiring this module, as it might be
not present), for example HTTP_OK, HTTP_INVALID, HTTP_FORBIDDEN,
HTTP_NOT_FOUND, HTTP_SERVER_ERROR or similar (HTTP::Status uses
RC_OK (200), RC_BAD_REQUEST (400), RC_FORBIDDEN (403),
RC_NOT_FOUND (404), RC_INTERNAL_SERVER_ERROR (500)).
..................................................................
Relevant fragment of RFC 2616 (http://tools.ietf.org/html/rfc2616)
6.1.1 Status Code and Reason Phrase
The Status-Code element is a 3-digit integer result code of the
attempt to understand and satisfy the request. These codes are
fully defined in section 10. The Reason-Phrase is intended to
give a short textual description of the Status-Code. The
Status-Code is intended for use by automata and the Reason-Phrase
is intended for the human user. The client is not required to
examine or display the Reason-Phrase.
[...]
(...) The reason phrases listed here are only recommendations --
they MAY be replaced by local equivalents without affecting the
protocol.
..................................................................
> Also documented status code conventions in die_error.
Putting copy of those status code conventions in the commit message,
and not only in the comment section, would be also good idea.
> Signed-off-by: Lea Wiemann <LeWiemann@gmail.com>
> ---
> Changes since v1:
Would be better to change subject to contain "[PATCH v2]", too?
> - Changed subject per Jakub's suggestion.
It was just suggestion. IMVHO current version reads better, but again
I am not a native English speaker.
> - Resolved merge conflict caused by "[PATCH v2] gitweb: quote commands
> properly when calling the shell".
> - Added the following documentation to die_error (I know Jakub might
> disagree with me on the last line ;-)):
No, I won't disagree. "403 Forbidden" is bad catch-all, and bad
default. "404 Not Found" or "500 Internal Server Error" are much
better default values.
> # die_error(<http_status_code>, <error_message>)
> # Example: die_error(404, 'Hash not found')
As I mentioned above, I'd rather have
# Example: die_error(HTTP_NOT_FOUND, 'Hash not found')
here, where HTTP_NOT_FOUND is expanded to '404 Not Found'.
> # By convention, use the following status codes (as defined in RFC 2616):
> # 400: Invalid or missing CGI parameters, or
> # requested object exists but has wrong type.
> # 403: Requested feature (like "pickaxe" or "snapshot") not enabled on
> # this server or project.
> # 404: Requested object/revision/project doesn't exist.
Note that all above 4xx client errors make one look at URL to check if
it is correct in the case of errors.
> # 500: The server isn't configured properly, or
> # an internal error occurred (e.g. failed assertions caused by bugs), or
> # an unknown error occurred (e.g. the git binary died unexpectedly).
Note that all 5xx server errors make one search for webmaster /
network administrator email (or other contact info, like IM or contact
form) to notify about error.
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 3a7adae..3ca45fd 100755
[...]
> @@ -399,21 +399,21 @@ if (defined $project) {
> ($export_ok && !(-e "$projectroot/$project/$export_ok")) ||
> ($strict_export && !project_in_list($project))) {
> undef $project;
> - die_error(undef, "No such project");
> + die_error(404, "No such project");
> }
> }
Here is one thing worth considering: if project exists, but is
excluded due to either $export_ok or $strict_export being set,
should we use '404 Not Found' or '403 Forbidden'?
It is easier to use catch-all '404 Not Found', and I think this is
better for security reason, but I guess that is worth thinking about a
bit.
> @@ -483,7 +483,7 @@ our $searchtext = $cgi->param('s');
> our $search_regexp;
> if (defined $searchtext) {
> if (length($searchtext) < 2) {
> - die_error(undef, "At least two characters are required for search parameter");
> + die_error(403, "At least two characters are required for search parameter");
> }
> $search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;
> }
Should gitweb use there '403 Forbidden', or '400 Bad Request'?
This is failing static validation of CGI parameters, not a matter of
some permissions...
> @@ -1665,7 +1665,7 @@ sub git_get_hash_by_path {
> $path =~ s,/+$,,;
>
> open my $fd, "-|", git_cmd(), "ls-tree", $base, "--", $path
> - or die_error(undef, "Open git-ls-tree failed");
> + or die_error(500, "Open git-ls-tree failed");
> my $line = <$fd>;
> close $fd or return undef;
>
I'm sorry for my mistake in earlier email.
*All* "open ... or die_error" constructs should use '500 Internal
Server Error', as the only errors that can be detected on open are
very serious, server related ones: cannot find git binaries, git
binaries doesn't have executable permissions for web server, number of
available filehandles exceeded, cannot fork (because of limits), etc.
All of those are result of error on the server side, not the client
mangling URL or something like that.
> + # Use a generic "Error" reason, e.g. "404 Error" instead of
> + # "404 Not Found". This is permissible by RFC 2616.
> + git_header_html("$status Error");
See comments above. In short: I don't think it is permissible, if it
is permissible I don't think it is recommended practice, if it is
common practice I don't think this is a good idea for gitweb.
> - my ($have_blame) = gitweb_check_feature('blame');
> - if (!$have_blame) {
> - die_error('403 Permission denied', "Permission denied");
> - }
> - die_error('404 Not Found', "File name not defined") if (!$file_name);
> + gitweb_check_feature('blame')
> + or die_error(403, "Blame not allowed");
That is a bit separate change, i.e. better explanation of an error
(although I'd say rather "Blame view not allowed").
But what about security reasons?
> - die_error(undef, "Couldn't find base commit") unless ($hash_base);
> + die_error(400, "Couldn't find base commit") unless ($hash_base);
Wound't it be '404 Not Found', as the explanation suggests?
> my %co = parse_commit($hash_base)
> - or die_error(undef, "Reading commit failed");
> + or die_error(404, "Commit not found");
Good!
> $/ = "\0";
> open my $fd, "-|", git_cmd(), "ls-tree", '-z', $hash
> - or die_error(undef, "Open git-ls-tree failed");
> + or die_error(500, "Open git-ls-tree failed");
O.K.
> my @entries = map { chomp; $_ } <$fd>;
> - close $fd or die_error(undef, "Reading tree failed");
> + close $fd or die_error(500, "Reading tree failed");
> $/ = "\n";
Not O.K. Barring errors in gitweb code this might happen when object
given by CGI parameters does not exist ('fatal: Not a valid object
name foo'), project does not exist ('fatal: Not a git repository:
project'), or the object is not a tree-ish ('fatal: not a tree
object'). All those are clearly 4xx _client_ errors, and perhaps with
the exception of last one which could be '400 Bad Request', all are of
the '404 Nof Found'; but '404 Not FOund' is good also for the last
reason, so for simplicity catch-all '404 Not Found' would be best.
One should examine URL for mistakes, not contact server admin here...
> sub git_commit {
> $hash ||= $hash_base || "HEAD";
> - my %co = parse_commit($hash);
> - if (!%co) {
> - die_error(undef, "Unknown commit object");
> - }
> + my %co = parse_commit($hash)
> + or die_error(404, "Unknown commit object");
Good, although if it makes code more readable or not is a matter of
taste.
> - close $fd or die_error(undef, "Reading git-diff-tree failed");
> + close $fd or die_error(500, "Reading git-diff-tree failed");
Again, this is ussually 4xx client error, and one should examine URL
for mistakes, not contact server admin.
> } else {
> - die_error('404 Not Found', "Not enough information to find object");
> + die_error(400, "Not enough information to find object");
> }
I'm not sure if it should be '400 Bad Request' or '404 Not Found'
here. RFC 2616 states:
10.4.5 404 Not Found
(...) This status code is commonly used when the server does not
wish to reveal exactly why the request has been refused, or when
no other response is applicable.
so I guess '404' _might_ be better here. But this is IMVHO a matter
of taste here, a bit.
> close $fd
> - or die_error(undef, "Reading git-diff-tree failed");
> + or die_error(500, "Reading git-diff-tree failed");
Again, I think it is usually 4xx client error, i.e. '404 Nof Found'
and not '500 Internal Sever Error' should be used here.
> @difftree
> - or die_error('404 Not Found', "Blob diff not found");
> + or die_error(404, "Blob diff not found");
This, if I am not mistaken, can happen only if path limiters doesn'
catch anything.
> close $fd
> - or die_error(undef, "Reading git-diff-tree failed");
> + or die_error(500, "Reading git-diff-tree failed");
And again...
> } else {
> - die_error('404 Not Found', "Missing one of the blob diff parameters");
> + die_error(400, "Missing one of the blob diff parameters");
> }
O.K. here.
> if (@difftree > 1) {
> - die_error('404 Not Found', "Ambiguous blob diff specification");
> + die_error(400, "Ambiguous blob diff specification");
> }
Or perhaps '404 Not Dound' here?
> } else {
> - die_error('404 Not Found', "Missing one of the blob diff parameters")
> + die_error(400, "Missing one of the blob diff parameters")
> unless %diffinfo;
> }
The "unless %diffinfo" makes it look more like '404 Not Found'...
> if (!defined $ftype) {
> - die_error(undef, "Unknown type of object");
> + die_error(500, "Unknown type of object");
> }
Errr... shouldn't be '400 Bad Request' here, per convention?
> - my ($have_pickaxe) = gitweb_check_feature('pickaxe');
> - if (!$have_pickaxe) {
> - die_error('403 Permission denied', "Permission denied");
> - }
> + gitweb_check_feature('pickaxe')
> + or die_error(403, "Pickaxe is disabled");
"Pickaxe" is git jargon... (just mentioning it).
> @@ -5484,7 +5481,7 @@ sub git_feed {
> # Atom: http://www.atomenabled.org/developers/syndication/
> # RSS: http://www.notestips.com/80256B3A007F2692/1/NAMO5P9UPQ
> if ($format ne 'rss' && $format ne 'atom') {
> - die_error(undef, "Unknown web feed format");
> + die_error(400, "Unknown web feed format");
> }
That is a good example of unambiguously '400 Bad Request'.
--
Jakub Narebski
Poland
ShadeHawk on #git
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2] gitweb: standarize HTTP status codes
2008-06-19 0:51 ` [PATCH v2] " Jakub Narebski
@ 2008-06-19 19:08 ` Lea Wiemann
2008-06-19 20:03 ` [PATCH v3] " Lea Wiemann
2008-06-19 22:22 ` [PATCH v2] " Jakub Narebski
0 siblings, 2 replies; 25+ messages in thread
From: Lea Wiemann @ 2008-06-19 19:08 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
Jakub Narebski wrote:
> Lea Wiemann <lewiemann@gmail.com> writes:
>> For convenience the die_error function now only takes the status code
>> without reason as first parameter (e.g. 404 instead of "404 Not Found")
>
> _Whose_ convenience?
The developer's convenience of course. It's plain redundant.
> * I don't think that RFC 2616 allows blanket replacing reason phrase
> by generic "Error",
> * Test::WWW::Mechanize displays both HTTP error status code and
> reason phrase when get_ok(...) fails:
> * From the point of view of someone examinimg gitweb.perl code, 400,
> 403, 404, 500 are _magic numbers_;
I think we're really arguing about the color of the bikeshed here. IMO
we're not stretching RFC 2616 too much by putting "Error" there (since
reason codes don't matter on a technical level), and the status codes
make enough sense to me (and I'm not even a web developer) that I'm not
concerned about readability.
I don't think your constants a la HTTP_INVALID are a good idea (I
remember the status codes in a year, but maybe not the constants);
die_error could figure out the right reason code using a hash. Either
way will be fine with me though.
Look, I really don't care about this and I think it's plain irrelevant
-- do you mind fixing the patch with whatever solution you prefer and
resend it? I'd rather spend my time on getting caching implemented, but
if you insist on me changing this myself, please let me know.
>> Also documented status code conventions in die_error.
>
> Putting copy of those status code conventions in the commit message,
> and not only in the comment section, would be also good idea.
*shrugs* I'd rather not duplicate, and it's in the code anyway.
>> - die_error(undef, "No such project");
>> + die_error(404, "No such project");
>
> Here is one thing worth considering: if project exists, but is
> excluded due to either $export_ok or $strict_export being set,
> should we use '404 Not Found' or '403 Forbidden'?
No, because (a) we'd have add code to check this (so it should be in a
separate patch), and (b) I don't think we even want to return 403, since
(as you say) it would reveal the existence of a project on the server.
Project names can contain sensitive/confidential information though.
>> - die_error(undef, "At least two characters are required for search parameter");
>> + die_error(403, "At least two characters are required for search parameter");
>
> Should gitweb use there '403 Forbidden', or '400 Bad Request'?
> This is failing static validation of CGI parameters, not a matter of
> some permissions...
I used 403 in the sense of "sorry, we don't have shorter search strings
activated for performance reasons". The '2' could even become
configurable. 400 is fine too, though, I don't care.
> *All* "open ... or die_error" constructs should use '500 Internal
> Server Error', as the only errors that can be detected on open are
> very serious, server related ones:
Right, thanks for pointing this out.
>> - my ($have_blame) = gitweb_check_feature('blame');
>> - if (!$have_blame) {
>> - die_error('403 Permission denied', "Permission denied");
>> - }
>> + gitweb_check_feature('blame')
>> + or die_error(403, "Blame not allowed");
>
> That is a bit separate change, i.e. better explanation of an error
> (although I'd say rather "Blame view not allowed").
>
> But what about security reasons?
Separate change: I don't think this has to be a separate patch. ;-)
"Blame view not allowed": Fine with me.
Security concerns: I don't see any, and anyway you can probably infer
from getting 403 on a=blame that it's disabled.
>> - die_error(undef, "Couldn't find base commit") unless ($hash_base);
>> + die_error(400, "Couldn't find base commit") unless ($hash_base);
>
> Wound't it be '404 Not Found', as the explanation suggests?
Yup, right.
>> - close $fd or die_error(undef, "Reading tree failed");
>> + close $fd or die_error(500, "Reading tree failed");
>
> Not O.K. Barring errors in gitweb code this might happen when
> [X Y Z]. All those are clearly 4xx _client_ errors,
I haven't verified that, so until we have better error handling I prefer
500, but I really won't bother objecting to 404. (Same goes for all
other occurrences of 500 you quoted, which I've snipped.) FWIW I'm
assuming that once gitweb uses the new API, that error handling code
will go away anyway.
> One should examine URL for mistakes, not contact server admin here...
I don't think that'll be a practical concern. ;-)
>> - die_error('404 Not Found', "Not enough information to find object");
>> + die_error(400, "Not enough information to find object");
>
> I'm not sure if it should be '400 Bad Request' or '404 Not Found' here.
It's about missing CGI parameters, so 400 should be fine.
>> @difftree
>> - or die_error('404 Not Found', "Blob diff not found");
>> + or die_error(404, "Blob diff not found");
>
> This, if I am not mistaken, can happen only if path limiters doesn'
> catch anything.
Your sentence doesn't quite parse -- why is 404 wrong?
>> - die_error('404 Not Found', "Ambiguous blob diff specification");
>> + die_error(400, "Ambiguous blob diff specification");
>
> Or perhaps '404 Not Dound' here?
Either way is fine.
>> - die_error('404 Not Found', "Missing one of the blob diff parameters")
>> + die_error(400, "Missing one of the blob diff parameters")
>> unless %diffinfo;
>
> The "unless %diffinfo" makes it look more like '404 Not Found'...
Looking at the preceding code (the if statement) I believe diffinfo
being false doesn't indicate 404 but actually a missing CGI parameter
(as the error message states).
>> if (!defined $ftype) {
>> - die_error(undef, "Unknown type of object");
>> + die_error(500, "Unknown type of object");
>
> Errr... shouldn't be '400 Bad Request' here, per convention?
Nope, we didn't get *anything* back, so something weird happened. 500.
-- Lea
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v3] gitweb: standarize HTTP status codes
2008-06-19 19:08 ` Lea Wiemann
@ 2008-06-19 20:03 ` Lea Wiemann
2008-06-19 20:25 ` Lea Wiemann
2008-06-20 0:48 ` Junio C Hamano
2008-06-19 22:22 ` [PATCH v2] " Jakub Narebski
1 sibling, 2 replies; 25+ messages in thread
From: Lea Wiemann @ 2008-06-19 20:03 UTC (permalink / raw)
To: git; +Cc: Lea Wiemann
Many error status codes simply default to 403 Forbidden, which is not
correct in most cases. This patch makes gitweb return semantically
correct status codes.
For convenience the die_error function now only takes the status code
without reason as first parameter (e.g. 404 instead of "404 Not
Found"), and it now defaults to 500 (Internal Server Error), even
though the default is not used anywhere.
Also documented status code conventions in die_error.
Signed-off-by: Lea Wiemann <LeWiemann@gmail.com>
---
Changes since v2: die_error now adds the reason strings defined by RFC
2616 to the HTTP status code; incorporated Jakub's other suggestions.
Diff to v2 follows.
I didn't use the HTTP_NOT_FOUND etc. suggestion because I found it too
verbose and obtrusive.
Just a friendly reminder, please remember that discussing fairly
trivial changes in-depth might be not a good use of all participants'
time; IOW, sending 340 lines of email about HTTP status codes that
don't *really* matter is probably overkill. (In case you're
interested, the reason why I wrote this patch in the first place is in
order to be able to tell unforeseen errors from expected errors in the
test suite.) I recommend you read http://bikeshed.com/ if you haven't
done so.
Also, increasing the time between patch submission and acceptance
causes added overhead (effort) by itself (i.e. *in addition* to the
time spend on discussing and resending patches); that's because you
cannot base anything on the code without risking merge conflicts.
Please think of that when replying to patches. ;-)
By the way, when you want to make trivial changes, it may be easier
for everyone if you simply implement them and send a follow-up patch
(and it might actually be just as quick as suggesting the changes!).
In your follow-up patch you could provide a diff to the previous
version, and comment on the changes you made (or send it without
comments, even -- oftentimes they're obvious).
Anyways, I hope everyone is happy with this version of the patch.
Diff to v2:
index 3ca45fd..3bc8f69 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2687,9 +2687,11 @@ sub die_error {
my $status = shift || 500;
my $error = shift || "Internal server error";
- # Use a generic "Error" reason, e.g. "404 Error" instead of
- # "404 Not Found". This is permissible by RFC 2616.
- git_header_html("$status Error");
+ my %http_responses = (400 => '400 Bad Request',
+ 403 => '403 Forbidden',
+ 404 => '404 Not Found',
+ 500 => '500 Internal Server Error');
+ git_header_html($http_responses{$status});
print <<EOF;
<div class="page_body">
<br /><br />
@@ -4131,11 +4133,11 @@ sub git_blame {
my $ftype;
gitweb_check_feature('blame')
- or die_error(403, "Blame not allowed");
+ or die_error(403, "Blame view not allowed");
die_error(400, "No file name given") unless $file_name;
$hash_base ||= git_get_head_hash($project);
- die_error(400, "Couldn't find base commit") unless ($hash_base);
+ die_error(404, "Couldn't find base commit") unless ($hash_base);
my %co = parse_commit($hash_base)
or die_error(404, "Commit not found");
if (!defined $hash) {
@@ -4403,7 +4405,7 @@ sub git_tree {
open my $fd, "-|", git_cmd(), "ls-tree", '-z', $hash
or die_error(500, "Open git-ls-tree failed");
my @entries = map { chomp; $_ } <$fd>;
- close $fd or die_error(500, "Reading tree failed");
+ close $fd or die_error(404, "Reading tree failed");
$/ = "\n";
my $refs = git_get_references();
@@ -4641,7 +4643,7 @@ sub git_commit {
$hash, "--"
or die_error(500, "Open git-diff-tree failed");
@difftree = map { chomp; $_ } <$fd>;
- close $fd or die_error(500, "Reading git-diff-tree failed");
+ close $fd or die_error(404, "Reading git-diff-tree failed");
# non-textual hash id's can be cached
my $expires;
@@ -4788,7 +4790,7 @@ sub git_blobdiff {
or die_error(500, "Open git-diff-tree failed");
@difftree = map { chomp; $_ } <$fd>;
close $fd
- or die_error(500, "Reading git-diff-tree failed");
+ or die_error(404, "Reading git-diff-tree failed");
@difftree
or die_error(404, "Blob diff not found");
@@ -4806,7 +4808,7 @@ sub git_blobdiff {
grep { /^:[0-7]{6} [0-7]{6} [0-9a-fA-F]{40} $hash/ }
map { chomp; $_ } <$fd>;
close $fd
- or die_error(500, "Reading git-diff-tree failed");
+ or die_error(404, "Reading git-diff-tree failed");
@difftree
or die_error(404, "Blob diff not found");
gitweb/gitweb.perl | 211 ++++++++++++++++++++++++++--------------------------
1 files changed, 105 insertions(+), 106 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 3a7adae..3bc8f69 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -386,7 +386,7 @@ $projects_list ||= $projectroot;
our $action = $cgi->param('a');
if (defined $action) {
if ($action =~ m/[^0-9a-zA-Z\.\-_]/) {
- die_error(undef, "Invalid action parameter");
+ die_error(400, "Invalid action parameter");
}
}
@@ -399,21 +399,21 @@ if (defined $project) {
($export_ok && !(-e "$projectroot/$project/$export_ok")) ||
($strict_export && !project_in_list($project))) {
undef $project;
- die_error(undef, "No such project");
+ die_error(404, "No such project");
}
}
our $file_name = $cgi->param('f');
if (defined $file_name) {
if (!validate_pathname($file_name)) {
- die_error(undef, "Invalid file parameter");
+ die_error(400, "Invalid file parameter");
}
}
our $file_parent = $cgi->param('fp');
if (defined $file_parent) {
if (!validate_pathname($file_parent)) {
- die_error(undef, "Invalid file parent parameter");
+ die_error(400, "Invalid file parent parameter");
}
}
@@ -421,21 +421,21 @@ if (defined $file_parent) {
our $hash = $cgi->param('h');
if (defined $hash) {
if (!validate_refname($hash)) {
- die_error(undef, "Invalid hash parameter");
+ die_error(400, "Invalid hash parameter");
}
}
our $hash_parent = $cgi->param('hp');
if (defined $hash_parent) {
if (!validate_refname($hash_parent)) {
- die_error(undef, "Invalid hash parent parameter");
+ die_error(400, "Invalid hash parent parameter");
}
}
our $hash_base = $cgi->param('hb');
if (defined $hash_base) {
if (!validate_refname($hash_base)) {
- die_error(undef, "Invalid hash base parameter");
+ die_error(400, "Invalid hash base parameter");
}
}
@@ -447,10 +447,10 @@ our @extra_options = $cgi->param('opt');
if (defined @extra_options) {
foreach my $opt (@extra_options) {
if (not exists $allowed_options{$opt}) {
- die_error(undef, "Invalid option parameter");
+ die_error(400, "Invalid option parameter");
}
if (not grep(/^$action$/, @{$allowed_options{$opt}})) {
- die_error(undef, "Invalid option parameter for this action");
+ die_error(400, "Invalid option parameter for this action");
}
}
}
@@ -458,7 +458,7 @@ if (defined @extra_options) {
our $hash_parent_base = $cgi->param('hpb');
if (defined $hash_parent_base) {
if (!validate_refname($hash_parent_base)) {
- die_error(undef, "Invalid hash parent base parameter");
+ die_error(400, "Invalid hash parent base parameter");
}
}
@@ -466,14 +466,14 @@ if (defined $hash_parent_base) {
our $page = $cgi->param('pg');
if (defined $page) {
if ($page =~ m/[^0-9]/) {
- die_error(undef, "Invalid page parameter");
+ die_error(400, "Invalid page parameter");
}
}
our $searchtype = $cgi->param('st');
if (defined $searchtype) {
if ($searchtype =~ m/[^a-z]/) {
- die_error(undef, "Invalid searchtype parameter");
+ die_error(400, "Invalid searchtype parameter");
}
}
@@ -483,7 +483,7 @@ our $searchtext = $cgi->param('s');
our $search_regexp;
if (defined $searchtext) {
if (length($searchtext) < 2) {
- die_error(undef, "At least two characters are required for search parameter");
+ die_error(403, "At least two characters are required for search parameter");
}
$search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;
}
@@ -580,11 +580,11 @@ if (!defined $action) {
}
}
if (!defined($actions{$action})) {
- die_error(undef, "Unknown action");
+ die_error(400, "Unknown action");
}
if ($action !~ m/^(opml|project_list|project_index)$/ &&
!$project) {
- die_error(undef, "Project needed");
+ die_error(400, "Project needed");
}
$actions{$action}->();
exit;
@@ -1665,7 +1665,7 @@ sub git_get_hash_by_path {
$path =~ s,/+$,,;
open my $fd, "-|", git_cmd(), "ls-tree", $base, "--", $path
- or die_error(undef, "Open git-ls-tree failed");
+ or die_error(500, "Open git-ls-tree failed");
my $line = <$fd>;
close $fd or return undef;
@@ -2127,7 +2127,7 @@ sub parse_commit {
"--max-count=1",
$commit_id,
"--",
- or die_error(undef, "Open git-rev-list failed");
+ or die_error(500, "Open git-rev-list failed");
%co = parse_commit_text(<$fd>, 1);
close $fd;
@@ -2152,7 +2152,7 @@ sub parse_commits {
$commit_id,
"--",
($filename ? ($filename) : ())
- or die_error(undef, "Open git-rev-list failed");
+ or die_error(500, "Open git-rev-list failed");
while (my $line = <$fd>) {
my %co = parse_commit_text($line);
push @cos, \%co;
@@ -2672,11 +2672,26 @@ sub git_footer_html {
"</html>";
}
+# die_error(<http_status_code>, <error_message>)
+# Example: die_error(404, 'Hash not found')
+# By convention, use the following status codes (as defined in RFC 2616):
+# 400: Invalid or missing CGI parameters, or
+# requested object exists but has wrong type.
+# 403: Requested feature (like "pickaxe" or "snapshot") not enabled on
+# this server or project.
+# 404: Requested object/revision/project doesn't exist.
+# 500: The server isn't configured properly, or
+# an internal error occurred (e.g. failed assertions caused by bugs), or
+# an unknown error occurred (e.g. the git binary died unexpectedly).
sub die_error {
- my $status = shift || "403 Forbidden";
- my $error = shift || "Malformed query, file missing or permission denied";
-
- git_header_html($status);
+ my $status = shift || 500;
+ my $error = shift || "Internal server error";
+
+ my %http_responses = (400 => '400 Bad Request',
+ 403 => '403 Forbidden',
+ 404 => '404 Not Found',
+ 500 => '500 Internal Server Error');
+ git_header_html($http_responses{$status});
print <<EOF;
<div class="page_body">
<br /><br />
@@ -3924,12 +3939,12 @@ sub git_search_grep_body {
sub git_project_list {
my $order = $cgi->param('o');
if (defined $order && $order !~ m/none|project|descr|owner|age/) {
- die_error(undef, "Unknown order parameter");
+ die_error(400, "Unknown order parameter");
}
my @list = git_get_projects_list();
if (!@list) {
- die_error(undef, "No projects found");
+ die_error(404, "No projects found");
}
git_header_html();
@@ -3947,12 +3962,12 @@ sub git_project_list {
sub git_forks {
my $order = $cgi->param('o');
if (defined $order && $order !~ m/none|project|descr|owner|age/) {
- die_error(undef, "Unknown order parameter");
+ die_error(400, "Unknown order parameter");
}
my @list = git_get_projects_list($project);
if (!@list) {
- die_error(undef, "No forks found");
+ die_error(404, "No forks found");
}
git_header_html();
@@ -4081,7 +4096,7 @@ sub git_tag {
my %tag = parse_tag($hash);
if (! %tag) {
- die_error(undef, "Unknown tag object");
+ die_error(404, "Unknown tag object");
}
git_print_header_div('commit', esc_html($tag{'name'}), $hash);
@@ -4117,26 +4132,25 @@ sub git_blame {
my $fd;
my $ftype;
- my ($have_blame) = gitweb_check_feature('blame');
- if (!$have_blame) {
- die_error('403 Permission denied', "Permission denied");
- }
- die_error('404 Not Found', "File name not defined") if (!$file_name);
+ gitweb_check_feature('blame')
+ or die_error(403, "Blame view not allowed");
+
+ die_error(400, "No file name given") unless $file_name;
$hash_base ||= git_get_head_hash($project);
- die_error(undef, "Couldn't find base commit") unless ($hash_base);
+ die_error(404, "Couldn't find base commit") unless ($hash_base);
my %co = parse_commit($hash_base)
- or die_error(undef, "Reading commit failed");
+ or die_error(404, "Commit not found");
if (!defined $hash) {
$hash = git_get_hash_by_path($hash_base, $file_name, "blob")
- or die_error(undef, "Error looking up file");
+ or die_error(404, "Error looking up file");
}
$ftype = git_get_type($hash);
if ($ftype !~ "blob") {
- die_error('400 Bad Request', "Object is not a blob");
+ die_error(400, "Object is not a blob");
}
open ($fd, "-|", git_cmd(), "blame", '-p', '--',
$file_name, $hash_base)
- or die_error(undef, "Open git-blame failed");
+ or die_error(500, "Open git-blame failed");
git_header_html();
my $formats_nav =
$cgi->a({-href => href(action=>"blob", -replay=>1)},
@@ -4198,7 +4212,7 @@ HTML
print "</td>\n";
}
open (my $dd, "-|", git_cmd(), "rev-parse", "$full_rev^")
- or die_error(undef, "Open git-rev-parse failed");
+ or die_error(500, "Open git-rev-parse failed");
my $parent_commit = <$dd>;
close $dd;
chomp($parent_commit);
@@ -4255,9 +4269,9 @@ sub git_blob_plain {
if (defined $file_name) {
my $base = $hash_base || git_get_head_hash($project);
$hash = git_get_hash_by_path($base, $file_name, "blob")
- or die_error(undef, "Error lookup file");
+ or die_error(404, "Cannot find file");
} else {
- die_error(undef, "No file name defined");
+ die_error(400, "No file name defined");
}
} elsif ($hash =~ m/^[0-9a-fA-F]{40}$/) {
# blobs defined by non-textual hash id's can be cached
@@ -4265,7 +4279,7 @@ sub git_blob_plain {
}
open my $fd, "-|", git_cmd(), "cat-file", "blob", $hash
- or die_error(undef, "Open git-cat-file blob '$hash' failed");
+ or die_error(500, "Open git-cat-file blob '$hash' failed");
# content-type (can include charset)
$type = blob_contenttype($fd, $file_name, $type);
@@ -4297,9 +4311,9 @@ sub git_blob {
if (defined $file_name) {
my $base = $hash_base || git_get_head_hash($project);
$hash = git_get_hash_by_path($base, $file_name, "blob")
- or die_error(undef, "Error lookup file");
+ or die_error(404, "Cannot find file");
} else {
- die_error(undef, "No file name defined");
+ die_error(400, "No file name defined");
}
} elsif ($hash =~ m/^[0-9a-fA-F]{40}$/) {
# blobs defined by non-textual hash id's can be cached
@@ -4308,7 +4322,7 @@ sub git_blob {
my ($have_blame) = gitweb_check_feature('blame');
open my $fd, "-|", git_cmd(), "cat-file", "blob", $hash
- or die_error(undef, "Couldn't cat $file_name, $hash");
+ or die_error(500, "Couldn't cat $file_name, $hash");
my $mimetype = blob_mimetype($fd, $file_name);
if ($mimetype !~ m!^(?:text/|image/(?:gif|png|jpeg)$)! && -B $fd) {
close $fd;
@@ -4389,9 +4403,9 @@ sub git_tree {
}
$/ = "\0";
open my $fd, "-|", git_cmd(), "ls-tree", '-z', $hash
- or die_error(undef, "Open git-ls-tree failed");
+ or die_error(500, "Open git-ls-tree failed");
my @entries = map { chomp; $_ } <$fd>;
- close $fd or die_error(undef, "Reading tree failed");
+ close $fd or die_error(404, "Reading tree failed");
$/ = "\n";
my $refs = git_get_references();
@@ -4481,16 +4495,16 @@ sub git_snapshot {
my $format = $cgi->param('sf');
if (!@supported_fmts) {
- die_error('403 Permission denied', "Permission denied");
+ die_error(403, "Snapshots not allowed");
}
# default to first supported snapshot format
$format ||= $supported_fmts[0];
if ($format !~ m/^[a-z0-9]+$/) {
- die_error(undef, "Invalid snapshot format parameter");
+ die_error(400, "Invalid snapshot format parameter");
} elsif (!exists($known_snapshot_formats{$format})) {
- die_error(undef, "Unknown snapshot format");
+ die_error(400, "Unknown snapshot format");
} elsif (!grep($_ eq $format, @supported_fmts)) {
- die_error(undef, "Unsupported snapshot format");
+ die_error(403, "Unsupported snapshot format");
}
if (!defined $hash) {
@@ -4518,7 +4532,7 @@ sub git_snapshot {
-status => '200 OK');
open my $fd, "-|", $cmd
- or die_error(undef, "Execute git-archive failed");
+ or die_error(500, "Execute git-archive failed");
binmode STDOUT, ':raw';
print <$fd>;
binmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi
@@ -4586,10 +4600,8 @@ sub git_log {
sub git_commit {
$hash ||= $hash_base || "HEAD";
- my %co = parse_commit($hash);
- if (!%co) {
- die_error(undef, "Unknown commit object");
- }
+ my %co = parse_commit($hash)
+ or die_error(404, "Unknown commit object");
my %ad = parse_date($co{'author_epoch'}, $co{'author_tz'});
my %cd = parse_date($co{'committer_epoch'}, $co{'committer_tz'});
@@ -4629,9 +4641,9 @@ sub git_commit {
@diff_opts,
(@$parents <= 1 ? $parent : '-c'),
$hash, "--"
- or die_error(undef, "Open git-diff-tree failed");
+ or die_error(500, "Open git-diff-tree failed");
@difftree = map { chomp; $_ } <$fd>;
- close $fd or die_error(undef, "Reading git-diff-tree failed");
+ close $fd or die_error(404, "Reading git-diff-tree failed");
# non-textual hash id's can be cached
my $expires;
@@ -4724,33 +4736,33 @@ sub git_object {
open my $fd, "-|", quote_command(
git_cmd(), 'cat-file', '-t', $object_id) . ' 2> /dev/null'
- or die_error('404 Not Found', "Object does not exist");
+ or die_error(404, "Object does not exist");
$type = <$fd>;
chomp $type;
close $fd
- or die_error('404 Not Found', "Object does not exist");
+ or die_error(404, "Object does not exist");
# - hash_base and file_name
} elsif ($hash_base && defined $file_name) {
$file_name =~ s,/+$,,;
system(git_cmd(), "cat-file", '-e', $hash_base) == 0
- or die_error('404 Not Found', "Base object does not exist");
+ or die_error(404, "Base object does not exist");
# here errors should not hapen
open my $fd, "-|", git_cmd(), "ls-tree", $hash_base, "--", $file_name
- or die_error(undef, "Open git-ls-tree failed");
+ or die_error(500, "Open git-ls-tree failed");
my $line = <$fd>;
close $fd;
#'100644 blob 0fa3f3a66fb6a137f6ec2c19351ed4d807070ffa panic.c'
unless ($line && $line =~ m/^([0-9]+) (.+) ([0-9a-fA-F]{40})\t/) {
- die_error('404 Not Found', "File or directory for given base does not exist");
+ die_error(404, "File or directory for given base does not exist");
}
$type = $2;
$hash = $3;
} else {
- die_error('404 Not Found', "Not enough information to find object");
+ die_error(400, "Not enough information to find object");
}
print $cgi->redirect(-uri => href(action=>$type, -full=>1,
@@ -4775,12 +4787,12 @@ sub git_blobdiff {
open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
$hash_parent_base, $hash_base,
"--", (defined $file_parent ? $file_parent : ()), $file_name
- or die_error(undef, "Open git-diff-tree failed");
+ or die_error(500, "Open git-diff-tree failed");
@difftree = map { chomp; $_ } <$fd>;
close $fd
- or die_error(undef, "Reading git-diff-tree failed");
+ or die_error(404, "Reading git-diff-tree failed");
@difftree
- or die_error('404 Not Found', "Blob diff not found");
+ or die_error(404, "Blob diff not found");
} elsif (defined $hash &&
$hash =~ /[0-9a-fA-F]{40}/) {
@@ -4789,23 +4801,23 @@ sub git_blobdiff {
# read filtered raw output
open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
$hash_parent_base, $hash_base, "--"
- or die_error(undef, "Open git-diff-tree failed");
+ or die_error(500, "Open git-diff-tree failed");
@difftree =
# ':100644 100644 03b21826... 3b93d5e7... M ls-files.c'
# $hash == to_id
grep { /^:[0-7]{6} [0-7]{6} [0-9a-fA-F]{40} $hash/ }
map { chomp; $_ } <$fd>;
close $fd
- or die_error(undef, "Reading git-diff-tree failed");
+ or die_error(404, "Reading git-diff-tree failed");
@difftree
- or die_error('404 Not Found', "Blob diff not found");
+ or die_error(404, "Blob diff not found");
} else {
- die_error('404 Not Found', "Missing one of the blob diff parameters");
+ die_error(400, "Missing one of the blob diff parameters");
}
if (@difftree > 1) {
- die_error('404 Not Found', "Ambiguous blob diff specification");
+ die_error(400, "Ambiguous blob diff specification");
}
%diffinfo = parse_difftree_raw_line($difftree[0]);
@@ -4826,7 +4838,7 @@ sub git_blobdiff {
'-p', ($format eq 'html' ? "--full-index" : ()),
$hash_parent_base, $hash_base,
"--", (defined $file_parent ? $file_parent : ()), $file_name
- or die_error(undef, "Open git-diff-tree failed");
+ or die_error(500, "Open git-diff-tree failed");
}
# old/legacy style URI
@@ -4862,9 +4874,9 @@ sub git_blobdiff {
open $fd, "-|", git_cmd(), "diff", @diff_opts,
'-p', ($format eq 'html' ? "--full-index" : ()),
$hash_parent, $hash, "--"
- or die_error(undef, "Open git-diff failed");
+ or die_error(500, "Open git-diff failed");
} else {
- die_error('404 Not Found', "Missing one of the blob diff parameters")
+ die_error(400, "Missing one of the blob diff parameters")
unless %diffinfo;
}
@@ -4897,7 +4909,7 @@ sub git_blobdiff {
print "X-Git-Url: " . $cgi->self_url() . "\n\n";
} else {
- die_error(undef, "Unknown blobdiff format");
+ die_error(400, "Unknown blobdiff format");
}
# patch
@@ -4932,10 +4944,8 @@ sub git_blobdiff_plain {
sub git_commitdiff {
my $format = shift || 'html';
$hash ||= $hash_base || "HEAD";
- my %co = parse_commit($hash);
- if (!%co) {
- die_error(undef, "Unknown commit object");
- }
+ my %co = parse_commit($hash)
+ or die_error(404, "Unknown commit object");
# choose format for commitdiff for merge
if (! defined $hash_parent && @{$co{'parents'}} > 1) {
@@ -5017,7 +5027,7 @@ sub git_commitdiff {
open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
"--no-commit-id", "--patch-with-raw", "--full-index",
$hash_parent_param, $hash, "--"
- or die_error(undef, "Open git-diff-tree failed");
+ or die_error(500, "Open git-diff-tree failed");
while (my $line = <$fd>) {
chomp $line;
@@ -5029,10 +5039,10 @@ sub git_commitdiff {
} elsif ($format eq 'plain') {
open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
'-p', $hash_parent_param, $hash, "--"
- or die_error(undef, "Open git-diff-tree failed");
+ or die_error(500, "Open git-diff-tree failed");
} else {
- die_error(undef, "Unknown commitdiff format");
+ die_error(400, "Unknown commitdiff format");
}
# non-textual hash id's can be cached
@@ -5115,19 +5125,15 @@ sub git_history {
$page = 0;
}
my $ftype;
- my %co = parse_commit($hash_base);
- if (!%co) {
- die_error(undef, "Unknown commit object");
- }
+ my %co = parse_commit($hash_base)
+ or die_error(404, "Unknown commit object");
my $refs = git_get_references();
my $limit = sprintf("--max-count=%i", (100 * ($page+1)));
my @commitlist = parse_commits($hash_base, 101, (100 * $page),
- $file_name, "--full-history");
- if (!@commitlist) {
- die_error('404 Not Found', "No such file or directory on given branch");
- }
+ $file_name, "--full-history")
+ or die_error(404, "No such file or directory on given branch");
if (!defined $hash && defined $file_name) {
# some commits could have deleted file in question,
@@ -5141,7 +5147,7 @@ sub git_history {
$ftype = git_get_type($hash);
}
if (!defined $ftype) {
- die_error(undef, "Unknown type of object");
+ die_error(500, "Unknown type of object");
}
my $paging_nav = '';
@@ -5179,19 +5185,16 @@ sub git_history {
}
sub git_search {
- my ($have_search) = gitweb_check_feature('search');
- if (!$have_search) {
- die_error('403 Permission denied', "Permission denied");
- }
+ gitweb_check_feature('search') or die_error(403, "Search is disabled");
if (!defined $searchtext) {
- die_error(undef, "Text field empty");
+ die_error(400, "Text field is empty");
}
if (!defined $hash) {
$hash = git_get_head_hash($project);
}
my %co = parse_commit($hash);
if (!%co) {
- die_error(undef, "Unknown commit object");
+ die_error(404, "Unknown commit object");
}
if (!defined $page) {
$page = 0;
@@ -5201,16 +5204,12 @@ sub git_search {
if ($searchtype eq 'pickaxe') {
# pickaxe may take all resources of your box and run for several minutes
# with every query - so decide by yourself how public you make this feature
- my ($have_pickaxe) = gitweb_check_feature('pickaxe');
- if (!$have_pickaxe) {
- die_error('403 Permission denied', "Permission denied");
- }
+ gitweb_check_feature('pickaxe')
+ or die_error(403, "Pickaxe is disabled");
}
if ($searchtype eq 'grep') {
- my ($have_grep) = gitweb_check_feature('grep');
- if (!$have_grep) {
- die_error('403 Permission denied', "Permission denied");
- }
+ gitweb_check_feature('grep')
+ or die_error(403, "Grep is disabled");
}
git_header_html();
@@ -5484,7 +5483,7 @@ sub git_feed {
# Atom: http://www.atomenabled.org/developers/syndication/
# RSS: http://www.notestips.com/80256B3A007F2692/1/NAMO5P9UPQ
if ($format ne 'rss' && $format ne 'atom') {
- die_error(undef, "Unknown web feed format");
+ die_error(400, "Unknown web feed format");
}
# log/feed of current (HEAD) branch, log of given branch, history of file/directory
--
1.5.6.149.g06c04.dirty
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v3] gitweb: standarize HTTP status codes
2008-06-19 20:03 ` [PATCH v3] " Lea Wiemann
@ 2008-06-19 20:25 ` Lea Wiemann
2008-06-19 22:37 ` Jakub Narebski
2008-06-20 0:48 ` Junio C Hamano
1 sibling, 1 reply; 25+ messages in thread
From: Lea Wiemann @ 2008-06-19 20:25 UTC (permalink / raw)
To: git; +Cc: Lea Wiemann
Many error status codes simply default to 403 Forbidden, which is not
correct in most cases. This patch makes gitweb return semantically
correct status codes.
For convenience the die_error function now only takes the status code
without reason as first parameter (e.g. 404 instead of "404 Not
Found"), and it now defaults to 500 (Internal Server Error), even
though the default is not used anywhere.
Also documented status code conventions in die_error.
Signed-off-by: Lea Wiemann <LeWiemann@gmail.com>
---
[Resent with fixed diff to v2 so git-am doesn't get confused. ;-)]
Changes since v2: die_error now adds the reason strings defined by RFC
2616 to the HTTP status code; incorporated Jakub's other suggestions.
Diff to v2 follows.
I didn't use the HTTP_NOT_FOUND etc. suggestion because I found it too
verbose and obtrusive.
Just a friendly reminder, please remember that discussing fairly
trivial changes in-depth might be not a good use of all participants'
time; IOW, sending 340 lines of email about HTTP status codes that
don't *really* matter is probably overkill. (In case you're
interested, the reason why I wrote this patch in the first place is in
order to be able to tell unforeseen errors from expected errors in the
test suite.) I recommend you read http://bikeshed.com/ if you haven't
done so.
Also, increasing the time between patch submission and acceptance
causes added overhead (effort) by itself (i.e. *in addition* to the
time spend on discussing and resending patches); that's because you
cannot base anything on the code without risking merge conflicts.
Please think of that when replying to patches. ;-)
By the way, when you want to make trivial changes, it may be easier
for everyone if you simply implement them and send a follow-up patch
(and it might actually be just as quick as suggesting the changes!).
In your follow-up patch you could provide a diff to the previous
version, and comment on the changes you made (or send it without
comments, even -- oftentimes they're obvious).
Anyways, I hope everyone is happy with this version of the patch.
Diff to v2:
index 3ca45fd..3bc8f69 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2687,9 +2687,11 @@ sub die_error {
my $status = shift || 500;
my $error = shift || "Internal server error";
- # Use a generic "Error" reason, e.g. "404 Error" instead of
- # "404 Not Found". This is permissible by RFC 2616.
- git_header_html("$status Error");
+ my %http_responses = (400 => '400 Bad Request',
+ 403 => '403 Forbidden',
+ 404 => '404 Not Found',
+ 500 => '500 Internal Server Error');
+ git_header_html($http_responses{$status});
print <<EOF;
<div class="page_body">
<br /><br />
@@ -4131,11 +4133,11 @@ sub git_blame {
my $ftype;
gitweb_check_feature('blame')
- or die_error(403, "Blame not allowed");
+ or die_error(403, "Blame view not allowed");
die_error(400, "No file name given") unless $file_name;
$hash_base ||= git_get_head_hash($project);
- die_error(400, "Couldn't find base commit") unless ($hash_base);
+ die_error(404, "Couldn't find base commit") unless ($hash_base);
my %co = parse_commit($hash_base)
or die_error(404, "Commit not found");
if (!defined $hash) {
@@ -4403,7 +4405,7 @@ sub git_tree {
open my $fd, "-|", git_cmd(), "ls-tree", '-z', $hash
or die_error(500, "Open git-ls-tree failed");
my @entries = map { chomp; $_ } <$fd>;
- close $fd or die_error(500, "Reading tree failed");
+ close $fd or die_error(404, "Reading tree failed");
$/ = "\n";
my $refs = git_get_references();
@@ -4641,7 +4643,7 @@ sub git_commit {
$hash, "--"
or die_error(500, "Open git-diff-tree failed");
@difftree = map { chomp; $_ } <$fd>;
- close $fd or die_error(500, "Reading git-diff-tree failed");
+ close $fd or die_error(404, "Reading git-diff-tree failed");
# non-textual hash id's can be cached
my $expires;
@@ -4788,7 +4790,7 @@ sub git_blobdiff {
or die_error(500, "Open git-diff-tree failed");
@difftree = map { chomp; $_ } <$fd>;
close $fd
- or die_error(500, "Reading git-diff-tree failed");
+ or die_error(404, "Reading git-diff-tree failed");
@difftree
or die_error(404, "Blob diff not found");
@@ -4806,7 +4808,7 @@ sub git_blobdiff {
grep { /^:[0-7]{6} [0-7]{6} [0-9a-fA-F]{40} $hash/ }
map { chomp; $_ } <$fd>;
close $fd
- or die_error(500, "Reading git-diff-tree failed");
+ or die_error(404, "Reading git-diff-tree failed");
@difftree
or die_error(404, "Blob diff not found");
gitweb/gitweb.perl | 211 ++++++++++++++++++++++++++--------------------------
1 files changed, 105 insertions(+), 106 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 3a7adae..3bc8f69 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -386,7 +386,7 @@ $projects_list ||= $projectroot;
our $action = $cgi->param('a');
if (defined $action) {
if ($action =~ m/[^0-9a-zA-Z\.\-_]/) {
- die_error(undef, "Invalid action parameter");
+ die_error(400, "Invalid action parameter");
}
}
@@ -399,21 +399,21 @@ if (defined $project) {
($export_ok && !(-e "$projectroot/$project/$export_ok")) ||
($strict_export && !project_in_list($project))) {
undef $project;
- die_error(undef, "No such project");
+ die_error(404, "No such project");
}
}
our $file_name = $cgi->param('f');
if (defined $file_name) {
if (!validate_pathname($file_name)) {
- die_error(undef, "Invalid file parameter");
+ die_error(400, "Invalid file parameter");
}
}
our $file_parent = $cgi->param('fp');
if (defined $file_parent) {
if (!validate_pathname($file_parent)) {
- die_error(undef, "Invalid file parent parameter");
+ die_error(400, "Invalid file parent parameter");
}
}
@@ -421,21 +421,21 @@ if (defined $file_parent) {
our $hash = $cgi->param('h');
if (defined $hash) {
if (!validate_refname($hash)) {
- die_error(undef, "Invalid hash parameter");
+ die_error(400, "Invalid hash parameter");
}
}
our $hash_parent = $cgi->param('hp');
if (defined $hash_parent) {
if (!validate_refname($hash_parent)) {
- die_error(undef, "Invalid hash parent parameter");
+ die_error(400, "Invalid hash parent parameter");
}
}
our $hash_base = $cgi->param('hb');
if (defined $hash_base) {
if (!validate_refname($hash_base)) {
- die_error(undef, "Invalid hash base parameter");
+ die_error(400, "Invalid hash base parameter");
}
}
@@ -447,10 +447,10 @@ our @extra_options = $cgi->param('opt');
if (defined @extra_options) {
foreach my $opt (@extra_options) {
if (not exists $allowed_options{$opt}) {
- die_error(undef, "Invalid option parameter");
+ die_error(400, "Invalid option parameter");
}
if (not grep(/^$action$/, @{$allowed_options{$opt}})) {
- die_error(undef, "Invalid option parameter for this action");
+ die_error(400, "Invalid option parameter for this action");
}
}
}
@@ -458,7 +458,7 @@ if (defined @extra_options) {
our $hash_parent_base = $cgi->param('hpb');
if (defined $hash_parent_base) {
if (!validate_refname($hash_parent_base)) {
- die_error(undef, "Invalid hash parent base parameter");
+ die_error(400, "Invalid hash parent base parameter");
}
}
@@ -466,14 +466,14 @@ if (defined $hash_parent_base) {
our $page = $cgi->param('pg');
if (defined $page) {
if ($page =~ m/[^0-9]/) {
- die_error(undef, "Invalid page parameter");
+ die_error(400, "Invalid page parameter");
}
}
our $searchtype = $cgi->param('st');
if (defined $searchtype) {
if ($searchtype =~ m/[^a-z]/) {
- die_error(undef, "Invalid searchtype parameter");
+ die_error(400, "Invalid searchtype parameter");
}
}
@@ -483,7 +483,7 @@ our $searchtext = $cgi->param('s');
our $search_regexp;
if (defined $searchtext) {
if (length($searchtext) < 2) {
- die_error(undef, "At least two characters are required for search parameter");
+ die_error(403, "At least two characters are required for search parameter");
}
$search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;
}
@@ -580,11 +580,11 @@ if (!defined $action) {
}
}
if (!defined($actions{$action})) {
- die_error(undef, "Unknown action");
+ die_error(400, "Unknown action");
}
if ($action !~ m/^(opml|project_list|project_index)$/ &&
!$project) {
- die_error(undef, "Project needed");
+ die_error(400, "Project needed");
}
$actions{$action}->();
exit;
@@ -1665,7 +1665,7 @@ sub git_get_hash_by_path {
$path =~ s,/+$,,;
open my $fd, "-|", git_cmd(), "ls-tree", $base, "--", $path
- or die_error(undef, "Open git-ls-tree failed");
+ or die_error(500, "Open git-ls-tree failed");
my $line = <$fd>;
close $fd or return undef;
@@ -2127,7 +2127,7 @@ sub parse_commit {
"--max-count=1",
$commit_id,
"--",
- or die_error(undef, "Open git-rev-list failed");
+ or die_error(500, "Open git-rev-list failed");
%co = parse_commit_text(<$fd>, 1);
close $fd;
@@ -2152,7 +2152,7 @@ sub parse_commits {
$commit_id,
"--",
($filename ? ($filename) : ())
- or die_error(undef, "Open git-rev-list failed");
+ or die_error(500, "Open git-rev-list failed");
while (my $line = <$fd>) {
my %co = parse_commit_text($line);
push @cos, \%co;
@@ -2672,11 +2672,26 @@ sub git_footer_html {
"</html>";
}
+# die_error(<http_status_code>, <error_message>)
+# Example: die_error(404, 'Hash not found')
+# By convention, use the following status codes (as defined in RFC 2616):
+# 400: Invalid or missing CGI parameters, or
+# requested object exists but has wrong type.
+# 403: Requested feature (like "pickaxe" or "snapshot") not enabled on
+# this server or project.
+# 404: Requested object/revision/project doesn't exist.
+# 500: The server isn't configured properly, or
+# an internal error occurred (e.g. failed assertions caused by bugs), or
+# an unknown error occurred (e.g. the git binary died unexpectedly).
sub die_error {
- my $status = shift || "403 Forbidden";
- my $error = shift || "Malformed query, file missing or permission denied";
-
- git_header_html($status);
+ my $status = shift || 500;
+ my $error = shift || "Internal server error";
+
+ my %http_responses = (400 => '400 Bad Request',
+ 403 => '403 Forbidden',
+ 404 => '404 Not Found',
+ 500 => '500 Internal Server Error');
+ git_header_html($http_responses{$status});
print <<EOF;
<div class="page_body">
<br /><br />
@@ -3924,12 +3939,12 @@ sub git_search_grep_body {
sub git_project_list {
my $order = $cgi->param('o');
if (defined $order && $order !~ m/none|project|descr|owner|age/) {
- die_error(undef, "Unknown order parameter");
+ die_error(400, "Unknown order parameter");
}
my @list = git_get_projects_list();
if (!@list) {
- die_error(undef, "No projects found");
+ die_error(404, "No projects found");
}
git_header_html();
@@ -3947,12 +3962,12 @@ sub git_project_list {
sub git_forks {
my $order = $cgi->param('o');
if (defined $order && $order !~ m/none|project|descr|owner|age/) {
- die_error(undef, "Unknown order parameter");
+ die_error(400, "Unknown order parameter");
}
my @list = git_get_projects_list($project);
if (!@list) {
- die_error(undef, "No forks found");
+ die_error(404, "No forks found");
}
git_header_html();
@@ -4081,7 +4096,7 @@ sub git_tag {
my %tag = parse_tag($hash);
if (! %tag) {
- die_error(undef, "Unknown tag object");
+ die_error(404, "Unknown tag object");
}
git_print_header_div('commit', esc_html($tag{'name'}), $hash);
@@ -4117,26 +4132,25 @@ sub git_blame {
my $fd;
my $ftype;
- my ($have_blame) = gitweb_check_feature('blame');
- if (!$have_blame) {
- die_error('403 Permission denied', "Permission denied");
- }
- die_error('404 Not Found', "File name not defined") if (!$file_name);
+ gitweb_check_feature('blame')
+ or die_error(403, "Blame view not allowed");
+
+ die_error(400, "No file name given") unless $file_name;
$hash_base ||= git_get_head_hash($project);
- die_error(undef, "Couldn't find base commit") unless ($hash_base);
+ die_error(404, "Couldn't find base commit") unless ($hash_base);
my %co = parse_commit($hash_base)
- or die_error(undef, "Reading commit failed");
+ or die_error(404, "Commit not found");
if (!defined $hash) {
$hash = git_get_hash_by_path($hash_base, $file_name, "blob")
- or die_error(undef, "Error looking up file");
+ or die_error(404, "Error looking up file");
}
$ftype = git_get_type($hash);
if ($ftype !~ "blob") {
- die_error('400 Bad Request', "Object is not a blob");
+ die_error(400, "Object is not a blob");
}
open ($fd, "-|", git_cmd(), "blame", '-p', '--',
$file_name, $hash_base)
- or die_error(undef, "Open git-blame failed");
+ or die_error(500, "Open git-blame failed");
git_header_html();
my $formats_nav =
$cgi->a({-href => href(action=>"blob", -replay=>1)},
@@ -4198,7 +4212,7 @@ HTML
print "</td>\n";
}
open (my $dd, "-|", git_cmd(), "rev-parse", "$full_rev^")
- or die_error(undef, "Open git-rev-parse failed");
+ or die_error(500, "Open git-rev-parse failed");
my $parent_commit = <$dd>;
close $dd;
chomp($parent_commit);
@@ -4255,9 +4269,9 @@ sub git_blob_plain {
if (defined $file_name) {
my $base = $hash_base || git_get_head_hash($project);
$hash = git_get_hash_by_path($base, $file_name, "blob")
- or die_error(undef, "Error lookup file");
+ or die_error(404, "Cannot find file");
} else {
- die_error(undef, "No file name defined");
+ die_error(400, "No file name defined");
}
} elsif ($hash =~ m/^[0-9a-fA-F]{40}$/) {
# blobs defined by non-textual hash id's can be cached
@@ -4265,7 +4279,7 @@ sub git_blob_plain {
}
open my $fd, "-|", git_cmd(), "cat-file", "blob", $hash
- or die_error(undef, "Open git-cat-file blob '$hash' failed");
+ or die_error(500, "Open git-cat-file blob '$hash' failed");
# content-type (can include charset)
$type = blob_contenttype($fd, $file_name, $type);
@@ -4297,9 +4311,9 @@ sub git_blob {
if (defined $file_name) {
my $base = $hash_base || git_get_head_hash($project);
$hash = git_get_hash_by_path($base, $file_name, "blob")
- or die_error(undef, "Error lookup file");
+ or die_error(404, "Cannot find file");
} else {
- die_error(undef, "No file name defined");
+ die_error(400, "No file name defined");
}
} elsif ($hash =~ m/^[0-9a-fA-F]{40}$/) {
# blobs defined by non-textual hash id's can be cached
@@ -4308,7 +4322,7 @@ sub git_blob {
my ($have_blame) = gitweb_check_feature('blame');
open my $fd, "-|", git_cmd(), "cat-file", "blob", $hash
- or die_error(undef, "Couldn't cat $file_name, $hash");
+ or die_error(500, "Couldn't cat $file_name, $hash");
my $mimetype = blob_mimetype($fd, $file_name);
if ($mimetype !~ m!^(?:text/|image/(?:gif|png|jpeg)$)! && -B $fd) {
close $fd;
@@ -4389,9 +4403,9 @@ sub git_tree {
}
$/ = "\0";
open my $fd, "-|", git_cmd(), "ls-tree", '-z', $hash
- or die_error(undef, "Open git-ls-tree failed");
+ or die_error(500, "Open git-ls-tree failed");
my @entries = map { chomp; $_ } <$fd>;
- close $fd or die_error(undef, "Reading tree failed");
+ close $fd or die_error(404, "Reading tree failed");
$/ = "\n";
my $refs = git_get_references();
@@ -4481,16 +4495,16 @@ sub git_snapshot {
my $format = $cgi->param('sf');
if (!@supported_fmts) {
- die_error('403 Permission denied', "Permission denied");
+ die_error(403, "Snapshots not allowed");
}
# default to first supported snapshot format
$format ||= $supported_fmts[0];
if ($format !~ m/^[a-z0-9]+$/) {
- die_error(undef, "Invalid snapshot format parameter");
+ die_error(400, "Invalid snapshot format parameter");
} elsif (!exists($known_snapshot_formats{$format})) {
- die_error(undef, "Unknown snapshot format");
+ die_error(400, "Unknown snapshot format");
} elsif (!grep($_ eq $format, @supported_fmts)) {
- die_error(undef, "Unsupported snapshot format");
+ die_error(403, "Unsupported snapshot format");
}
if (!defined $hash) {
@@ -4518,7 +4532,7 @@ sub git_snapshot {
-status => '200 OK');
open my $fd, "-|", $cmd
- or die_error(undef, "Execute git-archive failed");
+ or die_error(500, "Execute git-archive failed");
binmode STDOUT, ':raw';
print <$fd>;
binmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi
@@ -4586,10 +4600,8 @@ sub git_log {
sub git_commit {
$hash ||= $hash_base || "HEAD";
- my %co = parse_commit($hash);
- if (!%co) {
- die_error(undef, "Unknown commit object");
- }
+ my %co = parse_commit($hash)
+ or die_error(404, "Unknown commit object");
my %ad = parse_date($co{'author_epoch'}, $co{'author_tz'});
my %cd = parse_date($co{'committer_epoch'}, $co{'committer_tz'});
@@ -4629,9 +4641,9 @@ sub git_commit {
@diff_opts,
(@$parents <= 1 ? $parent : '-c'),
$hash, "--"
- or die_error(undef, "Open git-diff-tree failed");
+ or die_error(500, "Open git-diff-tree failed");
@difftree = map { chomp; $_ } <$fd>;
- close $fd or die_error(undef, "Reading git-diff-tree failed");
+ close $fd or die_error(404, "Reading git-diff-tree failed");
# non-textual hash id's can be cached
my $expires;
@@ -4724,33 +4736,33 @@ sub git_object {
open my $fd, "-|", quote_command(
git_cmd(), 'cat-file', '-t', $object_id) . ' 2> /dev/null'
- or die_error('404 Not Found', "Object does not exist");
+ or die_error(404, "Object does not exist");
$type = <$fd>;
chomp $type;
close $fd
- or die_error('404 Not Found', "Object does not exist");
+ or die_error(404, "Object does not exist");
# - hash_base and file_name
} elsif ($hash_base && defined $file_name) {
$file_name =~ s,/+$,,;
system(git_cmd(), "cat-file", '-e', $hash_base) == 0
- or die_error('404 Not Found', "Base object does not exist");
+ or die_error(404, "Base object does not exist");
# here errors should not hapen
open my $fd, "-|", git_cmd(), "ls-tree", $hash_base, "--", $file_name
- or die_error(undef, "Open git-ls-tree failed");
+ or die_error(500, "Open git-ls-tree failed");
my $line = <$fd>;
close $fd;
#'100644 blob 0fa3f3a66fb6a137f6ec2c19351ed4d807070ffa panic.c'
unless ($line && $line =~ m/^([0-9]+) (.+) ([0-9a-fA-F]{40})\t/) {
- die_error('404 Not Found', "File or directory for given base does not exist");
+ die_error(404, "File or directory for given base does not exist");
}
$type = $2;
$hash = $3;
} else {
- die_error('404 Not Found', "Not enough information to find object");
+ die_error(400, "Not enough information to find object");
}
print $cgi->redirect(-uri => href(action=>$type, -full=>1,
@@ -4775,12 +4787,12 @@ sub git_blobdiff {
open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
$hash_parent_base, $hash_base,
"--", (defined $file_parent ? $file_parent : ()), $file_name
- or die_error(undef, "Open git-diff-tree failed");
+ or die_error(500, "Open git-diff-tree failed");
@difftree = map { chomp; $_ } <$fd>;
close $fd
- or die_error(undef, "Reading git-diff-tree failed");
+ or die_error(404, "Reading git-diff-tree failed");
@difftree
- or die_error('404 Not Found', "Blob diff not found");
+ or die_error(404, "Blob diff not found");
} elsif (defined $hash &&
$hash =~ /[0-9a-fA-F]{40}/) {
@@ -4789,23 +4801,23 @@ sub git_blobdiff {
# read filtered raw output
open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
$hash_parent_base, $hash_base, "--"
- or die_error(undef, "Open git-diff-tree failed");
+ or die_error(500, "Open git-diff-tree failed");
@difftree =
# ':100644 100644 03b21826... 3b93d5e7... M ls-files.c'
# $hash == to_id
grep { /^:[0-7]{6} [0-7]{6} [0-9a-fA-F]{40} $hash/ }
map { chomp; $_ } <$fd>;
close $fd
- or die_error(undef, "Reading git-diff-tree failed");
+ or die_error(404, "Reading git-diff-tree failed");
@difftree
- or die_error('404 Not Found', "Blob diff not found");
+ or die_error(404, "Blob diff not found");
} else {
- die_error('404 Not Found', "Missing one of the blob diff parameters");
+ die_error(400, "Missing one of the blob diff parameters");
}
if (@difftree > 1) {
- die_error('404 Not Found', "Ambiguous blob diff specification");
+ die_error(400, "Ambiguous blob diff specification");
}
%diffinfo = parse_difftree_raw_line($difftree[0]);
@@ -4826,7 +4838,7 @@ sub git_blobdiff {
'-p', ($format eq 'html' ? "--full-index" : ()),
$hash_parent_base, $hash_base,
"--", (defined $file_parent ? $file_parent : ()), $file_name
- or die_error(undef, "Open git-diff-tree failed");
+ or die_error(500, "Open git-diff-tree failed");
}
# old/legacy style URI
@@ -4862,9 +4874,9 @@ sub git_blobdiff {
open $fd, "-|", git_cmd(), "diff", @diff_opts,
'-p', ($format eq 'html' ? "--full-index" : ()),
$hash_parent, $hash, "--"
- or die_error(undef, "Open git-diff failed");
+ or die_error(500, "Open git-diff failed");
} else {
- die_error('404 Not Found', "Missing one of the blob diff parameters")
+ die_error(400, "Missing one of the blob diff parameters")
unless %diffinfo;
}
@@ -4897,7 +4909,7 @@ sub git_blobdiff {
print "X-Git-Url: " . $cgi->self_url() . "\n\n";
} else {
- die_error(undef, "Unknown blobdiff format");
+ die_error(400, "Unknown blobdiff format");
}
# patch
@@ -4932,10 +4944,8 @@ sub git_blobdiff_plain {
sub git_commitdiff {
my $format = shift || 'html';
$hash ||= $hash_base || "HEAD";
- my %co = parse_commit($hash);
- if (!%co) {
- die_error(undef, "Unknown commit object");
- }
+ my %co = parse_commit($hash)
+ or die_error(404, "Unknown commit object");
# choose format for commitdiff for merge
if (! defined $hash_parent && @{$co{'parents'}} > 1) {
@@ -5017,7 +5027,7 @@ sub git_commitdiff {
open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
"--no-commit-id", "--patch-with-raw", "--full-index",
$hash_parent_param, $hash, "--"
- or die_error(undef, "Open git-diff-tree failed");
+ or die_error(500, "Open git-diff-tree failed");
while (my $line = <$fd>) {
chomp $line;
@@ -5029,10 +5039,10 @@ sub git_commitdiff {
} elsif ($format eq 'plain') {
open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
'-p', $hash_parent_param, $hash, "--"
- or die_error(undef, "Open git-diff-tree failed");
+ or die_error(500, "Open git-diff-tree failed");
} else {
- die_error(undef, "Unknown commitdiff format");
+ die_error(400, "Unknown commitdiff format");
}
# non-textual hash id's can be cached
@@ -5115,19 +5125,15 @@ sub git_history {
$page = 0;
}
my $ftype;
- my %co = parse_commit($hash_base);
- if (!%co) {
- die_error(undef, "Unknown commit object");
- }
+ my %co = parse_commit($hash_base)
+ or die_error(404, "Unknown commit object");
my $refs = git_get_references();
my $limit = sprintf("--max-count=%i", (100 * ($page+1)));
my @commitlist = parse_commits($hash_base, 101, (100 * $page),
- $file_name, "--full-history");
- if (!@commitlist) {
- die_error('404 Not Found', "No such file or directory on given branch");
- }
+ $file_name, "--full-history")
+ or die_error(404, "No such file or directory on given branch");
if (!defined $hash && defined $file_name) {
# some commits could have deleted file in question,
@@ -5141,7 +5147,7 @@ sub git_history {
$ftype = git_get_type($hash);
}
if (!defined $ftype) {
- die_error(undef, "Unknown type of object");
+ die_error(500, "Unknown type of object");
}
my $paging_nav = '';
@@ -5179,19 +5185,16 @@ sub git_history {
}
sub git_search {
- my ($have_search) = gitweb_check_feature('search');
- if (!$have_search) {
- die_error('403 Permission denied', "Permission denied");
- }
+ gitweb_check_feature('search') or die_error(403, "Search is disabled");
if (!defined $searchtext) {
- die_error(undef, "Text field empty");
+ die_error(400, "Text field is empty");
}
if (!defined $hash) {
$hash = git_get_head_hash($project);
}
my %co = parse_commit($hash);
if (!%co) {
- die_error(undef, "Unknown commit object");
+ die_error(404, "Unknown commit object");
}
if (!defined $page) {
$page = 0;
@@ -5201,16 +5204,12 @@ sub git_search {
if ($searchtype eq 'pickaxe') {
# pickaxe may take all resources of your box and run for several minutes
# with every query - so decide by yourself how public you make this feature
- my ($have_pickaxe) = gitweb_check_feature('pickaxe');
- if (!$have_pickaxe) {
- die_error('403 Permission denied', "Permission denied");
- }
+ gitweb_check_feature('pickaxe')
+ or die_error(403, "Pickaxe is disabled");
}
if ($searchtype eq 'grep') {
- my ($have_grep) = gitweb_check_feature('grep');
- if (!$have_grep) {
- die_error('403 Permission denied', "Permission denied");
- }
+ gitweb_check_feature('grep')
+ or die_error(403, "Grep is disabled");
}
git_header_html();
@@ -5484,7 +5483,7 @@ sub git_feed {
# Atom: http://www.atomenabled.org/developers/syndication/
# RSS: http://www.notestips.com/80256B3A007F2692/1/NAMO5P9UPQ
if ($format ne 'rss' && $format ne 'atom') {
- die_error(undef, "Unknown web feed format");
+ die_error(400, "Unknown web feed format");
}
# log/feed of current (HEAD) branch, log of given branch, history of file/directory
--
1.5.6.149.g06c04.dirty
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v2] gitweb: standarize HTTP status codes
2008-06-19 19:08 ` Lea Wiemann
2008-06-19 20:03 ` [PATCH v3] " Lea Wiemann
@ 2008-06-19 22:22 ` Jakub Narebski
1 sibling, 0 replies; 25+ messages in thread
From: Jakub Narebski @ 2008-06-19 22:22 UTC (permalink / raw)
To: Lea Wiemann; +Cc: git
Lea Wiemann wrote:
> Jakub Narebski wrote:
>> Lea Wiemann <lewiemann@gmail.com> writes:
>>>
>>> For convenience the die_error function now only takes the status code
>>> without reason as first parameter (e.g. 404 instead of "404 Not Found")
>>
>> _Whose_ convenience?
>
> The developer's convenience of course. It's plain redundant.
Redundancy isn't always bad.
Moreover I think that "convenience of developer" here is a bit matter
of taste, and the fact if one is web developer, or "accidental" gitweb
developer.
>> * I don't think that RFC 2616 allows blanket replacing reason phrase
>> by generic "Error",
>> * Test::WWW::Mechanize displays both HTTP error status code and
>> reason phrase when get_ok(...) fails:
>> * From the point of view of someone examinimg gitweb.perl code, 400,
>> 403, 404, 500 are _magic numbers_;
>
> I think we're really arguing about the color of the bikeshed here. IMO
> we're not stretching RFC 2616 too much by putting "Error" there (since
> reason codes don't matter on a technical level), and the status codes
> make enough sense to me (and I'm not even a web developer) that I'm not
> concerned about readability.
Well, I didn't know what 400 code meant, and I had to check RFC 2616
for that. '400 Bad Request' is more readable.
But it is a bit bikeshedding. This patch consist of two things: using
better HTTP error status codes (for example getting rid of
403 Forbidden as default catch-all code and using 500 Internal Server
Error for cases where an error _is_ serious server error), and
changing die_error(...) signature / calling convention (meant for
convenience). I agree wholeheartly with first part (modulo using
404 Not Found for errors which usually happens because of user error).
Second part might wait when code stabilizes and there is lull in the
gitweb development (changes shouldn't conflict anyway, but applying
patches might fail because of changed context)...
...but as I can see you have send PATCH v3, in the form I can agree
with.
> I don't think your constants a la HTTP_INVALID are a good idea (I
> remember the status codes in a year, but maybe not the constants);
I can agree with that.
> die_error could figure out the right reason code using a hash. (...)
Good idea. I see it is done this way in PATCH v3.
>>> - die_error(undef, "At least two characters are required for search parameter");
>>> + die_error(403, "At least two characters are required for search parameter");
>>
>> Should gitweb use there '403 Forbidden', or '400 Bad Request'?
>> This is failing static validation of CGI parameters, not a matter of
>> some permissions...
>
> I used 403 in the sense of "sorry, we don't have shorter search strings
> activated for performance reasons". The '2' could even become
> configurable. 400 is fine too, though, I don't care.
I had in mind using '403 Forbidden' for "permission denied" errors,
i.e. for cases where different _configuration_ could result in access.
>>> - close $fd or die_error(undef, "Reading tree failed");
>>> + close $fd or die_error(500, "Reading tree failed");
>>
>> Not O.K. Barring errors in gitweb code this might happen when
>> [X Y Z]. All those are clearly 4xx _client_ errors,
>
> I haven't verified that, so until we have better error handling I prefer
> 500, but I really won't bother objecting to 404.
I'd rather have '404 Not Found' here; in most cases this is client
error, and one should examine URL not mail webmaster.
> FWIW I'm
> assuming that once gitweb uses the new API, that error handling code
> will go away anyway.
I hope that performance impact for non-caching case would be negligible,
and cleaner code would overweigth this concern.
>>> if (!defined $ftype) {
>>> - die_error(undef, "Unknown type of object");
>>> + die_error(500, "Unknown type of object");
>>
>> Errr... shouldn't be '400 Bad Request' here, per convention?
>
> Nope, we didn't get *anything* back, so something weird happened. 500.
Ohhh... right. I didn't get that from seeing only this part.
--
Jakub Narebski
Poland
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3] gitweb: standarize HTTP status codes
2008-06-19 20:25 ` Lea Wiemann
@ 2008-06-19 22:37 ` Jakub Narebski
0 siblings, 0 replies; 25+ messages in thread
From: Jakub Narebski @ 2008-06-19 22:37 UTC (permalink / raw)
To: Lea Wiemann; +Cc: git, Lea Wiemann, Jakub Narebski
Lea Wiemann <lewiemann@gmail.com> writes:
> Changes since v2: die_error now adds the reason strings defined by RFC
> 2616 to the HTTP status code; incorporated Jakub's other suggestions.
> Diff to v2 follows.
This address both of my concerns: first, that for someone examining
Mechanize-based gitweb test number like 403, or 500 would be magical
number without explanation (reason phrase) other than 'Error'.
Second, that for casual / accidental gitweb developer who has to add
or modify a bit of code with die_error(...) wouldn't know which of
"magic number" to use, if the case didn't fail into described
situation. Now it is enough to example die_error(...) in addition to
similar code...
> I didn't use the HTTP_NOT_FOUND etc. suggestion because I found it too
> verbose and obtrusive.
I can agree with that.
> Just a friendly reminder, please remember that discussing fairly
> trivial changes in-depth might be not a good use of all participants'
> time [...]
Well, this was what I though was patch revies... :-/
> Anyways, I hope everyone is happy with this version of the patch.
FWIW:
Acked-by: Jakub Narebski <jnareb@gmail.com>
--
Jakub Narebski
Poland
ShadeHawk on #git
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3] gitweb: standarize HTTP status codes
2008-06-19 20:03 ` [PATCH v3] " Lea Wiemann
2008-06-19 20:25 ` Lea Wiemann
@ 2008-06-20 0:48 ` Junio C Hamano
1 sibling, 0 replies; 25+ messages in thread
From: Junio C Hamano @ 2008-06-20 0:48 UTC (permalink / raw)
To: Lea Wiemann; +Cc: git, Jakub Narebski
Thanks. This will be part of 'next' as of tonight.
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2008-06-20 0:49 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-06-15 21:15 [PATCH] gitweb: return correct HTTP status codes Lea Wiemann
2008-06-15 22:48 ` Jakub Narebski
2008-06-16 15:57 ` Lea Wiemann
2008-06-16 16:43 ` Jakub Narebski
2008-06-16 21:49 ` Lea Wiemann
2008-06-16 22:34 ` Jakub Narebski
2008-06-17 13:53 ` Lea Wiemann
2008-06-16 22:38 ` Junio C Hamano
2008-06-17 14:04 ` Lea Wiemann
2008-06-17 14:33 ` Jakub Narebski
2008-06-17 22:28 ` Lea Wiemann
2008-06-17 22:54 ` Jakub Narebski
2008-06-17 23:47 ` Lea Wiemann
2008-06-18 0:12 ` Jakub Narebski
2008-06-18 1:25 ` Lea Wiemann
2008-06-18 7:35 ` Jakub Narebski
2008-06-16 23:37 ` Jakub Narebski
2008-06-18 0:15 ` [PATCH] gitweb: standarize " Lea Wiemann
2008-06-19 0:51 ` [PATCH v2] " Jakub Narebski
2008-06-19 19:08 ` Lea Wiemann
2008-06-19 20:03 ` [PATCH v3] " Lea Wiemann
2008-06-19 20:25 ` Lea Wiemann
2008-06-19 22:37 ` Jakub Narebski
2008-06-20 0:48 ` Junio C Hamano
2008-06-19 22:22 ` [PATCH v2] " Jakub Narebski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).