[PATCH 0/4 v4] minor gitweb modifications

[PATCH 0/4 v4] minor gitweb modifications

[Sylvain Rabot]

Here the v4 and hopefully final version of minor modifications done to gitweb.

I added a fourth commit on top of v3 which adds a modeline header.

This serie has been improved regarding the comments of :

 - Jakub Narebski <jnareb@gmail.com>
 - Jonathan Nieder <jrnieder@gmail.com>

Regards.

Sylvain Rabot (4):
  gitweb: add extensions to highlight feature map
  gitweb: remove unnecessary test when closing file descriptor
  gitweb: add css class to remote url titles
  gitweb: add vim modeline header which describes gitweb coding rule

 gitweb/gitweb.perl       |   28 +++++++++++++++++-----------
 gitweb/static/gitweb.css |    5 +++++
 2 files changed, 22 insertions(+), 11 deletions(-)

-- 
1.7.3.4.523.g72f0d.dirty

[PATCH 1/4] gitweb: add extensions to highlight feature map

[Sylvain Rabot]

added: sql, php5, phps, bash, zsh, ksh, mk, make

Signed-off-by: Sylvain Rabot <sylvain@abstraction.fr>
---
 gitweb/gitweb.perl |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 4779618..ea984b9 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -250,13 +250,14 @@ our %highlight_ext = (
 	# main extensions, defining name of syntax;
 	# see files in /usr/share/highlight/langDefs/ directory
 	map { $_ => $_ }
-		qw(py c cpp rb java css php sh pl js tex bib xml awk bat ini spec tcl),
+		qw(py c cpp rb java css php sh pl js tex bib xml awk bat ini spec tcl sql make),
 	# alternate extensions, see /etc/highlight/filetypes.conf
 	'h' => 'c',
+	map { $_ => 'sh'  } qw(bash zsh ksh),
 	map { $_ => 'cpp' } qw(cxx c++ cc),
-	map { $_ => 'php' } qw(php3 php4),
+	map { $_ => 'php' } qw(php3 php4 php5 phps),
 	map { $_ => 'pl'  } qw(perl pm), # perhaps also 'cgi'
-	'mak' => 'make',
+	map { $_ => 'make'} qw(mak mk),
 	map { $_ => 'xml' } qw(xhtml html htm),
 );
 
-- 
1.7.3.4.523.g72f0d.dirty

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

[Sylvain Rabot]

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>
---
 gitweb/gitweb.perl |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index ea984b9..eae75ac 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -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");
+	close $fd;
 	open $fd, quote_command(git_cmd(), "cat-file", "blob", $hash)." | ".
 	          quote_command($highlight_bin).
 	          " --xhtml --fragment --syntax $syntax |"
-- 
1.7.3.4.523.g72f0d.dirty

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

[Junio C Hamano]

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.

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?

> ---
>  gitweb/gitweb.perl |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index ea984b9..eae75ac 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -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");
> +	close $fd;
>  	open $fd, quote_command(git_cmd(), "cat-file", "blob", $hash)." | ".
>  	          quote_command($highlight_bin).
>  	          " --xhtml --fragment --syntax $syntax |"
> -- 
> 1.7.3.4.523.g72f0d.dirty

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

[Jakub Narebski]

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

[PATCH 3/4] gitweb: add css class to remote url titles

[Sylvain Rabot]

add a new optional parameter to format_repo_url
routine used to add a css class to the url title cell.

Signed-off-by: Sylvain Rabot <sylvain@abstraction.fr>
---
 gitweb/gitweb.perl       |   17 +++++++++++------
 gitweb/static/gitweb.css |    5 +++++
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index eae75ac..350f8b8 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3881,8 +3881,13 @@ sub git_print_header_div {
 }
 
 sub format_repo_url {
-	my ($name, $url) = @_;
-	return "<tr class=\"metadata_url\"><td>$name</td><td>$url</td></tr>\n";
+	my ($name, $url, $class) = @_;
+
+	if (defined $class) {
+		return "<tr class=\"metadata_url\"><td class=\"$class\">$name</td><td>$url</td></tr>\n";
+	} else {
+		return "<tr class=\"metadata_url\"><td>$name</td><td>$url</td></tr>\n";
+	}
 }
 
 # Group output by placing it in a DIV element and adding a header.
@@ -5146,13 +5151,13 @@ sub git_remote_block {
 
 	if (defined $fetch) {
 		if ($fetch eq $push) {
-			$urls_table .= format_repo_url("URL", $fetch);
+			$urls_table .= format_repo_url("URL", $fetch, 'metadata_remote_fetch_url');
 		} else {
-			$urls_table .= format_repo_url("Fetch URL", $fetch);
-			$urls_table .= format_repo_url("Push URL", $push) if defined $push;
+			$urls_table .= format_repo_url("Fetch URL", $fetch, 'metadata_remote_fetch_url');
+			$urls_table .= format_repo_url("Push URL", $push, 'metadata_remote_push_url') if defined $push;
 		}
 	} elsif (defined $push) {
-		$urls_table .= format_repo_url("Push URL", $push);
+		$urls_table .= format_repo_url("Push URL", $push, 'metadata_remote_push_url');
 	} else {
 		$urls_table .= format_repo_url("", "No remote URL");
 	}
diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
index 79d7eeb..631b20d 100644
--- a/gitweb/static/gitweb.css
+++ b/gitweb/static/gitweb.css
@@ -579,6 +579,11 @@ div.remote {
 	display: inline-block;
 }
 
+.metadata_remote_fetch_url,
+.metadata_remote_push_url {
+	font-weight: bold;
+}
+
 /* Style definition generated by highlight 2.4.5, http://www.andre-simon.de/ */
 
 /* Highlighting theme definition: */
-- 
1.7.3.4.523.g72f0d.dirty

[PATCH 4/4] gitweb: add vim modeline header which describes gitweb coding rule

[Sylvain Rabot]

It is useful for people who have their modeline compliant editor(s)
configured to replace tabs by spaces by default.

Signed-off-by: Sylvain Rabot <sylvain@abstraction.fr>
---
 gitweb/gitweb.perl |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 350f8b8..cfe86b4 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1,4 +1,5 @@
 #!/usr/bin/perl
+# vim: syntax=perl tabstop=4 noexpandtab:
 
 # gitweb - simple web interface to track changes in git repositories
 #
-- 
1.7.3.4.523.g72f0d.dirty

Re: [PATCH 0/4 v4] minor gitweb modifications

[Jonathan Nieder]

(adding back cc: jakub)

Hi,

Sylvain Rabot wrote:

>   gitweb: add extensions to highlight feature map
>   gitweb: remove unnecessary test when closing file descriptor

I like the above two.

>   gitweb: add css class to remote url titles

I had a question (why make the remote url table inconsistent with the
older projects_list table) and suggested a more generic approach in
reply to v2[1]:

	<table class="projects_list">
	<tr id="metadata_desc">
		<td class="metadata_tag">description</td>
		<td>Unnamed repository; edit this file to name it for gitweb.</td>
	</tr>
	<tr id="metadata_owner">
		<td class="metadata_tag">owner</td>
		<td>UNKNOWN</td>
	</tr>
	...

The idea was that the rows are already labelled for use by css, so to
make this stylable all we need to do is use a class for the first
column.  This way if some site operator wants the first column
*always* be bold then that is easy to do.

Another approach with similar effect would be

	<dl class="projects_list">
	<dt>description</dt>
	<dd id="metadata_desc"
		>Unnamed repository; edit this file to name it for gitweb</dd>
	<dt>owner>
	<dd id="metadata_owner"
		>UNKNOWN</dd>
	...

but that does not degrade as well to browsers not supporting css.  Any
thoughts on this?

>   gitweb: add vim modeline header which describes gitweb coding rule

I don't like this one.  Isn't the tabstop whatever the reader wants it
to be (e.g., 8)?  I don't like modelines as a way of documenting
coding standards because

 (1) they are not clear to humans and editors other than vim
 (2) they require annotating each source file separately.

See [1] for an alternative approach to configuring an editor to hack
on git.

Regards,
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/109462/focus=109538

Re: [PATCH 0/4 v4] minor gitweb modifications

[Jonathan Nieder]

Jonathan Nieder wrote:
> Sylvain Rabot wrote:

>>   gitweb: add css class to remote url titles
>
> I had a question (why make the remote url table inconsistent with the
> older projects_list table) and suggested a more generic approach in
> reply to v2[1]:

Sorry, forgot the link before.

[1] http://thread.gmane.org/gmane.comp.version-control.git/164004/focus=164010

Re: [PATCH 0/4 v4] minor gitweb modifications

[Sylvain Rabot]

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

On Sat, 2011-01-01 at 04:41 -0600, Jonathan Nieder wrote:
> (adding back cc: jakub)
> 
> Hi,
> 
> Sylvain Rabot wrote:
> 
> >   gitweb: add extensions to highlight feature map
> >   gitweb: remove unnecessary test when closing file descriptor
> 
> I like the above two.
> 
> >   gitweb: add css class to remote url titles
> 
> I had a question (why make the remote url table inconsistent with the
> older projects_list table) and suggested a more generic approach in
> reply to v2[1]:
> 
> 	<table class="projects_list">
> 	<tr id="metadata_desc">
> 		<td class="metadata_tag">description</td>
> 		<td>Unnamed repository; edit this file to name it for gitweb.</td>
> 	</tr>
> 	<tr id="metadata_owner">
> 		<td class="metadata_tag">owner</td>
> 		<td>UNKNOWN</td>
> 	</tr>
> 	...
> 
> The idea was that the rows are already labelled for use by css, so to
> make this stylable all we need to do is use a class for the first
> column.  This way if some site operator wants the first column
> *always* be bold then that is easy to do.

So your idea is to use the same class for all this kind of tables' first
column ?

> 
> Another approach with similar effect would be
> 
> 	<dl class="projects_list">
> 	<dt>description</dt>
> 	<dd id="metadata_desc"
> 		>Unnamed repository; edit this file to name it for gitweb</dd>
> 	<dt>owner>
> 	<dd id="metadata_owner"
> 		>UNKNOWN</dd>
> 	...
> 
> but that does not degrade as well to browsers not supporting css.  Any
> thoughts on this?

I think table is fine, don't see the need to replace it by dd, dt, dl.

> 
> >   gitweb: add vim modeline header which describes gitweb coding rule
> 
> I don't like this one.  Isn't the tabstop whatever the reader wants it
> to be (e.g., 8)?  I don't like modelines as a way of documenting
> coding standards because
> 
>  (1) they are not clear to humans and editors other than vim
>  (2) they require annotating each source file separately.
> 
> See [1] for an alternative approach to configuring an editor to hack
> on git.
> 
> Regards,
> Jonathan
> 
> [1] http://thread.gmane.org/gmane.comp.version-control.git/109462/focus=109538


-- 
Sylvain Rabot <sylvain@abstraction.fr>

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

Re: [PATCH 0/4 v4] minor gitweb modifications

[Jonathan Nieder]

Sylvain Rabot wrote:
> On Sat, 2011-01-01 at 04:41 -0600, Jonathan Nieder wrote:

>> 	<tr id="metadata_owner">
>> 		<td class="metadata_tag">owner</td>
>> 		<td>UNKNOWN</td>
>> 	</tr>
>> 	...
>>
>> The idea was that the rows are already labelled for use by css, so to
>> make this stylable all we need to do is use a class for the first
>> column.  This way if some site operator wants the first column
>> *always* be bold then that is easy to do.
>
> So your idea is to use the same class for all this kind of tables' first
> column ?

Yes, or more generally to find a way to make the first column always
stylable.  Actually

 tr#metadata_desc > td:first-child {
	...
 }

already does the trick (or

 table.projects_list > tr > td:first-child {
	...
 }

for style that should apply to all first columns) but I haven't
checked how widely supported first-child is.