Re: [PATCH 2/4] gitweb: remove unnecessary test when closing file descriptor
Junio C Hamano <gitster@pobox.com> writes:
Show 27 quoted lines
> 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...
Show 5 quoted lines
>
> 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.
Show 6 quoted lines
> 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.
Show 7 quoted lines
> 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.
Show 10 quoted lines
> > @@ -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
Sylvain Rabot
