From: Jakub Narebski Date: Thu, 04 Nov 2010 10:41:57 GMT Subject: Re: [PATCHv6 10/10] gitweb: group remote heads by remote Message-ID: <201011041141.58334.jnareb@gmail.com> In-Reply-To: On Wed, 3 Nov 2010, Giuseppe Bilotta wrote: > On Wed, Nov 3, 2010 at 12:58 AM, Jakub Narebski wrote: >> On Tue, 2 Nov 2010, Giuseppe Bilotta wrote: >>> The limiting seems to be the most debatable part of this patch 8-) >> >> I think that untill this feature gets into gitweb we can only guess >> as to what kind of limiting would look and behave best on RL cases. > > I think that in most cases there won't be any need for limiting. > Public cases of lots of remotes with lots of branches are, I suspect, > rare. It's not about _public_ cases; I think that it is in very rare cases that public repository would want to display remotes and remote-tracking branches. 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)? >>>> +sub fill_remote_heads { >>>> +       my $remotes = shift; >>>> + >>>> +       my @heads = map { "remotes/$_" } keys %$remotes; >>>> +       ## A QUESTION: perhaps it would be better to pass @remoteheads as parameter? >>>> +       my @remoteheads = git_get_heads_list(undef, @heads); >>>> +       foreach my $remote (keys %$remotes) { >>>> +               $remotes->{$remote}{'heads'} = >>>> +                       [ grep { $_->{'name'} =~ s!^$remote/!! } @remoteheads ]; >>>> +       } >>>> +} [...] >>>> A QUESTION: perhaps it would be better to pass @remoteheads as parameter? >>>> I wonder if won't end up with calling git_get_heads_list multiple times... >>>> well, the improvement can be left for later.  Answering this question >>>> should not affect accepting this patch series. >>> >>> I'm not sure I actually understand the question. Who should we pass >>> @remoteheads to? >> >> I meant here passing @remoteheads or \@remoteheads, i.e. result of call >> to git_get_heads_list, to fill_remote_heads (as one of parameters). >> I was worrying, perhaps unnecessarily, about calling git_get_heads_list >> more than once... but I guess that we did it anyway. > > I'm still not sure I follow. git_get_heads_list only gets called once, Ah, that's all right then. I was worring that it is called more than once in one request by gitweb. > with all the remotes/ 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] ] + } + } +} >>>> Though I am not sure if it is good public API.  Perhaps it is... >>> >>> The alternative would be to have git_remote to handle the single >>> remote case, and possibly even have the action name be 'remote' rather >>> than 'remotes' in that case. >> >> We might want to have 'remote' as alias to 'remotes' anyway. >> >> Though what you propose, having separately 'remotes' for list of all >> remotes, and 'remote' + name of remote, might be a good idea. > > I hope you don't mind if I opt to leave these refinements for later ;-) Not a problem. For backward compatibility we would have to accept 'remotes/' in addition to more correct 'remote/', but this is not a problem. -- Jakub Narebski Poland