Re: [PATCH 3/3] gitweb: remove test when closing file descriptor

[Sylvain Rabot <sylvain@abstraction.fr>]

[-- Attachment #1: Type: text/plain, Size: 2599 bytes --]

On Thu, 2010-12-16 at 14:30 -0800, Jakub Narebski wrote:
> Sylvain Rabot <sylvain@abstraction.fr> writes:
> 
> > it happens that closing file descriptor fails whereas
> > the blob is perfectly readable.
> > 
> > Signed-off-by: Sylvain Rabot <sylvain@abstraction.fr>
> > ---
> >  gitweb/gitweb.perl |    3 +--
> >  1 files changed, 1 insertions(+), 2 deletions(-)
> > 
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index 9398475..9a885b1 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -3450,8 +3450,7 @@ sub run_highlighter {
> >  	my ($fd, $highlight, $syntax) = @_;
> >  	return $fd unless ($highlight && defined $syntax);
> >  
> > -	close $fd
> > -		or die_error(404, "Reading blob failed");
> > +	close $fd;
> 
> Actually what I think happens is the following.  "close $fh" cannot
> usually fail, unless (`perldoc -f close`):
> 
>    If the file handle came from a piped open, "close" will additionally
>    return false if one of the other system calls involved fails, or if the
>    program exits with non-zero status.  (If the only problem was that the
>    program exited non-zero, $! will be set to 0.)  Closing a pipe also waits
>    for the process executing on the pipe to complete, in case you want to
>    look at the output of the pipe afterwards, and implicitly puts the exit
>    status value of that command into $?.
> 
>    Prematurely closing the read end of a pipe (i.e. before the process writ-
>    ing to it at the other end has closed it) will result in a SIGPIPE being
>    delivered to the writer.  If the other end can't handle that, be sure to
>    read all the data before closing the pipe.
> 
> In this place we close read end of pipe after at most reading a few
> bytes (what -T test does), so what might happen is that writer got
> SIGPIPE and "failed".  But we are not interested in this, so we can
> safely ignore return from 'close'.
> 
> I think that some from above explanation should make it to commit
> message.

Thanks for explaining it, I did not have any idea why it was failing.
Can I use your explanation as commit message ?

> 
> >  	open $fd, quote_command(git_cmd(), "cat-file", "blob", $hash)." | ".
> >  	          quote_command($highlight_bin).
> >  	          " --xhtml --fragment --syntax $syntax |"
> 
> P.S. A better solution would be to redirect opened $fd to highlighter,
> instead of closing and reopening a pipe... but I'm not sure how it
> could be implemented.

I don't know either.

> 


-- 
Sylvain Rabot <sylvain@abstraction.fr>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]