Re: [PATCH] gitweb: only display "next" links in logs if there is a next page

Lea Wiemann [off-list ref] writes:

This looks like a no-op hunk, unless format_paging_nav sub has other uses
of $has_more_pages variable.  But the copies of gitweb I have do not begin
with these lines, but they begin like this:

        sub format_paging_nav {
                my ($action, $hash, $head, $page, $nrevs) = @_;
                my $paging_nav;

On what version is your patch based on?  I checked warthog9's copy and
that also seems to be different.

I am not very good at counting, but the change looks no-op to me.  Either
the last index of the list variable is strictly larger than 99, or it is
100 or greater --- aren't they the same thing?

A bit confused I am...

Junio C Hamano wrote:

This looks like a no-op hunk,

Argh!  Apologies, I'm new to git and accidentally only sent my second 
commit -- I should've checked more carefully.  I'll re-send the correct 
patch as a follow-up to this email.  Sorry again! :(

Best,

     Lea

There was a bug in the implementation of the "next" links in
format_paging_nav (for log and shortlog), which caused the next links
to always be displayed, even if there is no next page.  This fixes it.

Signed-off-by: Lea Wiemann <redacted>
---
 gitweb/gitweb.perl |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 8308e22..57a1905 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl

@@ -2756,7 +2756,7 @@ sub git_print_page_nav {
 }
 
 sub format_paging_nav {
-	my ($action, $hash, $head, $page, $nrevs) = @_;
+	my ($action, $hash, $head, $page, $has_next_link) = @_;
 	my $paging_nav;
 
 

@@ -2774,7 +2774,7 @@ sub format_paging_nav {
 		$paging_nav .= " ⋅ prev";
 	}
 
-	if ($nrevs >= (100 * ($page+1)-1)) {
+	if ($has_next_link) {
 		$paging_nav .= " ⋅ " .
 			$cgi->a({-href => href(-replay=>1, page=>$page+1),
 			         -accesskey => "n", -title => "Alt-n"}, "next");

@@ -4665,7 +4665,7 @@ sub git_log {
 
 	my @commitlist = parse_commits($hash, 101, (100 * $page));
 
-	my $paging_nav = format_paging_nav('log', $hash, $head, $page, (100 * ($page+1)));
+	my $paging_nav = format_paging_nav('log', $hash, $head, $page, $#commitlist >= 100);
 
 	git_header_html();
 	git_print_page_nav('log','', $hash,undef,undef, $paging_nav);

@@ -5585,7 +5585,7 @@ sub git_shortlog {
 
 	my @commitlist = parse_commits($hash, 101, (100 * $page));
 
-	my $paging_nav = format_paging_nav('shortlog', $hash, $head, $page, (100 * ($page+1)));
+	my $paging_nav = format_paging_nav('shortlog', $hash, $head, $page, $#commitlist >= 100);
 	my $next_link = '';
 	if ($#commitlist >= 100) {
 		$next_link =

-- 
1.5.5.1

Lea Wiemann wrote:

There was a bug in the implementation of the "next" links in
format_paging_nav (for log and shortlog), which caused the next links
to always be displayed, even if there is no next page.  This fixes it.

Oh, one more thing I forgot to mention: I've tested this with a small 
(single-page) log page and a long log page.  In both cases the "next" 
links get formatted correctly, and they stop linking to the next page on 
the correct (= last) page.  The only thing I haven't tested for is 
off-by-one errors, but I'm reasonably sure that $#commitlist >= 100 is 
right.

-- Lea

Lea Wiemann [off-list ref] writes:

There was a bug in the implementation of the "next" links in
format_paging_nav (for log and shortlog), which caused the next links
to always be displayed, even if there is no next page.  This fixes it.

Thanks for correcting this.

This makes logic much simpler.  Nice change.

Here I have realized the source of this bug.  Some time ago
git-rev-list acquired '--skip=<number>' option to have _git_ skip
commits and not _gitweb_, which improves performance a bit.  It was
required to implement huge performance improvement, namely getting
details for all commits from a single command, otherwise the
performance improvement of calling one git command instead of
$page_size git commands would be much reduced by generating large
amount of data which would be skipped (wound't be used by gitweb).

Unfortunately this change wasn't reviewed carefully enough; old logic
to decide whether to add 'next' link compared (tried to compare)
number of commits receivied with number of commits requested (via
'--max-count=<number>' option).  I guess that having format_paging_nav
decide whether to add "next" link was a bad idea...

-	my $paging_nav = format_paging_nav('log', $hash, $head, $page, (100 * ($page+1)));
+	my $paging_nav = format_paging_nav('log', $hash, $head, $page, $#commitlist >= 100);

I would agree with Junio here that @commitlist > 100 would be more
readable.

Logic goes as the following: we request ($page_size+1) revisions to
know if there are additional revisions, skipping ($page_size * $page)
revisions; gitweb adds 'next' link if it got more than $page_size
revisions.
 

What about git_history()... oh, I see, it generates paging itself, and
soes not use format_paging_nav() subroutine.  But I think it does not
exhibit mentioned (and corrected) error.  BTW. I *guess* that with
href(-replay=>1, ...) gitweb could use format_paging_nav() also for
other pages...

-- 
Jakub Narebski
Poland
ShadeHawk on #git