Jakub Narebski wrote:
quoted text
> 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.

quoted text
>  * 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.

quoted text
>> 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.

quoted text
>> -		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.

quoted text
>> -		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.

quoted text
> *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.

quoted text
>> -	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.

quoted text
>> -	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.

quoted text
>> -	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.

quoted text
> One should examine URL for mistakes, not contact server admin here...

I don't think that'll be a practical concern. ;-)

quoted text
>> -		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.

quoted text
>>  			@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?

quoted text
>> -			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.

quoted text
>> -		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).

quoted text
>>  	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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html