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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 16, 2008, 22:38 UTC
Message-ID: <7vy75580p1.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <48568D5C.5090909@gmail.com>

Lea Wiemann <lewiemann@gmail.com> writes:

Show 8 quoted lines

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

Show 28 quoted lines

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