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

Sylvain Rabot [off-list ref] 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 <redacted>

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.

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?

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
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?

Junio C Hamano [off-list ref] writes:

Sylvain Rabot [off-list ref] writes:

quoted

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 <redacted>

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.

quoted

@@ -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