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

[Jakub Narebski <jnareb@gmail.com>]

Lea Wiemann <lewiemann@gmail.com> 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.

>  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 .= " &sdot; prev";
>  	}
>  
> -	if ($nrevs >= (100 * ($page+1)-1)) {
> +	if ($has_next_link) {
>  		$paging_nav .= " &sdot; " .
>  			$cgi->a({-href => href(-replay=>1, page=>$page+1),
>  			         -accesskey => "n", -title => "Alt-n"}, "next");

This makes logic much simpler.  Nice change.

> @@ -4665,7 +4665,7 @@ sub git_log {
>  
>  	my @commitlist = parse_commits($hash, 101, (100 * $page));

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.
 
>  	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 =

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