{"thread":{"id":"13697","subject":"[PATCH] gitweb: only display \"next\" links in logs if there is a next page","startedAt":"2008-05-27T22:31:10Z","lastAt":"2008-05-28T09:01:53Z","messageCount":8,"participants":["Lea Wiemann","Junio C Hamano","Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"77896","messageId":"1211927470-21170-1-git-send-email-LeWiemann@gmail.com","threadId":"13697","inReplyTo":null,"subject":"[PATCH] gitweb: only display \"next\" links in logs if there is a next page","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-05-27T22:31:10Z","receivedAt":"2008-05-27T22:31:10Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"There was a bug in the implementation of the \"next\" links in\nformat_paging_nav (for log and shortlog), which caused the next links\nto always be displayed, even if there is no next page.  This fixes it.\n\nSigned-off-by: Lea Wiemann <LeWiemann@gmail.com>\n---\n gitweb/gitweb.perl |    8 ++++----\n 1 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 308fde2..874f53a 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -2752,7 +2752,7 @@ sub git_print_page_nav {\n }\n \n sub format_paging_nav {\n-\tmy ($action, $hash, $head, $page, $has_more_pages) = @_;\n+\tmy ($action, $hash, $head, $page, $has_next_link) = @_;\n \tmy $paging_nav;\n \n \n@@ -2770,7 +2770,7 @@ sub format_paging_nav {\n \t\t$paging_nav .= \" &sdot; prev\";\n \t}\n \n-\tif ($has_more_pages) {\n+\tif ($has_next_link) {\n \t\t$paging_nav .= \" &sdot; \" .\n \t\t\t$cgi->a({-href => href(-replay=>1, page=>$page+1),\n \t\t\t         -accesskey => \"n\", -title => \"Alt-n\"}, \"next\");\n@@ -4661,7 +4661,7 @@ sub git_log {\n \n \tmy @commitlist = parse_commits($hash, 101, (100 * $page));\n \n-\tmy $paging_nav = format_paging_nav('log', $hash, $head, $page, $#commitlist > 99);\n+\tmy $paging_nav = format_paging_nav('log', $hash, $head, $page, $#commitlist >= 100);\n \n \tgit_header_html();\n \tgit_print_page_nav('log','', $hash,undef,undef, $paging_nav);\n@@ -5581,7 +5581,7 @@ sub git_shortlog {\n \n \tmy @commitlist = parse_commits($hash, 101, (100 * $page));\n \n-\tmy $paging_nav = format_paging_nav('shortlog', $hash, $head, $page, $#commitlist > 99);\n+\tmy $paging_nav = format_paging_nav('shortlog', $hash, $head, $page, $#commitlist >= 100);\n \tmy $next_link = '';\n \tif ($#commitlist >= 100) {\n \t\t$next_link =\n-- \n1.5.5.1\n"},{"id":"77902","messageId":"7viqwzz6p7.fsf@gitster.siamese.dyndns.org","threadId":"13697","inReplyTo":"1211927470-21170-1-git-send-email-LeWiemann@gmail.com","subject":"Re: [PATCH] gitweb: only display \"next\" links in logs if there is a next page","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-27T23:08:04Z","receivedAt":"2008-05-27T23:08:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lea Wiemann <lewiemann@gmail.com> writes:\n\n> There was a bug in the implementation of the \"next\" links in\n> format_paging_nav (for log and shortlog), which caused the next links\n> to always be displayed, even if there is no next page.  This fixes it.\n>\n> Signed-off-by: Lea Wiemann <LeWiemann@gmail.com>\n> ---\n>  gitweb/gitweb.perl |    8 ++++----\n>  1 files changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 308fde2..874f53a 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -2752,7 +2752,7 @@ sub git_print_page_nav {\n>  }\n>  \n>  sub format_paging_nav {\n> -\tmy ($action, $hash, $head, $page, $has_more_pages) = @_;\n> +\tmy ($action, $hash, $head, $page, $has_next_link) = @_;\n>  \tmy $paging_nav;\n>  \n>  \n> @@ -2770,7 +2770,7 @@ sub format_paging_nav {\n>  \t\t$paging_nav .= \" &sdot; prev\";\n>  \t}\n>  \n> -\tif ($has_more_pages) {\n> +\tif ($has_next_link) {\n>  \t\t$paging_nav .= \" &sdot; \" .\n>  \t\t\t$cgi->a({-href => href(-replay=>1, page=>$page+1),\n>  \t\t\t         -accesskey => \"n\", -title => \"Alt-n\"}, \"next\");\n\nThis looks like a no-op hunk, unless format_paging_nav sub has other uses\nof $has_more_pages variable.  But the copies of gitweb I have do not begin\nwith these lines, but they begin like this:\n\n        sub format_paging_nav {\n                my ($action, $hash, $head, $page, $nrevs) = @_;\n                my $paging_nav;\n\nOn what version is your patch based on?  I checked warthog9's copy and\nthat also seems to be different.\n\n> @@ -4661,7 +4661,7 @@ sub git_log {\n>  \n>  \tmy @commitlist = parse_commits($hash, 101, (100 * $page));\n>  \n> -\tmy $paging_nav = format_paging_nav('log', $hash, $head, $page, $#commitlist > 99);\n> +\tmy $paging_nav = format_paging_nav('log', $hash, $head, $page, $#commitlist >= 100);\n>  \n>  \tgit_header_html();\n>  \tgit_print_page_nav('log','', $hash,undef,undef, $paging_nav);\n> @@ -5581,7 +5581,7 @@ sub git_shortlog {\n>  \n>  \tmy @commitlist = parse_commits($hash, 101, (100 * $page));\n>  \n> -\tmy $paging_nav = format_paging_nav('shortlog', $hash, $head, $page, $#commitlist > 99);\n> +\tmy $paging_nav = format_paging_nav('shortlog', $hash, $head, $page, $#commitlist >= 100);\n>  \tmy $next_link = '';\n>  \tif ($#commitlist >= 100) {\n>  \t\t$next_link =\n\nI am not very good at counting, but the change looks no-op to me.  Either\nthe last index of the list variable is strictly larger than 99, or it is\n100 or greater --- aren't they the same thing?\n\nA bit confused I am...\n"},{"id":"77903","messageId":"483C97E7.2020504@gmail.com","threadId":"13697","inReplyTo":"7viqwzz6p7.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] gitweb: only display \"next\" links in logs if there is a next page","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-05-27T23:23:19Z","receivedAt":"2008-05-27T23:23:19Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Junio C Hamano wrote:\n> This looks like a no-op hunk,\n\nArgh!  Apologies, I'm new to git and accidentally only sent my second \ncommit -- I should've checked more carefully.  I'll re-send the correct \npatch as a follow-up to this email.  Sorry again! :(\n\nBest,\n\n     Lea\n"},{"id":"77904","messageId":"1211930742-24978-1-git-send-email-LeWiemann@gmail.com","threadId":"13697","inReplyTo":"483C97E7.2020504@gmail.com","subject":"[PATCH] gitweb: only display \"next\" links in logs if there is a next page","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-05-27T23:25:42Z","receivedAt":"2008-05-27T23:25:42Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"There was a bug in the implementation of the \"next\" links in\nformat_paging_nav (for log and shortlog), which caused the next links\nto always be displayed, even if there is no next page.  This fixes it.\n\nSigned-off-by: Lea Wiemann <LeWiemann@gmail.com>\n---\n gitweb/gitweb.perl |    8 ++++----\n 1 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 8308e22..57a1905 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -2756,7 +2756,7 @@ sub git_print_page_nav {\n }\n \n sub format_paging_nav {\n-\tmy ($action, $hash, $head, $page, $nrevs) = @_;\n+\tmy ($action, $hash, $head, $page, $has_next_link) = @_;\n \tmy $paging_nav;\n \n \n@@ -2774,7 +2774,7 @@ sub format_paging_nav {\n \t\t$paging_nav .= \" &sdot; prev\";\n \t}\n \n-\tif ($nrevs >= (100 * ($page+1)-1)) {\n+\tif ($has_next_link) {\n \t\t$paging_nav .= \" &sdot; \" .\n \t\t\t$cgi->a({-href => href(-replay=>1, page=>$page+1),\n \t\t\t         -accesskey => \"n\", -title => \"Alt-n\"}, \"next\");\n@@ -4665,7 +4665,7 @@ sub git_log {\n \n \tmy @commitlist = parse_commits($hash, 101, (100 * $page));\n \n-\tmy $paging_nav = format_paging_nav('log', $hash, $head, $page, (100 * ($page+1)));\n+\tmy $paging_nav = format_paging_nav('log', $hash, $head, $page, $#commitlist >= 100);\n \n \tgit_header_html();\n \tgit_print_page_nav('log','', $hash,undef,undef, $paging_nav);\n@@ -5585,7 +5585,7 @@ sub git_shortlog {\n \n \tmy @commitlist = parse_commits($hash, 101, (100 * $page));\n \n-\tmy $paging_nav = format_paging_nav('shortlog', $hash, $head, $page, (100 * ($page+1)));\n+\tmy $paging_nav = format_paging_nav('shortlog', $hash, $head, $page, $#commitlist >= 100);\n \tmy $next_link = '';\n \tif ($#commitlist >= 100) {\n \t\t$next_link =\n-- \n1.5.5.1\n"},{"id":"77905","messageId":"483C99A2.90909@gmail.com","threadId":"13697","inReplyTo":"1211930742-24978-1-git-send-email-LeWiemann@gmail.com","subject":"Re: [PATCH] gitweb: only display \"next\" links in logs if there is a next page","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-05-27T23:30:42Z","receivedAt":"2008-05-27T23:30:42Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Lea Wiemann wrote:\n> There was a bug in the implementation of the \"next\" links in\n> format_paging_nav (for log and shortlog), which caused the next links\n> to always be displayed, even if there is no next page.  This fixes it.\n\nOh, one more thing I forgot to mention: I've tested this with a small \n(single-page) log page and a long log page.  In both cases the \"next\" \nlinks get formatted correctly, and they stop linking to the next page on \nthe correct (= last) page.  The only thing I haven't tested for is \noff-by-one errors, but I'm reasonably sure that $#commitlist >= 100 is \nright.\n\n-- Lea\n"},{"id":"77907","messageId":"7v1w3nz43f.fsf@gitster.siamese.dyndns.org","threadId":"13697","inReplyTo":"483C99A2.90909@gmail.com","subject":"Re: [PATCH] gitweb: only display \"next\" links in logs if there is a next page","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-28T00:04:20Z","receivedAt":"2008-05-28T00:04:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lea Wiemann <lewiemann@gmail.com> writes:\n\n> Lea Wiemann wrote:\n>> There was a bug in the implementation of the \"next\" links in\n>> format_paging_nav (for log and shortlog), which caused the next links\n>> to always be displayed, even if there is no next page.  This fixes it.\n>\n> Oh, one more thing I forgot to mention: I've tested this with a small\n> (single-page) log page and a long log page.  In both cases the \"next\"\n> links get formatted correctly, and they stop linking to the next page\n> on the correct (= last) page.  The only thing I haven't tested for is\n> off-by-one errors, but I'm reasonably sure that $#commitlist >= 100 is\n> right.\n\nI do not trust people who rely on \"tests\" to catch counting errors\n(although I am pretty bad at counting myself and often testsuite helps me\ncatch my own bugs).  Instead, let's follow the code to see if it is Ok.\n\nYou call format_paging_nav() from two places, both after calling\nparse_commits() with $maxcount = 101.  So @commitlist could have 101\nelements (in which case you would need to have next page because you will\nshow only 100 among them), or smaller than that (in which case you will\nfully show them).  In other words, you would want to say 'next' iff\n\n\t(@commitlist > 100)\n\nis true.  On the other hand, $#commitlist is the last index of @commitlist\narray, which is one smaller than the number of elements in that array.\nIOW, when it has 101 elements, it has $commitlist[0] thru $commitlist[100]\nand $#commitlist == 100.  So the above condition is the same as\n\n\t($#commitlist >= 100)\n\nSo your counting looks correct to me.\n\nI wonder if it is easier to read to use the more modern @array notation\nthan historic $#array notation (it looks very Perl 4 to me), but that is a\nseparate issue.  gitweb is littered with such anachronism ;-)\n"},{"id":"77908","messageId":"483CA307.8040900@gmail.com","threadId":"13697","inReplyTo":"7v1w3nz43f.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] gitweb: only display \"next\" links in logs if there is a next page","fromName":"Lea Wiemann","fromEmail":"lewiemann@gmail.com","sentAt":"2008-05-28T00:10:47Z","receivedAt":"2008-05-28T00:10:47Z","isPatch":true,"sender":{"key":"lewiemann@gmail.com","avatar":null},"body":"Junio C Hamano wrote:\n> I do not trust people who rely on \"tests\" to catch counting errors\n\nOh, I wasn't trying to say that I didn't \"brain-check\" my code for \ncorrectness -- I just think it's a good idea to actually test it in \naddition to that. :)\n\nAnyways, thanks for reviewing and confirming it!\n\n> I wonder if it is easier to read to use the more modern @array notation\n> than historic $#array notation (it looks very Perl 4 to me), but that is a\n> separate issue.  gitweb is littered with such anachronism ;-)\n\nYup, I was trying to be consistent there.\n\n-- Lea\n"},{"id":"77922","messageId":"m33ao23iqv.fsf@localhost.localdomain","threadId":"13697","inReplyTo":"1211930742-24978-1-git-send-email-LeWiemann@gmail.com","subject":"Re: [PATCH] gitweb: only display \"next\" links in logs if there is a next page","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-05-28T09:01:53Z","receivedAt":"2008-05-28T09:01:53Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Lea Wiemann <lewiemann@gmail.com> writes:\n\n> There was a bug in the implementation of the \"next\" links in\n> format_paging_nav (for log and shortlog), which caused the next links\n> to always be displayed, even if there is no next page.  This fixes it.\n\nThanks for correcting this.\n\n>  sub format_paging_nav {\n> -\tmy ($action, $hash, $head, $page, $nrevs) = @_;\n> +\tmy ($action, $hash, $head, $page, $has_next_link) = @_;\n>  \tmy $paging_nav;\n>  \n>  \n> @@ -2774,7 +2774,7 @@ sub format_paging_nav {\n>  \t\t$paging_nav .= \" &sdot; prev\";\n>  \t}\n>  \n> -\tif ($nrevs >= (100 * ($page+1)-1)) {\n> +\tif ($has_next_link) {\n>  \t\t$paging_nav .= \" &sdot; \" .\n>  \t\t\t$cgi->a({-href => href(-replay=>1, page=>$page+1),\n>  \t\t\t         -accesskey => \"n\", -title => \"Alt-n\"}, \"next\");\n\nThis makes logic much simpler.  Nice change.\n\n> @@ -4665,7 +4665,7 @@ sub git_log {\n>  \n>  \tmy @commitlist = parse_commits($hash, 101, (100 * $page));\n\nHere I have realized the source of this bug.  Some time ago\ngit-rev-list acquired '--skip=<number>' option to have _git_ skip\ncommits and not _gitweb_, which improves performance a bit.  It was\nrequired to implement huge performance improvement, namely getting\ndetails for all commits from a single command, otherwise the\nperformance improvement of calling one git command instead of\n$page_size git commands would be much reduced by generating large\namount of data which would be skipped (wound't be used by gitweb).\n\nUnfortunately this change wasn't reviewed carefully enough; old logic\nto decide whether to add 'next' link compared (tried to compare)\nnumber of commits receivied with number of commits requested (via\n'--max-count=<number>' option).  I guess that having format_paging_nav\ndecide whether to add \"next\" link was a bad idea...\n\n> -\tmy $paging_nav = format_paging_nav('log', $hash, $head, $page, (100 * ($page+1)));\n> +\tmy $paging_nav = format_paging_nav('log', $hash, $head, $page, $#commitlist >= 100);\n> \n\nI would agree with Junio here that @commitlist > 100 would be more\nreadable.\n\nLogic goes as the following: we request ($page_size+1) revisions to\nknow if there are additional revisions, skipping ($page_size * $page)\nrevisions; gitweb adds 'next' link if it got more than $page_size\nrevisions.\n \n>  \tgit_header_html();\n>  \tgit_print_page_nav('log','', $hash,undef,undef, $paging_nav);\n> @@ -5585,7 +5585,7 @@ sub git_shortlog {\n>  \n>  \tmy @commitlist = parse_commits($hash, 101, (100 * $page));\n>  \n> -\tmy $paging_nav = format_paging_nav('shortlog', $hash, $head, $page, (100 * ($page+1)));\n> +\tmy $paging_nav = format_paging_nav('shortlog', $hash, $head, $page, $#commitlist >= 100);\n>  \tmy $next_link = '';\n>  \tif ($#commitlist >= 100) {\n>  \t\t$next_link =\n\nWhat about git_history()... oh, I see, it generates paging itself, and\nsoes not use format_paging_nav() subroutine.  But I think it does not\nexhibit mentioned (and corrected) error.  BTW. I *guess* that with\nhref(-replay=>1, ...) gitweb could use format_paging_nav() also for\nother pages...\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"}]}