Re: [PATCH 2/4] gitweb: remove unnecessary test when closing file descriptor

[Jakub Narebski <jnareb@gmail.com>]

Junio C Hamano <gitster@pobox.com> writes:

> Sylvain Rabot <sylvain@abstraction.fr> writes:
> 
> > it happens that closing file descriptor fails whereas
> > the blob is perfectly readable. According to perlman
> > the reasons could be:
> >
> >    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 case we don't mind that close fails.
> >
> > Signed-off-by: Sylvain Rabot <sylvain@abstraction.fr>
> 
> Hmm, do you want a few helped-by lines here?
> 
> I'll queue this to 'pu', but only because I do not care too much about
> this part of the codepath, not because I think this is explained well.

True, I might now agree with code, but I still don't like the
explanation...

> 
> For example, what does "the reasons could be" mean?  If the reasons turned
> out to be totally different, that would make this patch useless?  IOW, is
> it fixing the real issue?  Without knowing the reasons, how can we
> conclude that "In this case" we don't mind?

Well, "in this case" of run_highlighter() we close filehandle from
git-cat-file, which was used only to test if it passes -T test (file
is an ASCII text file (heuristic guess)), to _reopen_ it with
highlighter as a filter.

Also, with test if failed for Sylvain, with test removed it works all
right.

> Having said all that, I agree that you are seeing a failure exactly
> because of the reason you stated above with an unnecessary weak "could
> be".  A filehandle to a pipe to cat-file is opened by the caller of
> blob_mimetype(), it gets peeked at with -T inside the function, then it
> gets peeked at with -B inside the caller (by the way, didn't anybody find
> this sloppy?  Why isn't blob_mimetype() doing all of that itself?), and

I think the -B test is here because -T test is last resort in
blob_mimetype; depending on used mime.types one can get something
other than application/octet-stream for non-text file.  But I agree
that it could have been done better.

> then after that the run_highligher closes the filehandle, because it does
> not want to read from the unadorned cat-file output at all.  Of course,
> cat-file may receive SIGPIPE if we do that, and we know we don't care how
> cat-file died in that particular case.
> 
> But do we care if the first cat-file died due to some other reason?  Is
> there anything that catches the failure mode?

Well, the alternate would be to examine $! or %!, e.g.

> > @@ -3465,8 +3465,7 @@ sub run_highlighter {
> >  	my ($fd, $highlight, $syntax) = @_;
> >  	return $fd unless ($highlight && defined $syntax);
> >  
> > 	close $fd
> > -		or die_error(404, "Reading blob failed");
> > +		or $!{EPIPE} or die_error(404, "Reading blob failed");
> >  	open $fd, quote_command(git_cmd(), "cat-file", "blob", $hash)." | ".
> >  	          quote_command($highlight_bin).
> >  	          " --xhtml --fragment --syntax $syntax |"

Though this version is cryptic (but compact).

-- 
Jakub Narebski
Poland
ShadeHawk on #git