gitweb: Improvements to commitdiff oview

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

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.

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

index 0a7d65c..8ac60be 100755
@@ -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

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.

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

index 8ac60be..a88e0a8 100755
@@ -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

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.

---
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(-)

index e35ecb4..345e336 100755
@@ -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

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.

---
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(-)

index 8ac60be..345e336 100755
@@ -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

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?

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.

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.

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

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

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.

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

index a88e0a8..e35ecb4 100755
@@ -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