Re: [PATCH 7/7] gitweb: group remote heads
- From
Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
- Date
- Sep 19, 2010, 05:39 UTC
- Message-ID
- <AANLkTinkikwt5cUxuXECfeQrKZthu271U82F3ebSrEmd@mail.gmail.com>
- In-Reply-To
- <201009171854.03476.jnareb@gmail.com>
Hello,
as I mentioned, this patch was the one I had most doubts about. I will therefore skip over the stylistic suggestions (which I _am_ following for the next release of this patchset) and only reply to the more technical remarks.
On Fri, Sep 17, 2010 at 6:54 PM, Jakub Narebski <jnareb@gmail.com> wrote:
Show 11 quoted lines
> On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:
>
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index 92551e4..66b5400 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -2758,6 +2758,16 @@ sub git_get_last_activity {
>> return (undef, undef);
>> }
>>
>> +sub git_get_remotes {[snip]
Show 14 quoted lines
>> + my @remoteheads = git_get_heads_list($limit, 'remotes'); >> + return (\@remotes, \@remoteheads); > > Why do you want for git_get_remotes() to also return remote-tracking > branches (refs/remotes/)? Those are separate issues, and I think it > would be better API for git_get_remotes() to provide only list of > remotes, i.e. > > + return @remotes; > > Especially that we might want in the summary view to only list remotes, > without listing remote-tracking branches. > > That would require more changes to the code.
This is kind of the main issue with this patch. What do we want to do with the remotes list in summary view and the remotes view? We basically have three possibilities:
(1) we can make the remotes list in summary view be a 'reduced remotes' view: this would make it behave the most similarly to the other components of summary view (2) we can make the remotes list be much more stripped down, by only listing the remotes and possibly some summarizing property such as the number of branches in it or when it was last updated (3) we can make the remotes list be just a copy of the full remotes view.
The third option is surely the easiest to implement. The second option with _only_ a list of remotes (no extra info) is also very easy to implement _and_ fast to render. The second option with extra info, or the first option, on the other hand, require the retrieval of some additional data which, maybe due to my limited knowledge of git, essentially means retrieving _all_ the remote heads and then doing the filtering in gitweb. But once we're getting all the information, why not display it all? isn't it faster to just display all of it, in which case we go back to option 3?
If we go with option 3, it does make sense to get all remote names and all remote branches at once, and thus to make the git_get_remotes() call do all of the work.
Show 13 quoted lines
>> +}
>> +
>> sub git_get_references {
>> my $type = shift || "";
>> my %refs;
>> @@ -4979,7 +4989,7 @@ sub git_heads_body {
>> "<td class=\"link\">" .
>> $cgi->a({-href => href(action=>"shortlog", hash=>$ref{'fullname'})}, "shortlog") . " | " .
>> $cgi->a({-href => href(action=>"log", hash=>$ref{'fullname'})}, "log") . " | " .
>> - $cgi->a({-href => href(action=>"tree", hash=>$ref{'fullname'}, hash_base=>$ref{'name'})}, "tree") .
>> + $cgi->a({-href => href(action=>"tree", hash=>$ref{'fullname'}, hash_base=>$ref{'fullname'})}, "tree") .
>
> This is independent change, and should be in a separate commit, isn't it?Probably yes, with an explanation of the why.
Show 17 quoted lines
>> "</td>\n" .
>> "</tr>";
>> }
>> @@ -4991,6 +5001,19 @@ sub git_heads_body {
>> print "</table>\n";
>> }
>>
>> +sub git_remotes_body {
>> + my ($remotedata, $head) = @_;
>> + my @remotenames = @{$remotedata->[0]};
>> + my @allheads = @{$remotedata->[1]};
>
> Why not
>
> + my ($remotenames, $allheads, $head) = @_;
>
> Beside, isn't it $remote_heads and not $allheads?I think it's a leftover name choice from the first version of the patch. Can change.
Show 13 quoted lines
>> + foreach my $remote (@remotenames) {
>
> It would be then
>
> + foreach my $remote (@$remotenames) {
>
>> + my @remoteheads = grep { $_->{'name'} =~ s!^\Q$remote\E/!! } @allheads;
>
> Should we display remote even if it doesn't have any remote heads
> associated with it?
>
> By the way, it would be place where we could limit number of
> remote-tracking branches displayed in each remote block.But does it make sense to reduce the number of displayed branches after we got the information about all of them? I think it depends on what summary view is intended to do exactly.
Show 6 quoted lines
>> + git_begin_group("remotes", $remote, "remotes/$remote",$remote);
>> + git_heads_body(\@remoteheads, $head);
>> + git_end_group();
>
> This would have to be modified with change to git_begin_group() /
> / git_end_group().Of course.
> BTW isn't it premature generalization? It is only place AFAIKS that > uses git_*_group() subroutines.
It's the only current use but I believe that, since it's factored out now already and since it may be used in other views too (think: grouping heads or tags by prefix) it might make sense to keep it this way.
Show 16 quoted lines
>> + }
>> +
>> +}
>> +
>> sub git_search_grep_body {
>> my ($commitlist, $from, $to, $extra) = @_;
>> $from = 0 unless defined $from;
>> @@ -5137,7 +5160,7 @@ sub git_summary {
>> # there are more ...
>> my @taglist = git_get_tags_list(16);
>> my @headlist = git_get_heads_list(16, 'heads');
>> - my @remotelist = $remote_heads ? git_get_heads_list(16, 'remotes') : ();
>> + my @remotelist = $remote_heads ? git_get_remotes(16) : ();
>
> No change of git_get_remotes() does only one thing: returning list
> of remotes.See above about what should git_get_remotes() do. Even better, I was thinking about git_get_remotes() returning a hash (mapping remote names to the heads from that remote)
So the big question (which essentially determines the functionality provided by this last patch in the set) is: what do we want to do in summary view?
-- Giuseppe "Oblomov" Bilotta