git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jakub Narebski <jnareb@gmail.com>
Date
Jun 19, 2008, 22:22 UTC
Message-ID
<200806200022.39685.jnareb@gmail.com>
In-Reply-To
<485AAEB9.2080100@gmail.com>
Lea Wiemann wrote:
Show 9 quoted lines
> 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.
Show 12 quoted lines
>>  * 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.
Show 10 quoted lines
>>> -		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.
Show 8 quoted lines
>>> -	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.
Show 7 quoted lines
>>>  	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
Previous: Junio C Hamano
Message 25 of 25 in “gitweb: return correct HTTP status codes”
  1. gitweb: return correct HTTP status codesLea Wiemann, Jun 15, 2008
  2. Jakub NarebskiJun 15, 2008
  3. Lea WiemannJun 16, 2008
  4. Jakub NarebskiJun 16, 2008
  5. Lea WiemannJun 16, 2008
  6. Jakub NarebskiJun 16, 2008
  7. Lea WiemannJun 17, 2008
  8. Junio C HamanoJun 16, 2008
  9. Lea WiemannJun 17, 2008
  10. Jakub NarebskiJun 17, 2008
  11. Lea WiemannJun 17, 2008
  12. Jakub NarebskiJun 17, 2008
  13. Lea WiemannJun 17, 2008
  14. Jakub NarebskiJun 18, 2008
  15. Lea WiemannJun 18, 2008
  16. Jakub NarebskiJun 18, 2008
  17. Jakub NarebskiJun 16, 2008
  18. gitweb: standarize HTTP status codesLea Wiemann, Jun 18, 2008
  19. Jakub NarebskiJun 19, 2008
  20. Lea WiemannJun 19, 2008
  21. gitweb: standarize HTTP status codesLea Wiemann, Jun 19, 2008
  22. gitweb: standarize HTTP status codesLea Wiemann, Jun 19, 2008
  23. Jakub NarebskiJun 19, 2008
  24. Junio C HamanoJun 20, 2008
  25. Jakub NarebskiJun 19, 2008

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.