Re: [PATCHv6 10/10] gitweb: group remote heads by remote
On Mon, 8 Nov 2010, Giuseppe Bilotta wrote:
> On Thu, Nov 4, 2010 at 11:41 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>> On Wed, 3 Nov 2010, Giuseppe Bilotta wrote:
Show 9 quoted lines
>> I think remote_heads feature is more important for _local_ use, for
>> example browsing one own repository using git-instaweb. In such cases
>> number of remotes and of remote-tracking branches might be large (I have
>> 11 remotes, not all active, and 58 remote-tracking branches).
>>
>> BTW. would next version of this series include patch to git-instaweb
>> enabling 'remote_heads' feature for it (gitweb_conf function)?
>
> I will look into that.
diff --git i/git-instaweb.sh w/git-instaweb.sh
index e6f6ecd..50f65b1 100755
--- i/git-instaweb.sh
+++ w/git-instaweb.sh
@@ -580,6 +580,8 @@ gitweb_conf() {
our \$projectroot = "$(dirname "$fqgitdir")";
our \$git_temp = "$fqgitdir/gitweb/tmp";
our \$projects_list = \$projectroot;
+
+$feature{'remote_heads'}{'default'} = [1]
EOF
}
We might want to enable more features for git-instaweb, but I think
it would out of scope for planned commit (for 'remote heads' series).
>>> with all the remotes/<remotename> pathspecs as a single array
>>> argument. This _does_ mean that when the total number of remote heads
>>> is greater than the limit some remotes will not display complete
>>> information in summary view. The real issue here is, I think, that
>>> there is no trivial way to tell which remotes have incomplete
>>> information and which don't, meaning that in the subsequent
>>> git_remote_block calls we'll have no way to provide visual feedback
>>> (the ellipsis) when some heads are missing.
>>
>> Errr... shouldn't we leave limiting number of heads to fill_remote_heads,
>> which can do limiting per remote (with each remote having up to $limit
>> remote-tracking branches / remote heads), instead of having
>> git_get_heads_list do it?
>>
>> Something like this:
>>
>> +sub fill_remote_heads {
>> + my ($remotes, $limit) = @_;
>> +
>> + my @heads = map { "remotes/$_" } keys %$remotes;
>> + my @remoteheads = git_get_heads_list(undef, @heads);
>> + foreach my $remote (keys %$remotes) {
>> + $remotes->{$remote}{'heads'} =
>> + [ grep { $_->{'name'} =~ s!^$remote/!! } @remoteheads ];
>> + $remotes->{$remote}{'heads'} =
>> + [ @{$remotes->{$remote}{'heads'}}[0..$limit-1] ]
>> + if (@{$remotes->{$remote}{'heads'}}> $limit);
>> + }
>> +}
>>
>> Though perhaps it will be more clear with if as statement, not as modifier:
>>
>> +sub fill_remote_heads {
>> + my ($remotes, $limit) = @_;
>> +
>> + my @heads = map { "remotes/$_" } keys %$remotes;
>> + my @remoteheads = git_get_heads_list(undef, @heads);
>> + foreach my $remote (keys %$remotes) {
>> + $remotes->{$remote}{'heads'} =
>> + [ grep { $_->{'name'} =~ s!^$remote/!! } @remoteheads ];
>> + if (@{$remotes->{$remote}{'heads'}}> $limit) {
>> + $remotes->{$remote}{'heads'} =
>> + [ @{$remotes->{$remote}{'heads'}}[0..$limit-1] ]
>> + }
>> + }
>> +}
>
> Either solution is fine, but it would require grabbing all the remote
> heads. The real issue here is, I think understanding what is the
> purpose of limiting in gitweb. Is it to reduce runtime? is it to
> reduce clutter on the screen? In the first case, the limiting should
> be done as early as possible (i.e. during the git call that retrieves
> the data); in the latter case, is it _really_ needed at all?
It is to reduce clutter on the screen, or rather have 'summary' view
for a project (for a repository) to be really a _summary_. That's why
there is limit of 15 commits in shortlog, of 15 branches in heads, of
15 tags. This action is meant to present balanced overview of
repository.
Regarding gitweb performance, it is quite important to pass limit to
git-log / git-rev-list needed also for 'summary' view; passing limit
to git command really matters here.
git_get_heads_list passes '--count='.($limit+1) to git-for-each-ref,
but I don't think that it improves performance in any measurable way.
Similar with saving a memory: it is negligible amount. So if we can
do better at the cost of running git_get_heads_list without a limit,
I say go for it.
Note that the costly part of git_get_heads_list is forking git command,
so it makes absolutely no sense to run git_get_heads_list once per
remote instead of doing limiting per-remote in Perl. The former would
affect performance badly, I can guess.
--
Jakub Narebski
Poland
Jakub Narebski <jnareb@gmail.com>