# Re: [PATCH] gitweb: Show '...' links in "summary" view only if there are more items

11 messages from 2006-12-18 to 2006-12-19. Participants: Jakub Narebski, Robert Fitzsimons, Junio C Hamano.
Thread: https://gitlist.dev/t/43394

## Robert Fitzsimons, 2006-12-18 22:43

Subject: [PATCH] Small optimizations to gitweb
Message-ID: <20061218224327.GG16029@localhost>
URL: https://gitlist.dev/e/20061218224327.GG16029%40localhost

```
Limit some of the git_cmd's so they only return the number of lines
that will be processed.  Don't recompute head hash or have_snapshot
values.

Signed-off-by: Robert Fitzsimons <robfitz@273k.net>
---
 gitweb/gitweb.perl |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 5ea3fda..1990f15 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1141,6 +1141,7 @@ sub git_get_last_activity {
 	open($fd, "-|", git_cmd(), 'for-each-ref',
 	     '--format=%(refname) %(committer)',
 	     '--sort=-committerdate',
+	     '--count=1',
 	     'refs/heads') or return;
 	my $most_recent = <$fd>;
 	close $fd or return;
@@ -2559,6 +2560,8 @@ sub git_shortlog_body {
 	# uses global variable $project
 	my ($revlist, $from, $to, $refs, $extra) = @_;
 
+	my $have_snapshot = gitweb_have_snapshot();
+
 	$from = 0 unless defined $from;
 	$to = $#{$revlist} if (!defined $to || $#{$revlist} < $to);
 
@@ -2586,7 +2589,7 @@ sub git_shortlog_body {
 		      $cgi->a({-href => href(action=>"commit", hash=>$commit)}, "commit") . " | " .
 		      $cgi->a({-href => href(action=>"commitdiff", hash=>$commit)}, "commitdiff") . " | " .
 		      $cgi->a({-href => href(action=>"tree", hash=>$commit, hash_base=>$commit)}, "tree");
-		if (gitweb_have_snapshot()) {
+		if ($have_snapshot) {
 			print " | " . $cgi->a({-href => href(action=>"snapshot", hash=>$commit)}, "snapshot");
 		}
 		print "</td>\n" .
@@ -2876,8 +2879,8 @@ sub git_summary {
 		}
 	}
 
-	open my $fd, "-|", git_cmd(), "rev-list", "--max-count=17",
-		git_get_head_hash($project), "--"
+	open my $fd, "-|", git_cmd(), "rev-list", "--max-count=16",
+		$head, "--"
 		or die_error(undef, "Open git-rev-list failed");
 	my @revlist = map { chomp; $_ } <$fd>;
 	close $fd;
-- 
1.4.4.2.gee60-dirty

```

## Jakub Narebski, 2006-12-18 23:17

Subject: Re: [PATCH] Small optimizations to gitweb
Message-ID: <em77cg$obn$1@sea.gmane.org>
URL: https://gitlist.dev/e/em77cg%24obn%241%40sea.gmane.org
In-Reply-To: <20061218224327.GG16029@localhost>

```
Robert Fitzsimons wrote:

> Limit some of the git_cmd's so they only return the number of lines
> that will be processed. 
[...]
> @@ -2876,8 +2879,8 @@ sub git_summary {
>                 }
>         }
>  
> -       open my $fd, "-|", git_cmd(), "rev-list", "--max-count=17",
> -               git_get_head_hash($project), "--"
> +       open my $fd, "-|", git_cmd(), "rev-list", "--max-count=16",
> +               $head, "--"
>                 or die_error(undef, "Open git-rev-list failed");
>         my @revlist = map { chomp; $_ } <$fd>;
>         close $fd;

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 same for heads and tags.
(On my short TODO list, but feel free to do it yourself, if you want).

So ack without the last chunk.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git


```

## Junio C Hamano, 2006-12-18 23:45

Subject: Re: [PATCH] Small optimizations to gitweb
Message-ID: <7vbqm0vkd6.fsf@assigned-by-dhcp.cox.net>
URL: https://gitlist.dev/e/7vbqm0vkd6.fsf%40assigned-by-dhcp.cox.net
In-Reply-To: <em77cg$obn$1@sea.gmane.org>

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



```

## Jakub Narebski, 2006-12-19 00:59

Subject: Re: [PATCH] Small optimizations to gitweb
Message-ID: <200612190159.24173.jnareb@gmail.com>
URL: https://gitlist.dev/e/200612190159.24173.jnareb%40gmail.com
In-Reply-To: <7vbqm0vkd6.fsf@assigned-by-dhcp.cox.net>

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

```

## Junio C Hamano, 2006-12-19 05:48

Subject: Re: [PATCH] Small optimizations to gitweb
Message-ID: <7vtzzstozi.fsf@assigned-by-dhcp.cox.net>
URL: https://gitlist.dev/e/7vtzzstozi.fsf%40assigned-by-dhcp.cox.net
In-Reply-To: <200612190159.24173.jnareb@gmail.com>

```
Jakub Narebski <jnareb@gmail.com> writes:

> Junio C Hamano wrote:
>> ...
>> The counting code in git_*_body is seriously unusual to tempt
>> anybody who reviews the code to reduce that 17 to 16.
>
> Well, this should be then corrected perhaps to

Well, I did not say it was _wrong_, just unusual, so there is
nothing to fix.

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

Check?

```

## Jakub Narebski, 2006-12-19 11:14

Subject: [PATCH] gitweb: Show '...' links in "summary" view only if there are more items
Message-ID: <200612191214.58474.jnareb@gmail.com>
URL: https://gitlist.dev/e/200612191214.58474.jnareb%40gmail.com
In-Reply-To: <200612190159.24173.jnareb@gmail.com>

```
Show "..." links in "summary" view to shortlog, heads (if there are
any), and tags (if there are any) only if there are more items to show
than shown already.

This means that "..." link is shown below shortened shortlog if there
are more than 16 commits, "..." link below shortened heads list if
there are more than 16 heads refs (16 branches), "..." link below
shortened tags list if there are more than 16 tags.

Added some comments.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---

Jakub Narebski wrote:
> 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.
[...]
>> 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.

And now it is done.

Slightly tested: on my clone (copy) of git repository, which more than
16 commits, more than 16 heads (most temporary, and no longer worked on,
few tracking branches) and more than 16 heads show all "..." as it should.
Test of freshly created repository shown no "..." for commits (only one
commit), no "..." for heads (only one default head 'master'), and no
tags list (no tags at all).

By the way, I have _NOT_ applied Robert Fitzsimons patch, but they
(this patch and Robert patch) should be not in conflict if we remove
last chunk of Robert's patch (this changing --count=17 to --count=15
in git_summary).

 gitweb/gitweb.perl |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 4059894..73877f2 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2915,8 +2915,9 @@ sub git_summary {
 	my $owner = git_get_project_owner($project);
 
 	my $refs = git_get_references();
-	my @taglist  = git_get_tags_list(15);
-	my @headlist = git_get_heads_list(15);
+	# we need to request one more than 16 (0..15) to check if those 16 are all
+	my @taglist  = git_get_tags_list(17);
+	my @headlist = git_get_heads_list(17);
 	my @forklist;
 	my ($check_forks) = gitweb_check_feature('forks');
 
@@ -2952,6 +2953,7 @@ sub git_summary {
 		}
 	}
 
+	# we need to request one more than 16 (0..15) to check if those 16 are all
 	open my $fd, "-|", git_cmd(), "rev-list", "--max-count=17",
 		git_get_head_hash($project), "--"
 		or die_error(undef, "Open git-rev-list failed");
@@ -2959,17 +2961,20 @@ sub git_summary {
 	close $fd;
 	git_print_header_div('shortlog');
 	git_shortlog_body(\@revlist, 0, 15, $refs,
+	                  $#revlist <=  15 ? undef :
 	                  $cgi->a({-href => href(action=>"shortlog")}, "..."));
 
 	if (@taglist) {
 		git_print_header_div('tags');
 		git_tags_body(\@taglist, 0, 15,
+		              $#taglist <=  15 ? undef :
 		              $cgi->a({-href => href(action=>"tags")}, "..."));
 	}
 
 	if (@headlist) {
 		git_print_header_div('heads');
 		git_heads_body(\@headlist, $head, 0, 15,
+		               $#headlist <= 15 ? undef :
 		               $cgi->a({-href => href(action=>"heads")}, "..."));
 	}
 
-- 

```

## Robert Fitzsimons, 2006-12-19 12:08

Subject: [PATCH] gitweb: Show '...' links in "summary" view only if there are more items
Message-ID: <20061219120854.GA16429@localhost>
URL: https://gitlist.dev/e/20061219120854.GA16429%40localhost
In-Reply-To: <200612191214.58474.jnareb@gmail.com>

```
Show "..." links in "summary" view to shortlog, heads (if there are
any), and tags (if there are any) only if there are more items to show
than shown already.

This means that "..." link is shown below shortened shortlog if there
are more than 16 commits, "..." link below shortened heads list if
there are more than 16 heads refs (16 branches), "..." link below
shortened tags list if there are more than 16 tags.

Modified patch from Jakub to to apply cleanly to master, also preform
the same "..." link logic to the forks list.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
Signed-off-by: Robert Fitzsimons <robfitz@273k.net>
---

> By the way, I have _NOT_ applied Robert Fitzsimons patch, but they
> (this patch and Robert patch) should be not in conflict if we remove
> last chunk of Robert's patch (this changing --count=17 to --count=15
> in git_summary).

Just removing the last chunk isn't correct, there are two slightly
different changes in that chuck.  The reduction in the max-count value
and a removal of a call to git_get_head_hash.

Here is an updated version of the patch which should apply against
master.

Robert



 gitweb/gitweb.perl |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 3bee34c..8d409c7 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2918,8 +2918,9 @@ sub git_summary {
 	my $owner = git_get_project_owner($project);
 
 	my $refs = git_get_references();
-	my @taglist  = git_get_tags_list(15);
-	my @headlist = git_get_heads_list(15);
+	# we need to request one more than 16 (0..15) to check if those 16 are all
+	my @taglist  = git_get_tags_list(16);
+	my @headlist = git_get_heads_list(16);
 	my @forklist;
 	my ($check_forks) = gitweb_check_feature('forks');
 
@@ -2955,30 +2956,35 @@ sub git_summary {
 		}
 	}
 
-	open my $fd, "-|", git_cmd(), "rev-list", "--max-count=16",
+	# we need to request one more than 16 (0..15) to check if those 16 are all
+	open my $fd, "-|", git_cmd(), "rev-list", "--max-count=17",
 		$head, "--"
 		or die_error(undef, "Open git-rev-list failed");
 	my @revlist = map { chomp; $_ } <$fd>;
 	close $fd;
 	git_print_header_div('shortlog');
 	git_shortlog_body(\@revlist, 0, 15, $refs,
+	                  $#revlist <=  15 ? undef :
 	                  $cgi->a({-href => href(action=>"shortlog")}, "..."));
 
 	if (@taglist) {
 		git_print_header_div('tags');
 		git_tags_body(\@taglist, 0, 15,
+		              $#taglist <=  15 ? undef :
 		              $cgi->a({-href => href(action=>"tags")}, "..."));
 	}
 
 	if (@headlist) {
 		git_print_header_div('heads');
 		git_heads_body(\@headlist, $head, 0, 15,
+		               $#headlist <= 15 ? undef :
 		               $cgi->a({-href => href(action=>"heads")}, "..."));
 	}
 
 	if (@forklist) {
 		git_print_header_div('forks');
 		git_project_list_body(\@forklist, undef, 0, 15,
+		                      $#forklist <= 15 ? undef :
 		                      $cgi->a({-href => href(action=>"forks")}, "..."),
 				      'noheader');
 	}
-- 

```

## Jakub Narebski, 2006-12-19 12:28

Subject: Re: [PATCH] gitweb: Show '...' links in "summary" view only if there are more items
Message-ID: <200612191328.08928.jnareb@gmail.com>
URL: https://gitlist.dev/e/200612191328.08928.jnareb%40gmail.com
In-Reply-To: <20061219120854.GA16429@localhost>

```
Robert Fitzsimons wrote:
> Show "..." links in "summary" view to shortlog, heads (if there are
> any), and tags (if there are any) only if there are more items to show
> than shown already.
> 
> This means that "..." link is shown below shortened shortlog if there
> are more than 16 commits, "..." link below shortened heads list if
> there are more than 16 heads refs (16 branches), "..." link below
> shortened tags list if there are more than 16 tags.
> 
> Modified patch from Jakub to to apply cleanly to master, also preform
> the same "..." link logic to the forks list.

Junio usually puts such comments in brackets (I don't know if it is
always used, i.e. if it is some 'convention'), e.g.:

  Also perform the same "..." link logic to the forks list.

  [rf: Modified patch from Jakub to to apply cleanly to master]

or something like that. Just a nitpick.


By the way, it looks like git_get_projects_list($project) used to get
list of forks does not have any count limit option.

[...]
> ---
>
> > By the way, I have _NOT_ applied Robert Fitzsimons patch, but they
> > (this patch and Robert patch) should be not in conflict if we
> > remove last chunk of Robert's patch (this changing --count=17 to
> > --count=15 in git_summary).
>
> Just removing the last chunk isn't correct, there are two slightly
> different changes in that chuck.  The reduction in the max-count
> value and a removal of a call to git_get_head_hash.

The last chunk I meant to be removed was:

> @@ -2876,8 +2879,8 @@ sub git_summary {
>                 }
>         }
>  
> -       open my $fd, "-|", git_cmd(), "rev-list", "--max-count=17",
> -               git_get_head_hash($project), "--"
> +       open my $fd, "-|", git_cmd(), "rev-list", "--max-count=16",
> +               $head, "--"
>                 or die_error(undef, "Open git-rev-list failed");
>         my @revlist = map { chomp; $_ } <$fd>;
>         close $fd;

and if we remove that chunk, then your earlier patch would not
touch git_summary at all, so mine would cleanly apply (I think).

[...]
> -	my @taglist  = git_get_tags_list(15);
> -	my @headlist = git_get_heads_list(15);
> +	# we need to request one more than 16 (0..15) to check if those 16 are all
> +	my @taglist  = git_get_tags_list(16);
> +	my @headlist = git_get_heads_list(16);

It needs to be 17, not 16, otherwise we never would get "...". By default
we show _16_ items, from 0 to 15 inclusive, so we must get _17_ items
to check if there are more than 16.

>  	my @forklist;
>  	my ($check_forks) = gitweb_check_feature('forks');
>  
> @@ -2955,30 +2956,35 @@ sub git_summary {
>  		}
>  	}
>  
> -	open my $fd, "-|", git_cmd(), "rev-list", "--max-count=16",
> +	# we need to request one more than 16 (0..15) to check if those 16 are all
> +	open my $fd, "-|", git_cmd(), "rev-list", "--max-count=17",

Here you have 17.


>  	if (@forklist) {
>  		git_print_header_div('forks');
>  		git_project_list_body(\@forklist, undef, 0, 15,
> +		                      $#forklist <= 15 ? undef :
>  		                      $cgi->a({-href => href(action=>"forks")}, "..."),
>  				      'noheader');
>  	}

Nice catch. I forgot about this one.

-- 
Jakub Narebski

```

## Robert Fitzsimons, 2006-12-19 12:41

Subject: Re: [PATCH] gitweb: Show '...' links in "summary" view only if there are more items
Message-ID: <20061219124133.GB16429@localhost>
URL: https://gitlist.dev/e/20061219124133.GB16429%40localhost
In-Reply-To: <200612191328.08928.jnareb@gmail.com>

```
> Junio usually puts such comments in brackets (I don't know if it is
> always used, i.e. if it is some 'convention'), e.g.:
> 
>   Also perform the same "..." link logic to the forks list.
> 
>   [rf: Modified patch from Jakub to to apply cleanly to master]
> 
> or something like that. Just a nitpick.

No problem, I tried to find the approreati convention.

> > -	my @taglist  = git_get_tags_list(15);
> > -	my @headlist = git_get_heads_list(15);
> > +	# we need to request one more than 16 (0..15) to check if those 16 are all
> > +	my @taglist  = git_get_tags_list(16);
> > +	my @headlist = git_get_heads_list(16);
> 
> It needs to be 17, not 16, otherwise we never would get "...". By default
> we show _16_ items, from 0 to 15 inclusive, so we must get _17_ items
> to check if there are more than 16.

That was a copy error on my part.  Though looking at the code
git_get_tags_list and git_get_heads_list already adds one to the limit
value, so if you pass in 17 they will return 18 items.

Robert

```

## Jakub Narebski, 2006-12-19 12:42

Subject: Re: [PATCH] gitweb: Show '...' links in "summary" view only if there are more items
Message-ID: <200612191342.46220.jnareb@gmail.com>
URL: https://gitlist.dev/e/200612191342.46220.jnareb%40gmail.com
In-Reply-To: <200612191328.08928.jnareb@gmail.com>

```

>> @@ -2876,8 +2879,8 @@ sub git_summary {
>>                 }
>>         }
>>  
>> -       open my $fd, "-|", git_cmd(), "rev-list", "--max-count=17",
>> -               git_get_head_hash($project), "--"
>> +       open my $fd, "-|", git_cmd(), "rev-list", "--max-count=16",
>> +               $head, "--"
>>                 or die_error(undef, "Open git-rev-list failed");
>>         my @revlist = map { chomp; $_ } <$fd>;
>>         close $fd;
> 
> and if we remove that chunk, then your earlier patch would not
> touch git_summary at all, so mine would cleanly apply (I think).

Sorry, I haven't noticed git_get_head_hash($project) -> $head
Sorry for the noise.
-- 
Jakub Narebski

```

## Junio C Hamano, 2006-12-19 18:05

Subject: Re: [PATCH] gitweb: Show '...' links in "summary" view only if there are more items
Message-ID: <7vr6uvpxqc.fsf@assigned-by-dhcp.cox.net>
URL: https://gitlist.dev/e/7vr6uvpxqc.fsf%40assigned-by-dhcp.cox.net
In-Reply-To: <200612191328.08928.jnareb@gmail.com>

```
Jakub Narebski <jnareb@gmail.com> writes:

> Robert Fitzsimons wrote:
>> Show "..." links in "summary" view to shortlog, heads (if there are
>> any), and tags (if there are any) only if there are more items to show
>> than shown already.
>> 
>> This means that "..." link is shown below shortened shortlog if there
>> are more than 16 commits, "..." link below shortened heads list if
>> there are more than 16 heads refs (16 branches), "..." link below
>> shortened tags list if there are more than 16 tags.
>> 
>> Modified patch from Jakub to to apply cleanly to master, also preform
>> the same "..." link logic to the forks list.
>
> Junio usually puts such comments in brackets (I don't know if it is
> always used, i.e. if it is some 'convention'), e.g.:
>
>   Also perform the same "..." link logic to the forks list.
>
>   [rf: Modified patch from Jakub to to apply cleanly to master]

I would actually discourage this on messages that are still on
the list (i.e. not in commits).  I do [jc:] comment when I take
the proposed commit log message from the incoming e-mail
verbatim but I made modification to the patch text, because
otherwise the original submitter cannot tell from the log
message if that is meant to be identical to what was submitted.

I've only took a cursory look of the actual patch text, but what
Robert sent looked good to me.  Thanks, both.

```