threads / patch / 6036

patch, 3 partsgitweb: Get rid of git_print_simplified_log

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

## tl;dr

8 messages between Oct 24, 2006 and Oct 25, 2006. Diffs are folded; open one to read it.

replies: 7people: 2as markdown or json

Jakub Narebski· Oct 24, 2006, 11:49 UTC · lore

[PATCH 0/3] 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
Jakub Narebski· Oct 24, 2006, 11:52 UTC · re: Jakub Narebski · lore

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(-)
Show changes to gitweb/gitweb.perl +2 −11
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
Jakub Narebski· Oct 24, 2006, 11:54 UTC · re: Jakub Narebski · lore

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

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(-)
Show changes to gitweb/gitweb.perl +6 −1
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
Junio C Hamano· Oct 25, 2006, 04:36 UTC · re: Jakub Narebski · lore

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

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?

Jakub Narebski· Oct 25, 2006, 08:03 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 13 quoted lines
> 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
Jakub Narebski· Oct 25, 2006, 12:17 UTC · re: Jakub Narebski · lore

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

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(-)
Show changes to gitweb/gitweb.perl +4 −7
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
Jakub Narebski· Oct 25, 2006, 12:23 UTC · re: Jakub Narebski · lore

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

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(-)
Show changes to gitweb/gitweb.perl +3 −1
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
Jakub Narebski· Oct 24, 2006, 11:55 UTC · re: Jakub Narebski · lore

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

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(-)
Show changes to gitweb/gitweb.perl +5 −3
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

← back to recent threads