gitweb: Show '...' links in "summary" view only if there are more items

Show "..." links in "summary" view to shortlog, heads (if there are any), and tags (if there are any) only if there are more items to show than shown already.

This means that "..." link is shown below shortened shortlog if there are more than 16 commits, "..." link below shortened heads list if there are more than 16 heads refs (16 branches), "..." link below shortened tags list if there are more than 16 tags.

Added some comments.

---

[...]

And now it is done.

Slightly tested: on my clone (copy) of git repository, which more than 16 commits, more than 16 heads (most temporary, and no longer worked on, few tracking branches) and more than 16 heads show all "..." as it should. Test of freshly created repository shown no "..." for commits (only one commit), no "..." for heads (only one default head 'master'), and no tags list (no tags at all).

By the way, I have _NOT_ applied Robert Fitzsimons patch, but they (this patch and Robert patch) should be not in conflict if we remove last chunk of Robert's patch (this changing --count=17 to --count=15 in git_summary).

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

index 4059894..73877f2 100755
@@ -2915,8 +2915,9 @@ sub git_summary {
 	my $owner = git_get_project_owner($project);
 
 	my $refs = git_get_references();
-	my @taglist  = git_get_tags_list(15);
-	my @headlist = git_get_heads_list(15);
+	# we need to request one more than 16 (0..15) to check if those 16 are all
+	my @taglist  = git_get_tags_list(17);
+	my @headlist = git_get_heads_list(17);
 	my @forklist;
 	my ($check_forks) = gitweb_check_feature('forks');
 

@@ -2952,6 +2953,7 @@ sub git_summary {
 		}
 	}
 
+	# we need to request one more than 16 (0..15) to check if those 16 are all
 	open my $fd, "-|", git_cmd(), "rev-list", "--max-count=17",
 		git_get_head_hash($project), "--"
 		or die_error(undef, "Open git-rev-list failed");

@@ -2959,17 +2961,20 @@ sub git_summary {
 	close $fd;
 	git_print_header_div('shortlog');
 	git_shortlog_body(\@revlist, 0, 15, $refs,
+	                  $#revlist <=  15 ? undef :
 	                  $cgi->a({-href => href(action=>"shortlog")}, "..."));
 
 	if (@taglist) {
 		git_print_header_div('tags');
 		git_tags_body(\@taglist, 0, 15,
+		              $#taglist <=  15 ? undef :
 		              $cgi->a({-href => href(action=>"tags")}, "..."));
 	}
 
 	if (@headlist) {
 		git_print_header_div('heads');
 		git_heads_body(\@headlist, $head, 0, 15,
+		               $#headlist <= 15 ? undef :
 		               $cgi->a({-href => href(action=>"heads")}, "..."));
 	}
 

Part of the patch for "gitweb: Show '...' links in "summary" view only if there are more items" (313ce8cee665447e4476d7e8985b270346a8e5a1) is missing. Add it back in.

---

With all the tooing and frowing this part of the original patch got lost. I'm also resubmiting the original optimizations patches with a few changes.

Robert

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

index ebbc397..80c04b8 100755
@@ -2983,6 +2983,7 @@ sub git_summary {
 	if (@forklist) {
 		git_print_header_div('forks');
 		git_project_list_body(\@forklist, undef, 0, 15,
+		                      $#forklist <= 15 ? undef :
 		                      $cgi->a({-href => href(action=>"forks")}, "..."),
 				      'noheader');
 	}

1.4.4.3.gc902c

Part of the patch for "gitweb: Show '...' links in "summary" view only if there are more items" (313ce8cee665447e4476d7e8985b270346a8e5a1) is missing. Add it back in.

---

With all the tooing and frowing this part of the original patch got lost. I'm also resubmiting the original optimizations patches with a few changes.

Robert

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

index ebbc397..80c04b8 100755
@@ -2983,6 +2983,7 @@ sub git_summary {
 	if (@forklist) {
 		git_print_header_div('forks');
 		git_project_list_body(\@forklist, undef, 0, 15,
+		                      $#forklist <= 15 ? undef :
 		                      $cgi->a({-href => href(action=>"forks")}, "..."),
 				      'noheader');
 	}

1.4.4.3.gc902c

Only return one line of output and we don't need the refname value.

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

index 80c04b8..01e3a8a 100755
@@ -1139,8 +1139,9 @@ sub git_get_last_activity {
 
 	$git_dir = "$projectroot/$path";
 	open($fd, "-|", git_cmd(), 'for-each-ref',
-	     '--format=%(refname) %(committer)',
+	     '--format=%(committer)',
 	     '--sort=-committerdate',
+	     '--count=1',
 	     'refs/heads') or return;
 	my $most_recent = <$fd>;
 	close $fd or return;

1.4.4.3.gc902c

Don't call gitweb_have_snapshot from within the loop.

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

index 01e3a8a..d2ddac8 100755
@@ -2636,6 +2636,8 @@ sub git_shortlog_body {
 	# uses global variable $project
 	my ($revlist, $from, $to, $refs, $extra) = @_;
 
+	my $have_snapshot = gitweb_have_snapshot();
+
 	$from = 0 unless defined $from;
 	$to = $#{$revlist} if (!defined $to || $#{$revlist} < $to);
 

@@ -2663,7 +2665,7 @@ sub git_shortlog_body {
 		      $cgi->a({-href => href(action=>"commit", hash=>$commit)}, "commit") . " | " .
 		      $cgi->a({-href => href(action=>"commitdiff", hash=>$commit)}, "commitdiff") . " | " .
 		      $cgi->a({-href => href(action=>"tree", hash=>$commit, hash_base=>$commit)}, "tree");
-		if (gitweb_have_snapshot()) {
+		if ($have_snapshot) {
 			print " | " . $cgi->a({-href => href(action=>"snapshot", hash=>$commit)}, "snapshot");
 		}
 		print "</td>\n" .

1.4.4.3.gc902c

We don't need to call git_get_head_hash at all just pass in "HEAD" and use the return id field.

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

index d2ddac8..b0e6fdf 100755
@@ -2911,9 +2911,9 @@ sub git_project_index {
 
 sub git_summary {
 	my $descr = git_get_project_description($project) || "none";
-	my $head = git_get_head_hash($project);
-	my %co = parse_commit($head);
+	my %co = parse_commit("HEAD");
 	my %cd = parse_date($co{'committer_epoch'}, $co{'committer_tz'});
+	my $head = $co{'id'};
 
 	my $owner = git_get_project_owner($project);
 

@@ -2960,7 +2960,7 @@ sub git_summary {
 	# we need to request one more than 16 (0..15) to check if
 	# those 16 are all
 	open my $fd, "-|", git_cmd(), "rev-list", "--max-count=17",
-		git_get_head_hash($project), "--"
+		$head, "--"
 		or die_error(undef, "Open git-rev-list failed");
 	my @revlist = map { chomp; $_ } <$fd>;
 	close $fd;

1.4.4.3.gc902c

Refname doesn't hurt and we have it "for free" (meaning: we have to calculate it anyway to get commiterepoch). Although we do not use it.

Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Nice series of patches. Ack (FWIW).

Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Thanks both.

Show "..." links in "summary" view to shortlog, heads (if there are any), and tags (if there are any) only if there are more items to show than shown already.

This means that "..." link is shown below shortened shortlog if there are more than 16 commits, "..." link below shortened heads list if there are more than 16 heads refs (16 branches), "..." link below shortened tags list if there are more than 16 tags.

Modified patch from Jakub to to apply cleanly to master, also preform the same "..." link logic to the forks list.

---

Just removing the last chunk isn't correct, there are two slightly different changes in that chuck. The reduction in the max-count value and a removal of a call to git_get_head_hash.

Here is an updated version of the patch which should apply against master.

Robert

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

index 3bee34c..8d409c7 100755
@@ -2918,8 +2918,9 @@ sub git_summary {
 	my $owner = git_get_project_owner($project);
 
 	my $refs = git_get_references();
-	my @taglist  = git_get_tags_list(15);
-	my @headlist = git_get_heads_list(15);
+	# we need to request one more than 16 (0..15) to check if those 16 are all
+	my @taglist  = git_get_tags_list(16);
+	my @headlist = git_get_heads_list(16);
 	my @forklist;
 	my ($check_forks) = gitweb_check_feature('forks');
 

@@ -2955,30 +2956,35 @@ sub git_summary {
 		}
 	}
 
-	open my $fd, "-|", git_cmd(), "rev-list", "--max-count=16",
+	# we need to request one more than 16 (0..15) to check if those 16 are all
+	open my $fd, "-|", git_cmd(), "rev-list", "--max-count=17",
 		$head, "--"
 		or die_error(undef, "Open git-rev-list failed");
 	my @revlist = map { chomp; $_ } <$fd>;
 	close $fd;
 	git_print_header_div('shortlog');
 	git_shortlog_body(\@revlist, 0, 15, $refs,
+	                  $#revlist <=  15 ? undef :
 	                  $cgi->a({-href => href(action=>"shortlog")}, "..."));
 
 	if (@taglist) {
 		git_print_header_div('tags');
 		git_tags_body(\@taglist, 0, 15,
+		              $#taglist <=  15 ? undef :
 		              $cgi->a({-href => href(action=>"tags")}, "..."));
 	}
 
 	if (@headlist) {
 		git_print_header_div('heads');
 		git_heads_body(\@headlist, $head, 0, 15,
+		               $#headlist <= 15 ? undef :
 		               $cgi->a({-href => href(action=>"heads")}, "..."));
 	}
 
 	if (@forklist) {
 		git_print_header_div('forks');
 		git_project_list_body(\@forklist, undef, 0, 15,
+		                      $#forklist <= 15 ? undef :
 		                      $cgi->a({-href => href(action=>"forks")}, "..."),
 				      'noheader');
 	}

Junio usually puts such comments in brackets (I don't know if it is always used, i.e. if it is some 'convention'), e.g.:

Also perform the same "..." link logic to the forks list.

[rf: Modified patch from Jakub to to apply cleanly to master]

or something like that. Just a nitpick.

By the way, it looks like git_get_projects_list($project) used to get list of forks does not have any count limit option.

[...]

The last chunk I meant to be removed was:

and if we remove that chunk, then your earlier patch would not touch git_summary at all, so mine would cleanly apply (I think).

[...]

Jakub Narebski

Sorry, I haven't noticed git_get_head_hash($project) -> $head Sorry for the noise.

Jakub Narebski

No problem, I tried to find the approreati convention.

That was a copy error on my part. Though looking at the code git_get_tags_list and git_get_heads_list already adds one to the limit value, so if you pass in 17 they will return 18 items.

Robert

I would actually discourage this on messages that are still on the list (i.e. not in commits). I do [jc:] comment when I take the proposed commit log message from the incoming e-mail verbatim but I made modification to the patch text, because otherwise the original submitter cannot tell from the log message if that is meant to be identical to what was submitted.

I've only took a cursory look of the actual patch text, but what Robert sent looked good to me. Thanks, both.

Limit some of the git_cmd's so they only return the number of lines that will be processed. Don't recompute head hash or have_snapshot values.

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

index 5ea3fda..1990f15 100755
@@ -1141,6 +1141,7 @@ sub git_get_last_activity {
 	open($fd, "-|", git_cmd(), 'for-each-ref',
 	     '--format=%(refname) %(committer)',
 	     '--sort=-committerdate',
+	     '--count=1',
 	     'refs/heads') or return;
 	my $most_recent = <$fd>;
 	close $fd or return;

@@ -2559,6 +2560,8 @@ sub git_shortlog_body {
 	# uses global variable $project
 	my ($revlist, $from, $to, $refs, $extra) = @_;
 
+	my $have_snapshot = gitweb_have_snapshot();
+
 	$from = 0 unless defined $from;
 	$to = $#{$revlist} if (!defined $to || $#{$revlist} < $to);
 

@@ -2586,7 +2589,7 @@ sub git_shortlog_body {
 		      $cgi->a({-href => href(action=>"commit", hash=>$commit)}, "commit") . " | " .
 		      $cgi->a({-href => href(action=>"commitdiff", hash=>$commit)}, "commitdiff") . " | " .
 		      $cgi->a({-href => href(action=>"tree", hash=>$commit, hash_base=>$commit)}, "tree");
-		if (gitweb_have_snapshot()) {
+		if ($have_snapshot) {
 			print " | " . $cgi->a({-href => href(action=>"snapshot", hash=>$commit)}, "snapshot");
 		}
 		print "</td>\n" .

@@ -2876,8 +2879,8 @@ sub git_summary {
 		}
 	}
 
-	open my $fd, "-|", git_cmd(), "rev-list", "--max-count=17",
-		git_get_head_hash($project), "--"
+	open my $fd, "-|", git_cmd(), "rev-list", "--max-count=16",
+		$head, "--"
 		or die_error(undef, "Open git-rev-list failed");
 	my @revlist = map { chomp; $_ } <$fd>;
 	close $fd;

1.4.4.2.gee60-dirty

[...]

Actually, that is needed to implement checking if we have more than the number of commits to show to add '...' at the end only if there are some commits which we don't show. The same for heads and tags. (On my short TODO list, but feel free to do it yourself, if you want).

So ack without the last chunk.

Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

The counting code in git_*_body is seriously unusual to tempt anybody who reviews the code to reduce that 17 to 16.

The caller says:

	git_shortlog_body(\@revlist, 0, 15, $refs,
	                  $cgi->a({-href => href(action=>"shortlog")}, "..."));

If it counts up, especially if it counts from zero, the loop would usually say:

	for (i = bottom; i < end; i++)

and anybody who reads that caller would expect it to show 15 lines of output.

But the actual code does this instead:

    sub git_shortlog_body {
            # uses global variable $project
            my ($revlist, $from, $to, $refs, $extra) = @_;

            $from = 0 unless defined $from;
            $to = $#{$revlist} if (!defined $to || $#{$revlist} < $to);
            ...
            for (my $i = $from; $i <= $to; $i++) {
                    ... draw each item ...
            }
            if (defined $extra) {
                    print "<tr>\n" .
                          "<td colspan=\"4\">$extra</td>\n" .
                          "</tr>\n";
            }
    }

By the way, I wonder how that $extra is omitted when $revlist is longer than $to; it should be a trivial fix but it seems to me that it is always spitted out with the current code.

Well, this should be then corrected perhaps to

            my ($revlist, $begin, $end, $refs, $extra) = @_;

            $begin = 0 unless defined $from;
            $end = scalar(@$revlist) if (!defined $end || @$revlist <= $end);
            ...
            for (my $i = $begin; $i < $end; $i++) {
                    ... draw each item ...
            }

I thought that $from..$to ($from <= i <= $to) is more natural and easier to understand than $begin..$end ($begin <= i < $end)... guess I guessed wrong.

We should check if we want to omit $extra, either in caller or in callee, the *_body subroutine itself.

Jakub Narebski

Well, I did not say it was _wrong_, just unusual, so there is nothing to fix.

Check?