threads / patch / 43394

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

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

## tl;dr

11 messages between Dec 18, 2006 and Dec 19, 2006. Diffs are folded; open one to read it.

replies: 10people: 3as markdown or json

Robert Fitzsimons· Dec 18, 2006, 22:43 UTC · lore

[PATCH] Small optimizations to gitweb

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(-)

Show changes to gitweb/gitweb.perl +6 −3

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· Dec 18, 2006, 23:17 UTC · re: Robert Fitzsimons · lore

Re: [PATCH] Small optimizations to gitweb

Robert Fitzsimons wrote:

> Limit some of the git_cmd's so they only return the number of lines
> that will be processed.

[...]

Show 11 quoted lines

> @@ -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· Dec 18, 2006, 23:45 UTC · re: Jakub Narebski · lore

Re: [PATCH] Small optimizations to gitweb

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· Dec 19, 2006, 00:59 UTC · re: Junio C Hamano · lore

Re: [PATCH] Small optimizations to gitweb

Junio C Hamano wrote:

Show 34 quoted lines

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

Show 10 quoted lines

>             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· Dec 19, 2006, 05:48 UTC · re: Jakub Narebski · lore

Re: [PATCH] Small optimizations to gitweb

Jakub Narebski <jnareb@gmail.com> writes:

Show 6 quoted lines

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

Show 6 quoted lines

>> 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· Dec 19, 2006, 11:14 UTC · re: Jakub Narebski · lore

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

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:

Show 6 quoted lines

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

[...]

Show 6 quoted lines

>> 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(-)

Show changes to gitweb/gitweb.perl +7 −2

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· Dec 19, 2006, 12:08 UTC · re: Jakub Narebski · lore

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

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.

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(-)

Show changes to gitweb/gitweb.perl +9 −3

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· Dec 19, 2006, 12:28 UTC · re: Robert Fitzsimons · lore

Robert Fitzsimons wrote:

Show 11 quoted lines

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

[...]

Show 10 quoted lines

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

Show 11 quoted lines

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

[...]

Show 5 quoted lines

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

Show 10 quoted lines

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

Show 7 quoted lines

>  	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· Dec 19, 2006, 12:41 UTC · re: Jakub Narebski · lore

Show 8 quoted lines

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

Show 9 quoted lines

> > -	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· Dec 19, 2006, 12:42 UTC · re: Jakub Narebski · lore

Show 14 quoted lines

>> @@ -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· Dec 19, 2006, 18:05 UTC · re: Jakub Narebski · lore

Jakub Narebski <jnareb@gmail.com> writes:

Show 19 quoted lines

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