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

From: Jakub Narebski <jnareb@gmail.com>
Date: Dec 10, 2010, 01:36 UTC
Message-ID: <m3vd32z9yk.fsf@localhost.localdomain>
In-Reply-To: <1291931844-28454-17-git-send-email-warthog9@eaglescrag.net>

"John 'Warthog9' Hawley" <warthog9@eaglescrag.net> writes:

> This sets up a trap for STDERR as well as STDOUT.  This should
> prevent any transient error messages from git itself percolating
> up to gitweb and outputting errant information before the HTTP
> header has been sent.

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.

Could you give us example which causes described misbehaviour?

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

Show 35 quoted lines

> Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
> ---
>  gitweb/gitweb.perl  |   22 +++++++++++++++++++++-
>  gitweb/lib/cache.pl |   22 ----------------------
>  2 files changed, 21 insertions(+), 23 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 7f8292e..d39982a 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1214,6 +1214,10 @@ sub evaluate_argv {
>  sub change_output {
>  	our $output;
>  
> +	#
> +	# STDOUT
> +	#
> +
>  	# Trap the 'proper' STDOUT to STDOUT_REAL for things like error messages and such
>  	open(STDOUT_REAL,">&STDOUT") or die "Unable to capture STDOUT $!\n";
>  	print STDOUT_REAL "";
> @@ -1223,12 +1227,28 @@ sub change_output {
>  
>  	# Trap STDOUT to the $output variable, which is what I was using in the original
>  	# patch anyway.
> -	open(STDOUT,">", \$output) || die "Unable to open STDOUT: $!"; #open STDOUT handle to use $var
> +	open(STDOUT,">", \$output) || die "Unable to open STDOUT: $!"; #open STDOUT handle to use $output
> +
> +	#
> +	# STDERR
> +	#
> +
> +	# Trap the 'proper' STDOUT to STDOUT_REAL for things like error messages and such
> +	open(STDERR_REAL,">&STDERR") or die "Unable to capture STDERR $!\n";
> +	print STDERR_REAL "";

'print STDERR_REAL "";' nicely solves the spurious warning problem. Nice.

Show 7 quoted lines

> +
> +	# Close STDOUT, so that it isn't being used anymore.
> +	close STDERR;
> +
> +	# Trap STDOUT to the $output variable, which is what I was using in the original
> +	# patch anyway.
> +	open(STDERR,">", \$output_err) || die "Unable to open STDERR: $!"; #open STDERR handle to use $output_err

Err... where $output_err is defined?

Show 38 quoted lines

>  }
>  
>  sub reset_output {
>  	# This basically takes STDOUT_REAL and puts it back as STDOUT
>  	open(STDOUT,">&STDOUT_REAL");
> +	open(STDERR,">&STDERR_REAL");
>  }
>  
>  sub run {
> diff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl
> index 28e4240..a8c902d 100644
> --- a/gitweb/lib/cache.pl
> +++ b/gitweb/lib/cache.pl
> @@ -380,28 +380,6 @@ EOF
>  	return;
>  }
>  
> -sub cacheDisplayErr {
> -
> -	return if ( ! -e "$fullhashpath.err" );
> -
> -	open($cacheFileErr, '<:utf8', "$fullhashpath.err");
> -	$lockStatus = flock($cacheFileErr,LOCK_SH|LOCK_NB);
> -
> -	if (! $lockStatus ){
> -		show_warning(
> -				"<p>".
> -				"<strong>*** Warning ***:</strong> Locking error when trying to lock error cache page, file $fullhashpath.err<br/>/\n".
> -				"This is about as screwed up as it gets folks - see your systems administrator for more help with this.".
> -				"<p>"
> -				);
> -	}
> -
> -	while( <$cacheFileErr> ){
> -		print $_;
> -	}
> -	exit(0);
> -}

Errr... in which patch it was added?

-- 
Jakub Narebski
Poland
ShadeHawk on #git