[PATCH 0/3] gitweb: Improvements to commitdiff oview

[PATCH 0/3] gitweb: Improvements to commitdiff oview

[Jakub Narebski]

This series (patch 2 to be more exact) fixes error seen for example in
  a=commitdiff;h=161332a521fe10c41979bcd493d95e2ac562b7f
in git.git repository.

Shortlog:
 [PATCH 1/3] gitweb: Get rid of git_print_simplified_log
 [PATCH 2/3] gitweb: Filter out commit ID from @difftree in git_commit and git_commitdiff
 [PATCH 3/3] gitweb: Print commit message without title in commitdiff only if there is any

Diffstat:
 gitweb/gitweb.perl |   17 +++++++----------
 1 files changed, 7 insertions(+), 10 deletions(-)

-- 
Jakub Narebski
Poland

[PATCH 1/3] gitweb: Get rid of git_print_simplified_log

[Jakub Narebski]

Replace calls to git_print_simplified_log with its expansion,
i.e. with calling git_print_log with appropriate options.

Remove no longer used git_print_simplified_log subroutine.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 0a7d65c..8ac60be 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1697,15 +1697,6 @@ sub git_print_log ($;%) {
 	}
 }
 
-sub git_print_simplified_log {
-	my $log = shift;
-	my $remove_title = shift;
-
-	git_print_log($log,
-		-final_empty_line=> 1,
-		-remove_title => $remove_title);
-}
-
 # print tree entry (row of git_tree), but without encompassing <tr> element
 sub git_print_tree_entry {
 	my ($t, $basedir, $hash_base, $have_blame) = @_;
@@ -2995,7 +2986,7 @@ sub git_log {
 		      "</div>\n";
 
 		print "<div class=\"log_body\">\n";
-		git_print_simplified_log($co{'comment'});
+		git_print_log($co{'comment'}, -final_empty_line=> 1);
 		print "</div>\n";
 	}
 	git_footer_html();
@@ -3365,7 +3356,7 @@ sub git_commitdiff {
 		git_print_authorship(\%co);
 		print "<div class=\"page_body\">\n";
 		print "<div class=\"log\">\n";
-		git_print_simplified_log($co{'comment'}, 1); # skip title
+		git_print_log($co{'comment'}, -final_empty_line=> 1, -remove_title => 1);
 		print "</div>\n"; # class="log"
 
 	} elsif ($format eq 'plain') {
-- 
1.4.2.1

[PATCH 2/3] gitweb: Filter out commit ID from @difftree in git_commit and git_commitdiff

[Jakub Narebski]

Filter out commit ID output that git-diff-tree adds when called with
only one <tree-ish> (not only for --stdin) in git_commit and
git_commitdiff.

This also works with older git versions, which doesn't have
--no-commit-id option to git-diff-tree.

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 8ac60be..a88e0a8 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3009,6 +3009,9 @@ sub git_commit {
 	my @difftree = map { chomp; $_ } <$fd>;
 	close $fd or die_error(undef, "Reading git-diff-tree failed");
 
+	# filter out commit ID output
+	@difftree = grep(!/^[0-9a-fA-F]{40}$/, @difftree);
+
 	# non-textual hash id's can be cached
 	my $expires;
 	if ($hash =~ m/^[0-9a-fA-F]{40}$/) {
@@ -3327,7 +3330,9 @@ sub git_commitdiff {
 		while (chomp(my $line = <$fd>)) {
 			# empty line ends raw part of diff-tree output
 			last unless $line;
-			push @difftree, $line;
+			# filter out commit ID output
+			push @difftree, $line
+				unless $line =~ m/^[0-9a-fA-F]{40}$/;
 		}
 
 	} elsif ($format eq 'plain') {
-- 
1.4.2.1

[PATCH 3/3] gitweb: Print commit message without title in commitdiff only if there is any

[Jakub Narebski]

Print the rest of commit message (title, i.e. first line of commit
message, is printed separately) only if there is any.

In repository which uses signoffs this shouldn't happen, because
commit message should consist of at least title and signoff.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index a88e0a8..e35ecb4 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3360,9 +3360,11 @@ sub git_commitdiff {
 		git_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash);
 		git_print_authorship(\%co);
 		print "<div class=\"page_body\">\n";
-		print "<div class=\"log\">\n";
-		git_print_log($co{'comment'}, -final_empty_line=> 1, -remove_title => 1);
-		print "</div>\n"; # class="log"
+		if (@{$co{'comment'}} > 1) {
+			print "<div class=\"log\">\n";
+			git_print_log($co{'comment'}, -final_empty_line=> 1, -remove_title => 1);
+			print "</div>\n"; # class="log"
+		}
 
 	} elsif ($format eq 'plain') {
 		my $refs = git_get_references("tags");
-- 
1.4.2.1

Re: [PATCH 2/3] gitweb: Filter out commit ID from @difftree in git_commit and git_commitdiff

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Filter out commit ID output that git-diff-tree adds when called with
> only one <tree-ish> (not only for --stdin) in git_commit and
> git_commitdiff.

I initially wondered why this, or other existing such sripping,
is necessary in the first place.

The collected result is given to git_difftree_body() and it
feeds parse_difftree_raw_line() with it.  Interestingly enough,
it _is_ prepared to handle the line with the commit object
name.

However, the very initial part of git_difftree_body assumes that
the array it gets does not have the commit object name (i.e. it
counts the array members and says "N files changed").

So I think your change is probably a good one, but I suspect you
probably are better off to make parse_difftree_raw_line() to
barf when it gets the commit object name to make sure that all
callers strip it at the same time; better yet, perhaps you can
have a single function that invokes git-diff-tree -r (with
different parameters) and returns the result that is already
only the difftree body lines?

Re: [PATCH 2/3] gitweb: Filter out commit ID from @difftree in git_commit and git_commitdiff

[Jakub Narebski]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> Filter out commit ID output that git-diff-tree adds when called with
>> only one <tree-ish> (not only for --stdin) in git_commit and
>> git_commitdiff.
> 
> I initially wondered why this, or other existing such sripping,
> is necessary in the first place.
> 
> The collected result is given to git_difftree_body() and it
> feeds parse_difftree_raw_line() with it.  Interestingly enough,
> it _is_ prepared to handle the line with the commit object
> name.

Perhaps I was overeager in adding that. The only place it was used
was git_tree_blame in the proof-of-concept/RFC patch which added
"tree_blame" view, and even there it was discarded solution: using
git-diff-tree can give only commit which changed the file to current
version; it cannot find commit which changed tree/directory (latest
commit which changed any of its files).

This can be useful if at any time we need to parse git-diff-tree --stdin
output.
 
> However, the very initial part of git_difftree_body assumes that
> the array it gets does not have the commit object name (i.e. it
> counts the array members and says "N files changed").

The same is true for git_patchset_body. Moreover if I remember correctly
they count elements of @difftree array (or do equivalent of counting)
_before_ parsing.

> So I think your change is probably a good one, but I suspect you
> probably are better off to make parse_difftree_raw_line() to
> barf when it gets the commit object name to make sure that all
> callers strip it at the same time; 

I'd rather not do that, and leave parse_difftree_raw_line generic.

>                                    better yet, perhaps you can 
> have a single function that invokes git-diff-tree -r (with
> different parameters) and returns the result that is already
> only the difftree body lines?

That would be good idea... if not for the fact that git_commitdiff
uses --patch-with-raw format... but that would make it only slightly
more complicated (strip commit-id, or rather tree-ish from output,
and stop on end of raw part i.e. on first empty line).

I'll do that (unless someone else would do this first).
-- 
Jakub Narebski

[PATCH 4/3] gitweb: Use --no-commit-id in git_commit and git_commitdiff

[Jakub Narebski]

Use --no-commit-id option to git diff-tree command in git_commit and
git_commitdiff to filter out commit ID output that git-diff-tree adds
when called with only one <tree-ish> (not only for --stdin).

This option is in git since at least v1.0.0, so make use of it.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Gaaah, --no-commit-id is at least since v1.0.0, and we do require
--full-history and --git-dir options which are much later additions.

This is "correction" patch.

 gitweb/gitweb.perl |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index e35ecb4..345e336 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3004,14 +3004,12 @@ sub git_commit {
 	if (!defined $parent) {
 		$parent = "--root";
 	}
-	open my $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts, $parent, $hash
+	open my $fd, "-|", git_cmd(), "diff-tree", '-r', "--no-commit-id",
+		@diff_opts, $parent, $hash
 		or die_error(undef, "Open git-diff-tree failed");
 	my @difftree = map { chomp; $_ } <$fd>;
 	close $fd or die_error(undef, "Reading git-diff-tree failed");
 
-	# filter out commit ID output
-	@difftree = grep(!/^[0-9a-fA-F]{40}$/, @difftree);
-
 	# non-textual hash id's can be cached
 	my $expires;
 	if ($hash =~ m/^[0-9a-fA-F]{40}$/) {
@@ -3324,15 +3322,14 @@ sub git_commitdiff {
 	my @difftree;
 	if ($format eq 'html') {
 		open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
+			"--no-commit-id",
 			"--patch-with-raw", "--full-index", $hash_parent, $hash
 			or die_error(undef, "Open git-diff-tree failed");
 
 		while (chomp(my $line = <$fd>)) {
 			# empty line ends raw part of diff-tree output
 			last unless $line;
-			# filter out commit ID output
-			push @difftree, $line
-				unless $line =~ m/^[0-9a-fA-F]{40}$/;
+			push @difftree, $line;
 		}
 
 	} elsif ($format eq 'plain') {
-- 
1.4.2.1

[PATCH 2/3 (amend)] gitweb: Use --no-commit-id in git_commit and git_commitdiff

[Jakub Narebski]

Use --no-commit-id option to git diff-tree command in git_commit and
git_commitdiff to filter out commit ID output that git-diff-tree adds
when called with only one <tree-ish> (not only for --stdin).

This option is in git since at least v1.0.0, so make use of it.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This is "amend" patch. It adds --no-commit-id option to git-diff-tree
invocation instead of removing commit IDs (tree-ish IDs to be more exact)
from git-diff-tree output.

Still, I'm not sure why git-diff-tree outputs tree-ish before diff
when called with only one tree-ish. Bugwards compatibility?

 gitweb/gitweb.perl |   4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 8ac60be..345e336 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3004,7 +3004,8 @@ sub git_commit {
 	if (!defined $parent) {
 		$parent = "--root";
 	}
-	open my $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts, $parent, $hash
+	open my $fd, "-|", git_cmd(), "diff-tree", '-r', "--no-commit-id",
+		@diff_opts, $parent, $hash
 		or die_error(undef, "Open git-diff-tree failed");
 	my @difftree = map { chomp; $_ } <$fd>;
 	close $fd or die_error(undef, "Reading git-diff-tree failed");
@@ -3321,6 +3322,7 @@ sub git_commitdiff {
 	my @difftree;
 	if ($format eq 'html') {
 		open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
+			"--no-commit-id",
 			"--patch-with-raw", "--full-index", $hash_parent, $hash
 			or die_error(undef, "Open git-diff-tree failed");
 
-- 
1.4.2.1