Re: [PATCH 16/18] gitweb: When changing output (STDOUT) change STDERR as well

On Sun, 12 Dec 2010, J.H. wrote:

quoted

Hmm... anuthing that happens after 'use CGI::Carp;' is parsed should
have STDERR redirected to web server logs, see CGI::Carp manpage

    [...]
 
       use CGI::Carp

    And the standard warn(), die (), croak(), confess() and carp() calls will
    automagically be replaced with functions that write out nicely time-stamped
    messages to the HTTP server error log.

    [...]

    REDIRECTING ERROR MESSAGES

       By default, error messages are sent to STDERR.  Most HTTPD servers direct
       STDERR to the server's error log.

    [...]

Especially the second part.

That was not what I was seeing, so either something I was doing was
horking how CGI::Carp works, or their claim that "most HTTPD server
direct STDERR to the server's error log" is false.

quoted

Could you give us example which causes described misbehaviour?

While I was working on the trapping of the error pages I started getting
500 errors when going to a non-existent sha1.  Running the command from
the cli revealed that a message from a git command was making it out to
the console.  Redirecting STDERR masked the error from git, and stopped
premature data being sent out before the headers were sent.

Generally if something worked, and stopped working, don't you think
that you should concentrate on fixing your code, and not papering
over the issue?


The fact that "Running the command from the cli revealed that a message
from a git command was making it out to the console." doesn't mean
anything, because when running gitweb from commandline both stdout
and stderr are redirected to terminal, by default.  So you should
worry only if there is premature data being sent to standard output,
with standard error redirected to /dev/null (2>/dev/null).

What CGI::Carp does is (re)define 'die' and 'warn' to support
fatalsToBrowser and warningsToBrowser, and to add timestamp and other
auxiliary information: in the end 'die' calls 'CORE::die', and 'warn'
calls 'CORE::warn' - both of which write to STDERR.  This means that
warnings from git commands sent to standard error do not get timestamp
appended.  Note that standard output from git commands run by gitweb
is always captured.
 

quoted

I have nothing against this patch: if you have to have it, then you
have to have it.  I oly try to understand what might be core cause
behind the issue that this patch is to solve...

I've re-tried this, if you remove this patch and attempt to visit a
non-exist sha1, *boom*

I can only speculate that CGI::Carp only redirects the output inside of
perl, and does not handle the case when called programs (like git) write
more directly to STDERR.

CGI::Carp doesn't redirect output: it adds timestamp and prints it to
STDERR (unless one use 'carpout') to the result of 'die' and 'warn' calls.

*Without your series* when I visit non-existing sha1, or non-existing
file I get correctly 404 error from gitweb.  So you have borked something.

The CGI standard (http://tools.ietf.org/html/rfc3875) doesn't talk about
'standard error' stream at all; on the other hand it talks only about
'standard input' and 'standard output'.  I have checked with simple CGI
script in Perl, that neither using die or warn (both before any HTTP 
headers are send), neither with plain CGI or with mod_perl 
(ModPerl::Registry), with CGI::Carp I never get the error you see.
Without CGI::Carp I get '500 Internal Server Error' instead of nicer
one formatted by CGI::Carp, but I don't get it even without CGI::Carp
with 'warn' and printing to STDERR directly.

The standard error stream either gets discarded (mod_cgid), or is
written to /var/log/httpd/error_log (mod_perl).

-- 
Jakub Narebski
Poland