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

On Sun, Jan 24, 2010 at 23:14 +0100, Petr Baudis wrote:

On Fri, Jan 15, 2010 at 04:43:32PM -0800, Jakub Narebski wrote:

quoted

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.

I totally agree, I was going to comment on this too. Please use
something very short, I think even the three characters might be pushing
it a bit. Having long idiom will make it real pain to both read and
write gitweb code.

It would still be nice if we could have some way to avoid this hack
entirely. Couldn't we layer PerlIO::via over STDOUT and continue to use
argument-less print as before?

First, the original version in v2 series, the one that can be found in
42641b1 (GITWEB - File based caching layer, 2009-12-10) commit in
git://git.kernel.org/pub/scm/git/warthog9/gitweb.git gitweb-ml-v2
http://git.kernel.org/?p=git/warthog9/gitweb.git;a=shortlog;h=refs/heads/gitweb-ml-v2
the change was even larger. It was
  print <something>;
replaced by
  $output .= <something>;
and of course make functions that printed sth return formatted output
instead.

Also with original version you paid the price even if caching was turned
off (not used): the whole output had to fit in memory, and there was no
streaming, which meant that the whole output had to be generated before
it could be displayed (decreasing latency).


Second, PerlIO::via is not a good solution.  One solution would be to
require Capture::Tiny if caching is enabled, and just do

  $out = capture {
  	$actions{$action}->();
  }

or even simply (if it is possible with the way 'capture' is prototyped)

  $out = capture $actions{$action};

Another solution would be to use PerlIO::Util to redirect output to
scalar:

  *STDOUT->push_layer(scalar => \$buffer);
  # ...
  *STDOUT->pop_layer();
  
Yet another solution would be to manipulate *STDOUT directly

  $stdout = *STDOUT;
  *STDOUT = $buffer_fh;
  ...
  *STDOUT = $stdout;

for which, I think, to be done correctly we would have to duplicate
(large?) parts of Capture::Tiny.


So there is a bit of rule of preservation of difficulty at work.  Either
we have large patch adding explicit filehandle to all print statements
'print <sth> -> print $out <sth>' but simple code, or have smaller patch
but complicated *STDOUT manipulation, or have small patch but rely on 
non-core CPAN modules present.

I think we (well, in practice, "you" nowadays :) should be less
perfectionist when accepting gitweb contributions, so if the answer is
"it's too hard for me to do right now", that's fine I guess; I know I
can't contribute the code currently. :(

True.  At least two series ('categories' which I have copy locally
in http://repo.or.cz/w/git/jnareb-git.git as gitweb/category, and
'committags' by Marcel M. Cary[1]) that are not yet in gitweb because
of that...

[1]: http://thread.gmane.org/gmane.comp.version-control.git/100418/focus=133141

-- 
Jakub Narebski
Poland