Re: [PATCH] gitweb: return correct HTTP status codes

[Junio C Hamano <gitster@pobox.com>]

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();
                }
        }