threads / patch / 13697

patchgitweb: only display "next" links in logs if there is a next page

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

## tl;dr

8 messages between May 27, 2008 and May 28, 2008. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Lea Wiemann· May 27, 2008, 22:31 UTC · lore

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 <LeWiemann@gmail.com>
---
 gitweb/gitweb.perl |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)
Show changes to gitweb/gitweb.perl +4 −4
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 308fde2..874f53a 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2752,7 +2752,7 @@ sub git_print_page_nav {
 }
 
 sub format_paging_nav {
-	my ($action, $hash, $head, $page, $has_more_pages) = @_;
+	my ($action, $hash, $head, $page, $has_next_link) = @_;
 	my $paging_nav;
 
 
@@ -2770,7 +2770,7 @@ sub format_paging_nav {
 		$paging_nav .= " &sdot; prev";
 	}
 
-	if ($has_more_pages) {
+	if ($has_next_link) {
 		$paging_nav .= " &sdot; " .
 			$cgi->a({-href => href(-replay=>1, page=>$page+1),
 			         -accesskey => "n", -title => "Alt-n"}, "next");
@@ -4661,7 +4661,7 @@ sub git_log {
 
 	my @commitlist = parse_commits($hash, 101, (100 * $page));
 
-	my $paging_nav = format_paging_nav('log', $hash, $head, $page, $#commitlist > 99);
+	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);
@@ -5581,7 +5581,7 @@ sub git_shortlog {
 
 	my @commitlist = parse_commits($hash, 101, (100 * $page));
 
-	my $paging_nav = format_paging_nav('shortlog', $hash, $head, $page, $#commitlist > 99);
+	my $paging_nav = format_paging_nav('shortlog', $hash, $head, $page, $#commitlist >= 100);
 	my $next_link = '';
 	if ($#commitlist >= 100) {
 		$next_link =

-- 
1.5.5.1
Junio C Hamano· May 27, 2008, 23:08 UTC · re: Lea Wiemann · lore

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

Lea Wiemann <lewiemann@gmail.com> writes:
Show 31 quoted lines
> 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 <LeWiemann@gmail.com>
> ---
>  gitweb/gitweb.perl |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 308fde2..874f53a 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2752,7 +2752,7 @@ sub git_print_page_nav {
>  }
>  
>  sub format_paging_nav {
> -	my ($action, $hash, $head, $page, $has_more_pages) = @_;
> +	my ($action, $hash, $head, $page, $has_next_link) = @_;
>  	my $paging_nav;
>  
>  
> @@ -2770,7 +2770,7 @@ sub format_paging_nav {
>  		$paging_nav .= " &sdot; prev";
>  	}
>  
> -	if ($has_more_pages) {
> +	if ($has_next_link) {
>  		$paging_nav .= " &sdot; " .
>  			$cgi->a({-href => href(-replay=>1, page=>$page+1),
>  			         -accesskey => "n", -title => "Alt-n"}, "next");

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.

Show 18 quoted lines
> @@ -4661,7 +4661,7 @@ sub git_log {
>  
>  	my @commitlist = parse_commits($hash, 101, (100 * $page));
>  
> -	my $paging_nav = format_paging_nav('log', $hash, $head, $page, $#commitlist > 99);
> +	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);
> @@ -5581,7 +5581,7 @@ sub git_shortlog {
>  
>  	my @commitlist = parse_commits($hash, 101, (100 * $page));
>  
> -	my $paging_nav = format_paging_nav('shortlog', $hash, $head, $page, $#commitlist > 99);
> +	my $paging_nav = format_paging_nav('shortlog', $hash, $head, $page, $#commitlist >= 100);
>  	my $next_link = '';
>  	if ($#commitlist >= 100) {
>  		$next_link =

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...
Lea Wiemann· May 27, 2008, 23:23 UTC · re: Junio C Hamano · lore

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

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
Lea Wiemann· May 27, 2008, 23:25 UTC · re: Lea Wiemann · lore

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 <LeWiemann@gmail.com>
---
 gitweb/gitweb.perl |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)
Show changes to gitweb/gitweb.perl +4 −4
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 .= " &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");
@@ -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· May 27, 2008, 23:30 UTC · re: Lea Wiemann · lore

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

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
Junio C Hamano· May 28, 2008, 00:04 UTC · re: Lea Wiemann · lore

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

Lea Wiemann <lewiemann@gmail.com> writes:
Show 11 quoted lines
> 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.

I do not trust people who rely on "tests" to catch counting errors (although I am pretty bad at counting myself and often testsuite helps me catch my own bugs). Instead, let's follow the code to see if it is Ok.

You call format_paging_nav() from two places, both after calling parse_commits() with $maxcount = 101. So @commitlist could have 101 elements (in which case you would need to have next page because you will show only 100 among them), or smaller than that (in which case you will fully show them). In other words, you would want to say 'next' iff

	(@commitlist > 100)

is true. On the other hand, $#commitlist is the last index of @commitlist array, which is one smaller than the number of elements in that array. IOW, when it has 101 elements, it has $commitlist[0] thru $commitlist[100] and $#commitlist == 100. So the above condition is the same as

	($#commitlist >= 100)
So your counting looks correct to me.

I wonder if it is easier to read to use the more modern @array notation than historic $#array notation (it looks very Perl 4 to me), but that is a separate issue. gitweb is littered with such anachronism ;-)

Lea Wiemann· May 28, 2008, 00:10 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
> I do not trust people who rely on "tests" to catch counting errors

Oh, I wasn't trying to say that I didn't "brain-check" my code for correctness -- I just think it's a good idea to actually test it in addition to that. :)

Anyways, thanks for reviewing and confirming it!
> I wonder if it is easier to read to use the more modern @array notation
> than historic $#array notation (it looks very Perl 4 to me), but that is a
> separate issue.  gitweb is littered with such anachronism ;-)
Yup, I was trying to be consistent there.
-- Lea
Jakub Narebski· May 28, 2008, 09:01 UTC · re: Lea Wiemann · lore

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

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

← back to recent threads