Re: [PATCH] Small optimizations to gitweb

[Jakub Narebski <jnareb@gmail.com>]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> Actually, that is needed to implement checking if we have more than
>> the number of commits to show to add '...' at the end only if there
>> are some commits which we don't show.
> 
> The counting code in git_*_body is seriously unusual to tempt
> anybody who reviews the code to reduce that 17 to 16.
> 
> The caller says:
> 
> 	git_shortlog_body(\@revlist, 0, 15, $refs,
> 	                  $cgi->a({-href => href(action=>"shortlog")}, "..."));
> 
> If it counts up, especially if it counts from zero, the loop
> would usually say:
> 
> 	for (i = bottom; i < end; i++)
> 
> and anybody who reads that caller would expect it to show 15
> lines of output.
> 
> But the actual code does this instead:
> 
>     sub git_shortlog_body {
>             # uses global variable $project
>             my ($revlist, $from, $to, $refs, $extra) = @_;
> 
>             $from = 0 unless defined $from;
>             $to = $#{$revlist} if (!defined $to || $#{$revlist} < $to);
>             ...
>             for (my $i = $from; $i <= $to; $i++) {
>                     ... draw each item ...
>             }

Well, this should be then corrected perhaps to

            my ($revlist, $begin, $end, $refs, $extra) = @_;

            $begin = 0 unless defined $from;
            $end = scalar(@$revlist) if (!defined $end || @$revlist <= $end);
            ...
            for (my $i = $begin; $i < $end; $i++) {
                    ... draw each item ...
            }

I thought that $from..$to ($from <= i <= $to) is more natural
and easier to understand than $begin..$end ($begin <= i < $end)...
guess I guessed wrong.

>             if (defined $extra) {
>                     print "<tr>\n" .
>                           "<td colspan=\"4\">$extra</td>\n" .
>                           "</tr>\n";
>             }
>     }
> 
> By the way, I wonder how that $extra is omitted when $revlist is
> longer than $to; it should be a trivial fix but it seems to me
> that it is always spitted out with the current code.

We should check if we want to omit $extra, either in caller or
in callee, the *_body subroutine itself.

-- 
Jakub Narebski