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

From: Jakub Narebski <jnareb@gmail.com>
Date: May 28, 2008, 09:01 UTC
Message-ID: <m33ao23iqv.fsf@localhost.localdomain>
In-Reply-To: <1211930742-24978-1-git-send-email-LeWiemann@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.

Show 15 quoted lines

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

Show 11 quoted lines

>  	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