{"thread":{"id":"26165","subject":"[PATCH 1/4] gitweb: add extensions to highlight feature map","startedAt":"2010-12-30T21:20:27Z","lastAt":"2011-01-05T00:50:25Z","messageCount":11,"participants":["Sylvain Rabot","Jonathan Nieder","Junio C Hamano","Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"158763","messageId":"1293744031-17790-1-git-send-email-sylvain@abstraction.fr","threadId":"26165","inReplyTo":null,"subject":"[PATCH 0/4 v4] minor gitweb modifications","fromName":"Sylvain Rabot","fromEmail":"sylvain@abstraction.fr","sentAt":"2010-12-30T21:20:27Z","receivedAt":"2010-12-30T21:20:27Z","isPatch":true,"sender":{"key":"sylvain@abstraction.fr","avatar":"https://avatars.githubusercontent.com/u/153052?v=4"},"body":"Here the v4 and hopefully final version of minor modifications done to gitweb.\n\nI added a fourth commit on top of v3 which adds a modeline header.\n\nThis serie has been improved regarding the comments of :\n\n - Jakub Narebski <jnareb@gmail.com>\n - Jonathan Nieder <jrnieder@gmail.com>\n\nRegards.\n\nSylvain Rabot (4):\n  gitweb: add extensions to highlight feature map\n  gitweb: remove unnecessary test when closing file descriptor\n  gitweb: add css class to remote url titles\n  gitweb: add vim modeline header which describes gitweb coding rule\n\n gitweb/gitweb.perl       |   28 +++++++++++++++++-----------\n gitweb/static/gitweb.css |    5 +++++\n 2 files changed, 22 insertions(+), 11 deletions(-)\n\n-- \n1.7.3.4.523.g72f0d.dirty\n"},{"id":"158762","messageId":"1293744031-17790-2-git-send-email-sylvain@abstraction.fr","threadId":"26165","inReplyTo":"1293744031-17790-1-git-send-email-sylvain@abstraction.fr","subject":"[PATCH 1/4] gitweb: add extensions to highlight feature map","fromName":"Sylvain Rabot","fromEmail":"sylvain@abstraction.fr","sentAt":"2010-12-30T21:20:28Z","receivedAt":"2010-12-30T21:20:28Z","isPatch":true,"sender":{"key":"sylvain@abstraction.fr","avatar":"https://avatars.githubusercontent.com/u/153052?v=4"},"body":"added: sql, php5, phps, bash, zsh, ksh, mk, make\n\nSigned-off-by: Sylvain Rabot <sylvain@abstraction.fr>\n---\n gitweb/gitweb.perl |    7 ++++---\n 1 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 4779618..ea984b9 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -250,13 +250,14 @@ our %highlight_ext = (\n \t# main extensions, defining name of syntax;\n \t# see files in /usr/share/highlight/langDefs/ directory\n \tmap { $_ => $_ }\n-\t\tqw(py c cpp rb java css php sh pl js tex bib xml awk bat ini spec tcl),\n+\t\tqw(py c cpp rb java css php sh pl js tex bib xml awk bat ini spec tcl sql make),\n \t# alternate extensions, see /etc/highlight/filetypes.conf\n \t'h' => 'c',\n+\tmap { $_ => 'sh'  } qw(bash zsh ksh),\n \tmap { $_ => 'cpp' } qw(cxx c++ cc),\n-\tmap { $_ => 'php' } qw(php3 php4),\n+\tmap { $_ => 'php' } qw(php3 php4 php5 phps),\n \tmap { $_ => 'pl'  } qw(perl pm), # perhaps also 'cgi'\n-\t'mak' => 'make',\n+\tmap { $_ => 'make'} qw(mak mk),\n \tmap { $_ => 'xml' } qw(xhtml html htm),\n );\n \n-- \n1.7.3.4.523.g72f0d.dirty\n"},{"id":"158764","messageId":"1293744031-17790-3-git-send-email-sylvain@abstraction.fr","threadId":"26165","inReplyTo":"1293744031-17790-1-git-send-email-sylvain@abstraction.fr","subject":"[PATCH 2/4] gitweb: remove unnecessary test when closing file descriptor","fromName":"Sylvain Rabot","fromEmail":"sylvain@abstraction.fr","sentAt":"2010-12-30T21:20:29Z","receivedAt":"2010-12-30T21:20:29Z","isPatch":true,"sender":{"key":"sylvain@abstraction.fr","avatar":"https://avatars.githubusercontent.com/u/153052?v=4"},"body":"it happens that closing file descriptor fails whereas\nthe blob is perfectly readable. According to perlman\nthe reasons could be:\n\n   If the file handle came from a piped open, \"close\" will additionally\n   return false if one of the other system calls involved fails, or if the\n   program exits with non-zero status.  (If the only problem was that the\n   program exited non-zero, $! will be set to 0.)  Closing a pipe also waits\n   for the process executing on the pipe to complete, in case you want to\n   look at the output of the pipe afterwards, and implicitly puts the exit\n   status value of that command into $?.\n\n   Prematurely closing the read end of a pipe (i.e. before the process writ-\n   ing to it at the other end has closed it) will result in a SIGPIPE being\n   delivered to the writer.  If the other end can't handle that, be sure to\n   read all the data before closing the pipe.\n\nIn this case we don't mind that close fails.\n\nSigned-off-by: Sylvain Rabot <sylvain@abstraction.fr>\n---\n gitweb/gitweb.perl |    3 +--\n 1 files changed, 1 insertions(+), 2 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex ea984b9..eae75ac 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -3465,8 +3465,7 @@ sub run_highlighter {\n \tmy ($fd, $highlight, $syntax) = @_;\n \treturn $fd unless ($highlight && defined $syntax);\n \n-\tclose $fd\n-\t\tor die_error(404, \"Reading blob failed\");\n+\tclose $fd;\n \topen $fd, quote_command(git_cmd(), \"cat-file\", \"blob\", $hash).\" | \".\n \t          quote_command($highlight_bin).\n \t          \" --xhtml --fragment --syntax $syntax |\"\n-- \n1.7.3.4.523.g72f0d.dirty\n"},{"id":"158766","messageId":"1293744031-17790-4-git-send-email-sylvain@abstraction.fr","threadId":"26165","inReplyTo":"1293744031-17790-1-git-send-email-sylvain@abstraction.fr","subject":"[PATCH 3/4] gitweb: add css class to remote url titles","fromName":"Sylvain Rabot","fromEmail":"sylvain@abstraction.fr","sentAt":"2010-12-30T21:20:30Z","receivedAt":"2010-12-30T21:20:30Z","isPatch":true,"sender":{"key":"sylvain@abstraction.fr","avatar":"https://avatars.githubusercontent.com/u/153052?v=4"},"body":"add a new optional parameter to format_repo_url\nroutine used to add a css class to the url title cell.\n\nSigned-off-by: Sylvain Rabot <sylvain@abstraction.fr>\n---\n gitweb/gitweb.perl       |   17 +++++++++++------\n gitweb/static/gitweb.css |    5 +++++\n 2 files changed, 16 insertions(+), 6 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex eae75ac..350f8b8 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -3881,8 +3881,13 @@ sub git_print_header_div {\n }\n \n sub format_repo_url {\n-\tmy ($name, $url) = @_;\n-\treturn \"<tr class=\\\"metadata_url\\\"><td>$name</td><td>$url</td></tr>\\n\";\n+\tmy ($name, $url, $class) = @_;\n+\n+\tif (defined $class) {\n+\t\treturn \"<tr class=\\\"metadata_url\\\"><td class=\\\"$class\\\">$name</td><td>$url</td></tr>\\n\";\n+\t} else {\n+\t\treturn \"<tr class=\\\"metadata_url\\\"><td>$name</td><td>$url</td></tr>\\n\";\n+\t}\n }\n \n # Group output by placing it in a DIV element and adding a header.\n@@ -5146,13 +5151,13 @@ sub git_remote_block {\n \n \tif (defined $fetch) {\n \t\tif ($fetch eq $push) {\n-\t\t\t$urls_table .= format_repo_url(\"URL\", $fetch);\n+\t\t\t$urls_table .= format_repo_url(\"URL\", $fetch, 'metadata_remote_fetch_url');\n \t\t} else {\n-\t\t\t$urls_table .= format_repo_url(\"Fetch URL\", $fetch);\n-\t\t\t$urls_table .= format_repo_url(\"Push URL\", $push) if defined $push;\n+\t\t\t$urls_table .= format_repo_url(\"Fetch URL\", $fetch, 'metadata_remote_fetch_url');\n+\t\t\t$urls_table .= format_repo_url(\"Push URL\", $push, 'metadata_remote_push_url') if defined $push;\n \t\t}\n \t} elsif (defined $push) {\n-\t\t$urls_table .= format_repo_url(\"Push URL\", $push);\n+\t\t$urls_table .= format_repo_url(\"Push URL\", $push, 'metadata_remote_push_url');\n \t} else {\n \t\t$urls_table .= format_repo_url(\"\", \"No remote URL\");\n \t}\ndiff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css\nindex 79d7eeb..631b20d 100644\n--- a/gitweb/static/gitweb.css\n+++ b/gitweb/static/gitweb.css\n@@ -579,6 +579,11 @@ div.remote {\n \tdisplay: inline-block;\n }\n \n+.metadata_remote_fetch_url,\n+.metadata_remote_push_url {\n+\tfont-weight: bold;\n+}\n+\n /* Style definition generated by highlight 2.4.5, http://www.andre-simon.de/ */\n \n /* Highlighting theme definition: */\n-- \n1.7.3.4.523.g72f0d.dirty\n"},{"id":"158765","messageId":"1293744031-17790-5-git-send-email-sylvain@abstraction.fr","threadId":"26165","inReplyTo":"1293744031-17790-1-git-send-email-sylvain@abstraction.fr","subject":"[PATCH 4/4] gitweb: add vim modeline header which describes gitweb coding rule","fromName":"Sylvain Rabot","fromEmail":"sylvain@abstraction.fr","sentAt":"2010-12-30T21:20:31Z","receivedAt":"2010-12-30T21:20:31Z","isPatch":true,"sender":{"key":"sylvain@abstraction.fr","avatar":"https://avatars.githubusercontent.com/u/153052?v=4"},"body":"It is useful for people who have their modeline compliant editor(s)\nconfigured to replace tabs by spaces by default.\n\nSigned-off-by: Sylvain Rabot <sylvain@abstraction.fr>\n---\n gitweb/gitweb.perl |    1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 350f8b8..cfe86b4 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1,4 +1,5 @@\n #!/usr/bin/perl\n+# vim: syntax=perl tabstop=4 noexpandtab:\n \n # gitweb - simple web interface to track changes in git repositories\n #\n-- \n1.7.3.4.523.g72f0d.dirty\n"},{"id":"158799","messageId":"20110101104121.GA12734@burratino","threadId":"26165","inReplyTo":"1293744031-17790-1-git-send-email-sylvain@abstraction.fr","subject":"Re: [PATCH 0/4 v4] minor gitweb modifications","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-01-01T10:41:21Z","receivedAt":"2011-01-01T10:41:21Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(adding back cc: jakub)\n\nHi,\n\nSylvain Rabot wrote:\n\n>   gitweb: add extensions to highlight feature map\n>   gitweb: remove unnecessary test when closing file descriptor\n\nI like the above two.\n\n>   gitweb: add css class to remote url titles\n\nI had a question (why make the remote url table inconsistent with the\nolder projects_list table) and suggested a more generic approach in\nreply to v2[1]:\n\n\t<table class=\"projects_list\">\n\t<tr id=\"metadata_desc\">\n\t\t<td class=\"metadata_tag\">description</td>\n\t\t<td>Unnamed repository; edit this file to name it for gitweb.</td>\n\t</tr>\n\t<tr id=\"metadata_owner\">\n\t\t<td class=\"metadata_tag\">owner</td>\n\t\t<td>UNKNOWN</td>\n\t</tr>\n\t...\n\nThe idea was that the rows are already labelled for use by css, so to\nmake this stylable all we need to do is use a class for the first\ncolumn.  This way if some site operator wants the first column\n*always* be bold then that is easy to do.\n\nAnother approach with similar effect would be\n\n\t<dl class=\"projects_list\">\n\t<dt>description</dt>\n\t<dd id=\"metadata_desc\"\n\t\t>Unnamed repository; edit this file to name it for gitweb</dd>\n\t<dt>owner>\n\t<dd id=\"metadata_owner\"\n\t\t>UNKNOWN</dd>\n\t...\n\nbut that does not degrade as well to browsers not supporting css.  Any\nthoughts on this?\n\n>   gitweb: add vim modeline header which describes gitweb coding rule\n\nI don't like this one.  Isn't the tabstop whatever the reader wants it\nto be (e.g., 8)?  I don't like modelines as a way of documenting\ncoding standards because\n\n (1) they are not clear to humans and editors other than vim\n (2) they require annotating each source file separately.\n\nSee [1] for an alternative approach to configuring an editor to hack\non git.\n\nRegards,\nJonathan\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/109462/focus=109538\n"},{"id":"158803","messageId":"20110101230214.GA1483@burratino","threadId":"26165","inReplyTo":"20110101104121.GA12734@burratino","subject":"Re: [PATCH 0/4 v4] minor gitweb modifications","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-01-01T23:02:14Z","receivedAt":"2011-01-01T23:02:14Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n> Sylvain Rabot wrote:\n\n>>   gitweb: add css class to remote url titles\n>\n> I had a question (why make the remote url table inconsistent with the\n> older projects_list table) and suggested a more generic approach in\n> reply to v2[1]:\n\nSorry, forgot the link before.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/164004/focus=164010\n"},{"id":"158817","messageId":"1293985641.15404.11.camel@kheops","threadId":"26165","inReplyTo":"20110101104121.GA12734@burratino","subject":"Re: [PATCH 0/4 v4] minor gitweb modifications","fromName":"Sylvain Rabot","fromEmail":"sylvain@abstraction.fr","sentAt":"2011-01-02T16:27:21Z","receivedAt":"2011-01-02T16:27:21Z","isPatch":true,"sender":{"key":"sylvain@abstraction.fr","avatar":"https://avatars.githubusercontent.com/u/153052?v=4"},"body":"On Sat, 2011-01-01 at 04:41 -0600, Jonathan Nieder wrote:\n> (adding back cc: jakub)\n> \n> Hi,\n> \n> Sylvain Rabot wrote:\n> \n> >   gitweb: add extensions to highlight feature map\n> >   gitweb: remove unnecessary test when closing file descriptor\n> \n> I like the above two.\n> \n> >   gitweb: add css class to remote url titles\n> \n> I had a question (why make the remote url table inconsistent with the\n> older projects_list table) and suggested a more generic approach in\n> reply to v2[1]:\n> \n> \t<table class=\"projects_list\">\n> \t<tr id=\"metadata_desc\">\n> \t\t<td class=\"metadata_tag\">description</td>\n> \t\t<td>Unnamed repository; edit this file to name it for gitweb.</td>\n> \t</tr>\n> \t<tr id=\"metadata_owner\">\n> \t\t<td class=\"metadata_tag\">owner</td>\n> \t\t<td>UNKNOWN</td>\n> \t</tr>\n> \t...\n> \n> The idea was that the rows are already labelled for use by css, so to\n> make this stylable all we need to do is use a class for the first\n> column.  This way if some site operator wants the first column\n> *always* be bold then that is easy to do.\n\nSo your idea is to use the same class for all this kind of tables' first\ncolumn ?\n\n> \n> Another approach with similar effect would be\n> \n> \t<dl class=\"projects_list\">\n> \t<dt>description</dt>\n> \t<dd id=\"metadata_desc\"\n> \t\t>Unnamed repository; edit this file to name it for gitweb</dd>\n> \t<dt>owner>\n> \t<dd id=\"metadata_owner\"\n> \t\t>UNKNOWN</dd>\n> \t...\n> \n> but that does not degrade as well to browsers not supporting css.  Any\n> thoughts on this?\n\nI think table is fine, don't see the need to replace it by dd, dt, dl.\n\n> \n> >   gitweb: add vim modeline header which describes gitweb coding rule\n> \n> I don't like this one.  Isn't the tabstop whatever the reader wants it\n> to be (e.g., 8)?  I don't like modelines as a way of documenting\n> coding standards because\n> \n>  (1) they are not clear to humans and editors other than vim\n>  (2) they require annotating each source file separately.\n> \n> See [1] for an alternative approach to configuring an editor to hack\n> on git.\n> \n> Regards,\n> Jonathan\n> \n> [1] http://thread.gmane.org/gmane.comp.version-control.git/109462/focus=109538\n\n\n-- \nSylvain Rabot <sylvain@abstraction.fr>\n"},{"id":"158819","messageId":"20110102175314.GB13358@burratino","threadId":"26165","inReplyTo":"1293985641.15404.11.camel@kheops","subject":"Re: [PATCH 0/4 v4] minor gitweb modifications","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-01-02T17:53:14Z","receivedAt":"2011-01-02T17:53:14Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Sylvain Rabot wrote:\n> On Sat, 2011-01-01 at 04:41 -0600, Jonathan Nieder wrote:\n\n>> \t<tr id=\"metadata_owner\">\n>> \t\t<td class=\"metadata_tag\">owner</td>\n>> \t\t<td>UNKNOWN</td>\n>> \t</tr>\n>> \t...\n>>\n>> The idea was that the rows are already labelled for use by css, so to\n>> make this stylable all we need to do is use a class for the first\n>> column.  This way if some site operator wants the first column\n>> *always* be bold then that is easy to do.\n>\n> So your idea is to use the same class for all this kind of tables' first\n> column ?\n\nYes, or more generally to find a way to make the first column always\nstylable.  Actually\n\n tr#metadata_desc > td:first-child {\n\t...\n }\n\nalready does the trick (or\n\n table.projects_list > tr > td:first-child {\n\t...\n }\n\nfor style that should apply to all first columns) but I haven't\nchecked how widely supported first-child is.\n"},{"id":"158926","messageId":"7vaajgdx35.fsf@alter.siamese.dyndns.org","threadId":"26165","inReplyTo":"1293744031-17790-3-git-send-email-sylvain@abstraction.fr","subject":"Re: [PATCH 2/4] gitweb: remove unnecessary test when closing file descriptor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-01-05T00:15:58Z","receivedAt":"2011-01-05T00:15:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sylvain Rabot <sylvain@abstraction.fr> writes:\n\n> it happens that closing file descriptor fails whereas\n> the blob is perfectly readable. According to perlman\n> the reasons could be:\n>\n>    If the file handle came from a piped open, \"close\" will additionally\n>    return false if one of the other system calls involved fails, or if the\n>    program exits with non-zero status.  (If the only problem was that the\n>    program exited non-zero, $! will be set to 0.)  Closing a pipe also waits\n>    for the process executing on the pipe to complete, in case you want to\n>    look at the output of the pipe afterwards, and implicitly puts the exit\n>    status value of that command into $?.\n>\n>    Prematurely closing the read end of a pipe (i.e. before the process writ-\n>    ing to it at the other end has closed it) will result in a SIGPIPE being\n>    delivered to the writer.  If the other end can't handle that, be sure to\n>    read all the data before closing the pipe.\n>\n> In this case we don't mind that close fails.\n>\n> Signed-off-by: Sylvain Rabot <sylvain@abstraction.fr>\n\nHmm, do you want a few helped-by lines here?\n\nI'll queue this to 'pu', but only because I do not care too much about\nthis part of the codepath, not because I think this is explained well.\n\nFor example, what does \"the reasons could be\" mean?  If the reasons turned\nout to be totally different, that would make this patch useless?  IOW, is\nit fixing the real issue?  Without knowing the reasons, how can we\nconclude that \"In this case\" we don't mind?\n\nHaving said all that, I agree that you are seeing a failure exactly\nbecause of the reason you stated above with an unnecessary weak \"could\nbe\".  A filehandle to a pipe to cat-file is opened by the caller of\nblob_mimetype(), it gets peeked at with -T inside the function, then it\ngets peeked at with -B inside the caller (by the way, didn't anybody find\nthis sloppy?  Why isn't blob_mimetype() doing all of that itself?), and\nthen after that the run_highligher closes the filehandle, because it does\nnot want to read from the unadorned cat-file output at all.  Of course,\ncat-file may receive SIGPIPE if we do that, and we know we don't care how\ncat-file died in that particular case.\n\nBut do we care if the first cat-file died due to some other reason?  Is\nthere anything that catches the failure mode?\n\n> ---\n>  gitweb/gitweb.perl |    3 +--\n>  1 files changed, 1 insertions(+), 2 deletions(-)\n>\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index ea984b9..eae75ac 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -3465,8 +3465,7 @@ sub run_highlighter {\n>  \tmy ($fd, $highlight, $syntax) = @_;\n>  \treturn $fd unless ($highlight && defined $syntax);\n>  \n> -\tclose $fd\n> -\t\tor die_error(404, \"Reading blob failed\");\n> +\tclose $fd;\n>  \topen $fd, quote_command(git_cmd(), \"cat-file\", \"blob\", $hash).\" | \".\n>  \t          quote_command($highlight_bin).\n>  \t          \" --xhtml --fragment --syntax $syntax |\"\n> -- \n> 1.7.3.4.523.g72f0d.dirty\n"},{"id":"158931","messageId":"m3y670b2ef.fsf@localhost.localdomain","threadId":"26165","inReplyTo":"7vaajgdx35.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/4] gitweb: remove unnecessary test when closing file descriptor","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-01-05T00:50:25Z","receivedAt":"2011-01-05T00:50:25Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Sylvain Rabot <sylvain@abstraction.fr> writes:\n> \n> > it happens that closing file descriptor fails whereas\n> > the blob is perfectly readable. According to perlman\n> > the reasons could be:\n> >\n> >    If the file handle came from a piped open, \"close\" will additionally\n> >    return false if one of the other system calls involved fails, or if the\n> >    program exits with non-zero status.  (If the only problem was that the\n> >    program exited non-zero, $! will be set to 0.)  Closing a pipe also waits\n> >    for the process executing on the pipe to complete, in case you want to\n> >    look at the output of the pipe afterwards, and implicitly puts the exit\n> >    status value of that command into $?.\n> >\n> >    Prematurely closing the read end of a pipe (i.e. before the process writ-\n> >    ing to it at the other end has closed it) will result in a SIGPIPE being\n> >    delivered to the writer.  If the other end can't handle that, be sure to\n> >    read all the data before closing the pipe.\n> >\n> > In this case we don't mind that close fails.\n> >\n> > Signed-off-by: Sylvain Rabot <sylvain@abstraction.fr>\n> \n> Hmm, do you want a few helped-by lines here?\n> \n> I'll queue this to 'pu', but only because I do not care too much about\n> this part of the codepath, not because I think this is explained well.\n\nTrue, I might now agree with code, but I still don't like the\nexplanation...\n\n> \n> For example, what does \"the reasons could be\" mean?  If the reasons turned\n> out to be totally different, that would make this patch useless?  IOW, is\n> it fixing the real issue?  Without knowing the reasons, how can we\n> conclude that \"In this case\" we don't mind?\n\nWell, \"in this case\" of run_highlighter() we close filehandle from\ngit-cat-file, which was used only to test if it passes -T test (file\nis an ASCII text file (heuristic guess)), to _reopen_ it with\nhighlighter as a filter.\n\nAlso, with test if failed for Sylvain, with test removed it works all\nright.\n\n> Having said all that, I agree that you are seeing a failure exactly\n> because of the reason you stated above with an unnecessary weak \"could\n> be\".  A filehandle to a pipe to cat-file is opened by the caller of\n> blob_mimetype(), it gets peeked at with -T inside the function, then it\n> gets peeked at with -B inside the caller (by the way, didn't anybody find\n> this sloppy?  Why isn't blob_mimetype() doing all of that itself?), and\n\nI think the -B test is here because -T test is last resort in\nblob_mimetype; depending on used mime.types one can get something\nother than application/octet-stream for non-text file.  But I agree\nthat it could have been done better.\n\n> then after that the run_highligher closes the filehandle, because it does\n> not want to read from the unadorned cat-file output at all.  Of course,\n> cat-file may receive SIGPIPE if we do that, and we know we don't care how\n> cat-file died in that particular case.\n> \n> But do we care if the first cat-file died due to some other reason?  Is\n> there anything that catches the failure mode?\n\nWell, the alternate would be to examine $! or %!, e.g.\n\n> > @@ -3465,8 +3465,7 @@ sub run_highlighter {\n> >  \tmy ($fd, $highlight, $syntax) = @_;\n> >  \treturn $fd unless ($highlight && defined $syntax);\n> >  \n> > \tclose $fd\n> > -\t\tor die_error(404, \"Reading blob failed\");\n> > +\t\tor $!{EPIPE} or die_error(404, \"Reading blob failed\");\n> >  \topen $fd, quote_command(git_cmd(), \"cat-file\", \"blob\", $hash).\" | \".\n> >  \t          quote_command($highlight_bin).\n> >  \t          \" --xhtml --fragment --syntax $syntax |\"\n\nThough this version is cryptic (but compact).\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"}]}