Re: [PATCH] gitweb: only display "next" links in logs if there is a next page
From: Jakub Narebski <hidden>
Date: 2016-06-15 22:44:39
Possibly related (same subject, not in this thread)
- 2016-06-15 · [PATCH] gitweb: only display "next" links in logs if there is a next page · Lea Wiemann <hidden>
- 2016-06-15 · Re: [PATCH] gitweb: only display "next" links in logs if there is a next page · Junio C Hamano <hidden>
- 2016-06-15 · Re: [PATCH] gitweb: only display "next" links in logs if there is a next page · Lea Wiemann <hidden>
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.
quoted hunk ↗ jump to hunk
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");
This makes logic much simpler. Nice change.
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
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