Re: [PATCH v2] gitweb: standarize HTTP status codes

[Jakub Narebski <jnareb@gmail.com>]

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