Re: [PATCH 8/9] gitweb: Convert output to using indirect file handle

From: Jakub Narebski <jnareb@gmail.com>
Date: Jan 16, 2010, 00:43 UTC
Message-ID: <m3ljfydgmt.fsf@localhost.localdomain>
In-Reply-To: <1263432185-21334-9-git-send-email-warthog9@eaglescrag.net>

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

Show 8 quoted lines

> This converts the output handling of gitweb to using an indirect
> file handle.  This is in preparation to add the caching layer.  This
> is a slight modification to the way I was originally doing it by
> passing the output around.  This should be a nop and this shouldn't
> change the behavior of gitweb.  This does leave error reporting
> functions (die_error specifically) continuing to output directly
> as I want to garauntee those will report their errors regardless of
> what may be going on with respect to the rest of the output.

Signoff?

Compare with my version of this patch:
  http://repo.or.cz/w/git/jnareb-git.git/commitdiff/0dd15cb3f18e2a26fc834fd3b071e6d3ecc00557
in the gitweb/cache-kernel branch:
  http://repo.or.cz/w/git/jnareb-git.git/shortlog/refs/heads/gitweb/cache-kernel

My commit message looks like the following:

.... gitweb: Print to explicit filehandle (preparing for caching)

This means replacing

  print <something>;
by
  print {$out} <something>;

and

  binmode STDOUT, <layer>;
by
  binmode $out, <layer>;

where $out is global variable set to \*STDOUT at the beginning of gitweb, but after reading gitweb config. This way it would be simple to e.g. tie output filehandle or use PerlIO layers to simultaneously write to standard output and to some specified file (like "tee" utility does), or redirect output to a scalar, or a file.

die_error (re)sets $out to \*STDOUT; we would (probably) want to treat errors in a special way, and do not cache them.

The only other differences are reindent of continued lines (if needed), and sometimes word-wrapping lines which this change made too long.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
....

Show 19 quoted lines

> ---
>  gitweb/gitweb.perl |  880 ++++++++++++++++++++++++++--------------------------
>  1 files changed, 448 insertions(+), 432 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index c4a177d..8bb323c 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -450,6 +450,13 @@ our %feature = (
>  		'default' => [0]},
>  );
>  
> +# Basic file handler for all of gitweb, there are two of them.  The first
> +# is the basic text/html file handler which is used for everything other
> +# then the binary files, that uses a separate file handler though
> +# these are both set to STDOUT for the time being.
> +our $output_handler = *STDOUT;
> +our $output_handler_bin = *STDOUT;
> +

First it is not file handleR, but filehandle.

Second, there is no need for separate filehandle for binary files, if you do it correctly (i.e. call binmode on filehandle, and not on STDOUT). When caching is enabled, and 'print {$output_handle} <sth>' prints to in-memory file (or even directly to cache file) it would do conversion, so when reading from cache file we can dump it raw, in binary mode.

Third, wouldn't it be better to use shorter variable name, e.g. $out or $oh, instead of $output_handle? We would be able to align print(f) statements without making lines much longer.

Fourth, there is slight difference between
  our $out = *STDOUT;
and
  out $out = \*STDOUT;
In the former we have global variable, in latter we have indirect
filehandle.  CGI::Cache uses the latter form, IIRC.

Show 9 quoted lines

> @@ -3313,7 +3320,7 @@ EOF
>  		if ($use_pathinfo) {
>  			$action .= "/".esc_url($project);
>  		}
> -		print $cgi->startform(-method => "get", -action => $action) .
> +		print {$output_handler} $cgi->startform(-method => "get", -action => $action) .
>  		      "<div class=\"search\">\n" .
>  		      (!$use_pathinfo &&
>  		      $cgi->input({-name=>"p", -value=>$project, -type=>"hidden"}) . "\n") .

Here for example after change gitweb source stops being nicely aligned. OTOH it makes for bigger patch. In my version I did realign.

You can always check for true differences with "diff -w".

-- 
Jakub Narebski
Poland
ShadeHawk on #git