From: Jakub Narebski Date: Tue, 02 Nov 2010 23:58:52 GMT Subject: Re: [PATCHv6 10/10] gitweb: group remote heads by remote Message-ID: <201011030058.53366.jnareb@gmail.com> In-Reply-To: On Tue, 2 Nov 2010, Giuseppe Bilotta wrote: > 2010/10/27 Jakub Narebski : >> On Sun, 24 Oct 2010, Giuseppe Bilotta wrote: >> >>> In remote and summary view, display a block for each remote, with the >>> fetch and push URL(s) as well as the list of the remote heads. >>> >>> In summary view, if the number of remotes is higher than a prescribed >>> limit, only display the first remotes and their fetch and push >>> urls, without any heads information and without grouping. >> I guess that we can always implement more fancy limiting in the future >> (e.g. taking into account total number of displayed remote heads). > > 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. >>> +# >>> +# It is possible to limit the retrieved remotes either by number >>> +# (specifying a -limit parameter) or by name (-wanted parameter). >> >> I don't quite like limiting when generating, and would prefer do limiting >> on display, especially if not doing limiting would not affect performance >> much (git command invoked doesn't do limiting, like in case of >> git_get_heads_list / git_get_tags_list or *most important* parse_commits). >> >> Especially if it complicates code that much (see below). >> >> Not doing limiting here, in git_get_remotes_list (or just git_get_remotes) >> would also make API simpler; the single optional argument would be name of >> remote we want to retrieve. > > Hm. By the same token, there would be no need to do the limiting even > when trying to get information about a single remote, meaning we could > make the sub totally parameter-less. OTOH, this would make the calling > routine quite more complex (since it would have to check if the key is > there, and then select that single key etc), much more so than > limiting the number of displayed heads. True, we have to balance complexity in generating function vs. complexity in display code. > I'll take the numerical limiting off the routine. You meant here limiting number of remotes returned (except of case of limiting to single remote), isn't it? >>> + >>> +# Takes a hash of remotes as first parameter and fills it by adding the >>> +# available remote heads for each of the indicated remotes. >>> +# A maximum number of heads can also be specified. >> >> All git_get_* subroutines _return_ something; this looks more like fill_* >> function for me. > > I'm not particularly enamored with _get_, fill will do We have fill_from_file_info, fill_project_list_info; I think fill_remote_heads would be a good name. >> >>> +sub git_get_remote_heads { >>> +     my ($remotes, $limit) = @_; >>> +     my @heads = map { "remotes/$_" } keys %$remotes; >>> +     my @remoteheads = git_get_heads_list($limit, @heads); >>> +     foreach (keys %$remotes) { >>> +             my $remote = $_; >>> +             $remotes->{$remote}{'heads'} = [ grep { >>> +                     $_->{'name'} =~ s!^$remote/!! >>> +                     } @remoteheads ]; >>> +     } >>> +} >> >> We could remove limiting number of heads shown per remote also from here, >> but this time 1.) the $limit parameter is passed down to git_get_heads_list >> which in turn uses $limit as parameter to git command  2.) it doesn't >> simplify code almost at all: >> >> +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 ]; >> +       } >> +} I now think that passing limit to fill_remote_heads (like you have) might be a good solution (contrary to what I wrote earlier). >> 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. >>> + >>> +     my $urls = "\n" ; >>> + >>> +     if (defined $fetch) { >>> +             if ($fetch eq $push) { >>> +                     $urls .= format_repo_url("URL", $fetch); >>> +             } else { >>> +                     $urls .= format_repo_url("Fetch URL", $fetch); >>> +                     $urls .= format_repo_url("Push URL", $push) if defined $push; >>> +             } >>> +     } elsif (defined $push) { >>> +             $urls .= format_repo_url("Push URL", $push); >>> +     } else { >>> +             $urls .= format_repo_url("", "No remote URL"); >>> +     } >>> + >>> +     $urls .= "
\n"; >> >> I'm not sure about naming this variable $urls... > > I'm open to suggestions. $urls_table ? It is better. >> 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. Thank you on diligently working on this feature. -- Jakub Narebski Poland