[PATCH 0/7] gitweb: Cleanups, fixes and small improvements

[PATCH 0/7] gitweb: Cleanups, fixes and small improvements

[Jakub Narebski]

This series contains fixes, improvements and cleanups.

Fixes are patch 2 and 4, with 4 more important.

This series is based on next 
  (885a89a077c2c79e1fac81c57711566ec30f9bc1)
---
 gitweb/gitweb.perl |   75 +++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 54 insertions(+), 21 deletions(-)

Shortlog:

      gitweb: Restore old git_blame using git-annotate under "annotate"
      gitweb: Remove workaround for git-diff bug fixed in f82cd3c
      gitweb: Improve comments about gitweb features configuration
      gitweb: Fix typo in git_patchset_body
      gitweb: Use 'local $/ = undef;' before 'print <$fd>;'
      gitweb: blobs defined by non-textual hash ids can be cached
      gitweb: Always link to plain version of the blob

[PATCH 1/7] gitweb: Restore old git_blame using git-annotate under "annotate"

[Jakub Narebski]

Rename git_blame to git_annotate, and git_blame2 to git_blame.
Link git_annotate under "annotate" action.  Add link to "blame
in git_annotate, and to "annotate" in git_blame.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index e00a6ed..abd0f0d 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -229,7 +229,8 @@ if (defined $searchtext) {
 
 # dispatch
 my %actions = (
-	"blame" => \&git_blame2,
+	"blame" => \&git_blame,
+	"annotate" => \&git_annotate,
 	"blobdiff" => \&git_blobdiff,
 	"blobdiff_plain" => \&git_blobdiff_plain,
 	"blob" => \&git_blob,
@@ -2166,7 +2167,7 @@ sub git_tag {
 	git_footer_html();
 }
 
-sub git_blame2 {
+sub git_blame {
 	my $fd;
 	my $ftype;
 
@@ -2193,6 +2194,9 @@ sub git_blame2 {
 		$cgi->a({-href => href(action=>"blob", hash=>$hash, hash_base=>$hash_base, file_name=>$file_name)},
 		        "blob") .
 		" | " .
+		$cgi->a({-href => href(action=>"annotate", hash=>$hash, hash_base=>$hash_base, file_name=>$file_name)},
+		        "annotate") .
+		" | " .
 		$cgi->a({-href => href(action=>"blame", file_name=>$file_name)},
 		        "head");
 	git_print_page_nav('','', $hash_base,$co{'tree'},$hash_base, $formats_nav);
@@ -2236,7 +2240,7 @@ HTML
 	git_footer_html();
 }
 
-sub git_blame {
+sub git_annotate {
 	my $fd;
 
 	if (!gitweb_check_feature('blame')) {
@@ -2258,6 +2262,9 @@ sub git_blame {
 		$cgi->a({-href => href(action=>"blob", hash=>$hash, hash_base=>$hash_base, file_name=>$file_name)},
 		        "blob") .
 		" | " .
+		$cgi->a({-href => href(action=>"blame", hash=>$hash, hash_base=>$hash_base, file_name=>$file_name)},
+		        "blame") .
+		" | " .
 		$cgi->a({-href => href(action=>"blame", file_name=>$file_name)},
 		        "head");
 	git_print_page_nav('','', $hash_base,$co{'tree'},$hash_base, $formats_nav);
-- 
1.4.1.1

[PATCH 2/7] gitweb: Remove workaround for git-diff bug fixed in f82cd3c

[Jakub Narebski]

Remove workaround in git_blobdiff for error in git-diff (showing
reversed diff for diff of blobs), corrected in commit
 f82cd3c  Fix "git diff blob1 blob2" showing the diff in reverse.
which is post 1.4.2-rc2 commit.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index abd0f0d..df2f9e5 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2841,8 +2841,7 @@ sub git_blobdiff {
 		}
 
 		# open patch output
-		#open $fd, "-|", $GIT, "diff", '-p', $hash_parent, $hash
-		open $fd, "-|", $GIT, "diff", '-p', $hash, $hash_parent
+		open $fd, "-|", $GIT, "diff", '-p', $hash_parent, $hash
 			or die_error(undef, "Open git-diff failed");
 	} else  {
 		die_error('404 Not Found', "Missing one of the blob diff parameters")
-- 
1.4.1.1

[PATCH 3/7] gitweb: Improve comments about gitweb features configuration

[Jakub Narebski]

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |   25 ++++++++++++++++---------
 1 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index df2f9e5..c29ffa8 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -67,9 +67,16 @@ our $mimetypes_file = undef;
 # You define site-wide feature defaults here; override them with
 # $GITWEB_CONFIG as necessary.
 our %feature = (
-	# feature => {'sub' => feature-sub, 'override' => allow-override, 'default' => [ default options...]
-	# if feature is overridable, feature-sub will be called with default options;
-	# return value indicates if to enable specified feature
+	# feature => {
+	# 	'sub' => feature-sub (subroutine),
+	# 	'override' => allow-override (boolean),
+	# 	'default' => [ default options...] (array reference)}
+	#
+	# if feature is overridable (it means that allow-override has true value,
+	# then feature-sub will be called with default options as parameters;
+	# return value of feature-sub indicates if to enable specified feature
+	#
+	# use gitweb_check_feature(<feature>) to check if <feature> is enabled
 
 	'blame' => {
 		'sub' => \&feature_blame,
@@ -95,9 +102,9 @@ sub gitweb_check_feature {
 }
 
 # To enable system wide have in $GITWEB_CONFIG
-# $feature{'blame'}{'default'} =  [1];
-# To have project specific config enable override in  $GITWEB_CONFIG
-# $feature{'blame'}{'override'} =  1;
+# $feature{'blame'}{'default'} = [1];
+# To have project specific config enable override in $GITWEB_CONFIG
+# $feature{'blame'}{'override'} = 1;
 # and in project config gitweb.blame = 0|1;
 
 sub feature_blame {
@@ -113,9 +120,9 @@ sub feature_blame {
 }
 
 # To disable system wide have in $GITWEB_CONFIG
-# $feature{'snapshot'}{'default'} =  [undef];
-# To have project specific config enable override in  $GITWEB_CONFIG
-# $feature{'blame'}{'override'} =  1;
+# $feature{'snapshot'}{'default'} = [undef];
+# To have project specific config enable override in $GITWEB_CONFIG
+# $feature{'blame'}{'override'} = 1;
 # and in project config  gitweb.snapshot = none|gzip|bzip2
 
 sub feature_snapshot {
-- 
1.4.1.1

[PATCH 4/7] gitweb: Fix typo in git_patchset_body

[Jakub Narebski]

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index c29ffa8..093deab 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1657,7 +1657,7 @@ sub git_patchset_body {
 	print "<div class=\"patchset\">\n";
 
 	LINE:
-	while (my $patch_line @$fd>) {
+	while (my $patch_line = <$fd>) {
 		chomp $patch_line;
 
 		if ($patch_line =~ m/^diff /) { # "git diff" header
-- 
1.4.1.1

[PATCH 5/7] gitweb: Use 'local $/ = undef;' before 'print <$fd>;'

[Jakub Narebski]

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 093deab..eae83e6 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2017,6 +2017,7 @@ sub git_project_list {
 	if (-f $home_text) {
 		print "<div class=\"index_include\">\n";
 		open (my $fd, $home_text);
+		local $/ = undef;
 		print <$fd>;
 		close $fd;
 		print "</div>\n";
@@ -2395,11 +2396,10 @@ sub git_blob_plain {
 
 	print $cgi->header(-type => "$type",
 	                   -content_disposition => "inline; filename=\"$save_as\"");
-	undef $/;
 	binmode STDOUT, ':raw';
+	local $/ = undef;
 	print <$fd>;
 	binmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi
-	$/ = "\n";
 	close $fd;
 }
 
@@ -2585,6 +2585,7 @@ sub git_snapshot {
 	open my $fd, "-|", "$GIT tar-tree $hash \'$project\' | $command" or
 		die_error(undef, "Execute git-tar-tree failed.");
 	binmode STDOUT, ':raw';
+	local $/ = undef;
 	print <$fd>;
 	binmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi
 	close $fd;
-- 
1.4.1.1

[PATCH 6/7] gitweb: blobs defined by non-textual hash ids can be cached

[Jakub Narebski]

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |   20 +++++++++++++++++---
 1 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index eae83e6..f6175bb 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2371,6 +2371,12 @@ sub git_heads {
 }
 
 sub git_blob_plain {
+	# blobs defined by non-textual hash id's can be cached
+	my $expires;
+	if ($hash =~ m/^[0-9a-fA-F]{40}$/) {
+		$expires = "+1d";
+	}
+
 	if (!defined $hash) {
 		if (defined $file_name) {
 			my $base = $hash_base || git_get_head_hash($project);
@@ -2394,8 +2400,10 @@ sub git_blob_plain {
 		$save_as .= '.txt';
 	}
 
-	print $cgi->header(-type => "$type",
-	                   -content_disposition => "inline; filename=\"$save_as\"");
+	print $cgi->header(
+		-type => "$type",
+		-expires=>$expires,
+		-content_disposition => "inline; filename=\"$save_as\"");
 	binmode STDOUT, ':raw';
 	local $/ = undef;
 	print <$fd>;
@@ -2404,6 +2412,12 @@ sub git_blob_plain {
 }
 
 sub git_blob {
+	# blobs defined by non-textual hash id's can be cached
+	my $expires;
+	if ($hash =~ m/^[0-9a-fA-F]{40}$/) {
+		$expires = "+1d";
+	}
+
 	if (!defined $hash) {
 		if (defined $file_name) {
 			my $base = $hash_base || git_get_head_hash($project);
@@ -2421,7 +2435,7 @@ sub git_blob {
 		close $fd;
 		return git_blob_plain($mimetype);
 	}
-	git_header_html();
+	git_header_html(undef, $expires);
 	my $formats_nav = '';
 	if (defined $hash_base && (my %co = parse_commit($hash_base))) {
 		if (defined $file_name) {
-- 
1.4.1.1

[PATCH 7/7] gitweb: Always link to plain version of the blob in git_blob.

[Jakub Narebski]

Link to plain version of the blob. even if no valid hash_base is given
in "blob" view (in git_blob).

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index f6175bb..02d327c 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2461,8 +2461,13 @@ sub git_blob {
 		git_print_page_nav('','', $hash_base,$co{'tree'},$hash_base, $formats_nav);
 		git_print_header_div('commit', esc_html($co{'title'}), $hash_base);
 	} else {
+		$formats_nav .=
+			$cgi->a({-href => href(action=>"blob_plain",
+			                       hash=>$hash, file_name=>$file_name)},
+			        "plain") .
+
 		print "<div class=\"page_nav\">\n" .
-		      "<br/><br/></div>\n" .
+		      "<br/>$formats_nav<br/></div>\n" .
 		      "<div class=\"title\">$hash</div>\n";
 	}
 	git_print_page_path($file_name, "blob", $hash_base);
-- 
1.4.1.1

Re: [PATCH 0/7] gitweb: Cleanups, fixes and small improvements

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

* gitweb: Restore old git_blame using git-annotate under "annotate"

  I actually was hoping to see a patch to remove the git_blame
  which is not used as far as I can see.

  Although we carried annotate and blame in git.git tree for
  quite a while, the intention was always to deprecate one over
  the other once pros-and-cons of each implementation become
  clear (and I think people would not miss annotate if we remove 
  annotate and make it an alias for blame -c anymore).  What's
  the reason we would want to have both?

* gitweb: Remove workaround for git-diff bug fixed in f82cd3c
. gitweb: Fix typo in git_patchset_body

  I think you had separate patches for these; applied.

* gitweb: Use 'local $/ = undef;' before 'print <$fd>;'

  You changed:

	$/ = undef;
	print <$fd>;
        ... hope that nobody depends on standard value of $/
	... around here, which may still break if you did sub
	... calls, the sub did not localize $/ (who would?),
        ... and depended to have a sane $/.
	$/ = "\n";

  to

	local $/ = undef;
	print <$fd>;
        ... hope that nobody depends on standard value of $/
	... until the end of scope, and whoever changes this
        ... sub is careful enough in the future

  which I think is worse.  Introducing an extra scope explicitly
  delimit the part you want to use localized $/ like this

	{ local $/; print <$fd>; } 

  might have been more palatable.  Am I guessing the reason of
  your change wrong?

* gitweb: Always link to plain version of the blob

  Needs a better description to justify the change by describing
  in what way the current implementation is broken.

The ones above I won't apply just yet (I first applied, merged
into "next" and then commented on the earlier 19 series, but I'm
not going to repeat that mistake again this time).

The rest look OK so I'll apply them; [6/7] depended on [5/7] so
I manually adjusted the context.

Re: [PATCH 0/7] gitweb: Cleanups, fixes and small improvements

[Jakub Narebski]

Junio C Hamano wrote:

> Jakub Narebski <jnareb@gmail.com> writes:
> 
> * gitweb: Restore old git_blame using git-annotate under "annotate"
> 
>   I actually was hoping to see a patch to remove the git_blame
>   which is not used as far as I can see.
> 
>   Although we carried annotate and blame in git.git tree for
>   quite a while, the intention was always to deprecate one over
>   the other once pros-and-cons of each implementation become
>   clear (and I think people would not miss annotate if we remove 
>   annotate and make it an alias for blame -c anymore).  What's
>   the reason we would want to have both?

Well, removing it completely might be not a best idea for now,
as git_annotate has for example two more columns ("Age" and "Author") 
which might be usefull.

I guess the intention is to provide patch for those who want 
to improve git_blame, to have comparison with git_annotate,
i.e. patch not applied but available.

> * gitweb: Use 'local $/ = undef;' before 'print <$fd>;'
> 
>   You changed:
> 
>         $/ = undef;
>         print <$fd>;
>         ... hope that nobody depends on standard value of $/
>         ... around here, which may still break if you did sub
>         ... calls, the sub did not localize $/ (who would?),
>         ... and depended to have a sane $/.
>         $/ = "\n";
> 
>   to
> 
>         local $/ = undef;
>         print <$fd>;
>         ... hope that nobody depends on standard value of $/
>         ... until the end of scope, and whoever changes this
>         ... sub is careful enough in the future
> 
>   which I think is worse.  Introducing an extra scope explicitly
>   delimit the part you want to use localized $/ like this
> 
>         { local $/; print <$fd>; } 
> 
>   might have been more palatable.  Am I guessing the reason of
>   your change wrong?

Actually only one change was from '$/ = undef; print <$fd>; $/ = "\n"',
namely at the end of git_blob_plain. I think nobody will print anything
to the end of the sub.

Two other chunks actually _introduced_ 'local $/ = undef', both in the
fairly short 'if' body. First to read $home_text (there change is not that
important, as $home_text should be fairly short), next more important in
git_snapshot.

IMVHO 'local $/ = undef' in all commands that get the rest of the output
from filehandle (and outputting nothing later), like all 'plain' outputs
should be the idiom to use.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH 0/7] gitweb: Cleanups, fixes and small improvements

[Jakub Narebski]

Junio C Hamano wrote:

> * gitweb: Remove workaround for git-diff bug fixed in f82cd3c
> . gitweb: Fix typo in git_patchset_body
> 
>   I think you had separate patches for these; applied.

I think the second one, the one fixing typo ('@$fd>' instead of '= <$fd>')
was not applied. I don't know how this typo got into gitweb, as I have
tested gitweb after the 19-patch series, and this error looks was introduced
in 'next' branch, 157e43b4b0dd5a08eb7a6838192ac58bca62fa5b commit

  gitweb: Streamify patch output in git_commitdiff

This error is critical!
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git