Re: [PATCH 7/7] gitweb: group remote heads

[Giuseppe Bilotta <giuseppe.bilotta@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:
> 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]

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

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

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

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

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

>> +     }
>> +
>> +}
>> +
>>  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