Re: [PATCH 2/3] gitweb: add output buffering and associated functions

From: Jakub Narebski <jnareb@gmail.com>
Date: Oct 28, 2010, 09:56 UTC
Message-ID: <m3bp6eiqu5.fsf@localhost.localdomain>
In-Reply-To: <1288226574-19068-3-git-send-email-warthog9@eaglescrag.net>

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

Show 10 quoted lines

> This adds output buffering for gitweb, mainly in preparation for
> caching support.  This is a dramatic change to how caching was being
> done, mainly in passing around the variable manually and such.
> 
> This centrally flips the entire STDOUT to a variable, which after the
> completion of the run, flips it back and does a print on the resulting
> data.
> 
> This should save on the previous 10K line patch (or so) that adds more
> explicit output passing.

Signoff?

Show 40 quoted lines

> ---
>  gitweb/gitweb.perl |   29 +++++++++++++++++++++++++++++
>  1 files changed, 29 insertions(+), 0 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 215a4e9..757ef46 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -30,6 +30,9 @@ BEGIN {
>  
>  our $version = "++GIT_VERSION++";
>  
> +# Output buffer variable
> +$output = "";
> +
>  our ($my_url, $my_uri, $base_url, $path_info, $home_link);
>  sub evaluate_uri {
>  	our $cgi;
> @@ -1151,6 +1154,25 @@ sub evaluate_argv {
>  	);
>  }
>  
> +sub change_output {
> +	our $output;
> +
> +	# 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";
> +
> +	# Close STDOUT, so that it isn't being used anymore.
> +	close STDOUT;
> +
> +	# 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
> +}
> +
> +sub reset_output {
> +	# This basically takes STDOUT_REAL and puts it back as STDOUTl
> +	open(STDOUT,">&STDOUT_REAL");
> +}

Nice solution. I'll steal it for GitwebCache::Capture::Redirect (or something like that, instead of relying on a bit cryptic select($fh)).

Show 8 quoted lines

> @@ -1163,7 +1185,10 @@ sub run {
>  		$pre_dispatch_hook->()
>  			if $pre_dispatch_hook;
>  
> +		change_output();
>  		run_request();
> +		reset_output();
> +		print $output;

Doesn't this enable capture unconditionally?

Wouldn't that screw-up the blame_data part of blame_interactive Ajax-y view? Wouldn't that decrease perceived responsiveness of gitweb?

Show 11 quoted lines

>  
>  		$post_dispatch_hook->()
>  			if $post_dispatch_hook;
> @@ -3673,6 +3698,10 @@ sub die_error {
>  		500 => '500 Internal Server Error',
>  		503 => '503 Service Unavailable',
>  	);
> +	# Reset the output so that we are actually going to STDOUT as opposed
> +	# to buffering the output.
> +	reset_output();
> +

Good.

>  	git_header_html($http_responses{$status}, undef, %opts);
>  	print <<EOF;
>  <div class="page_body">

-- 
Jakub Narebski
Poland
ShadeHawk on #git