Re: [PATCH v2 05/11] gitweb: git_split_heads_body function.
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Nov 14, 2008, 23:59 UTC
- Message-ID
- <200811150059.14515.jnareb@gmail.com>
- In-Reply-To
- <1226616555-24503-6-git-send-email-giuseppe.bilotta@gmail.com>
On Thu, 13 Nov 2008, Giuseppe Bilotta wrote:
> The purpose of this function is to split a headlist into groups > determined by the leading part of the refname, and call git_heads_body() > on each group.
What is the reason of this patch? Is it to split remote-tracking branches ('remotes' references) into remotes, and group them by the remote repository name?
If it is true, then first: you should have wrote the _reason_ behind this patch and not only what it does in this commit message. And use better summary (commit title / subject of this patch).
Second, this patch wouldn't do what you want from it if there are remotes with '/' in name. I for example use "gsoc2008/gitweb-caching" for Lea Wiemann repository with her GSoC 2008 work on adding caching to gitweb. Because there are many ways to specify remotes due to backwards compatibility (and simplicity, as some for example prefer old 'branches/' way to specify remotes), namely config, files under '.git/remotes', and (from Cogito) files in '.git/branches', you would have to either reimplement/reuse parts of git-remote (there is old Perl implementation in contrib/examples), or use "git remote" or "git remote -v" command output[1].
So from me there is slight NAK on this patch, in this form.
Show 16 quoted lines
>
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
> gitweb/gitweb.perl | 33 ++++++++++++++++++++++++++++++++-
> 1 files changed, 32 insertions(+), 1 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index a736f2a..836b6ba 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -4271,6 +4271,37 @@ sub git_tags_body {
> print "</table>\n";
> }
>
> +sub git_split_heads_body {
> + my ($headlist, $head, $from, $to, $extra) = @_;It should probably be said somewhere that git_split_heads_body has to have the same signature as git_heads_body.
> + my %headlists; > + my $leader; my $list; my @list;
Style - I would use:
+ my ($leader, $list, @list);
although I wouldn't use $list and @list together...
Show 7 quoted lines
> +
> + # Split @$headlist into a hash of lists
> + map {
> + my %ref = %$_;
> + $ref{'hname'} = $ref{'name'};
> + if ($ref{'name'} =~ /\//) {
> + $ref{'name'} =~ s!^([^/]+)/!!;As I said, this would fail on for example "gsoc2008/gitweb-caching" remote...
> + $leader = $1;
> + } else {
> + $leader = "\000";Can't you use undef or "" for $leader? $headlists{undef} works...Show 8 quoted lines
> + }
> + if (defined $headlists{$leader}) {
> + @list = @{$headlists{$leader}}
> + } else {
> + @list = ()
> + }
> + push @list, \%ref;
> + $headlists{$leader} = [@list];We have similar code in href(), but we use there:
if (defined $ref{key}) {
push @{$ref{$key}}, $elem;
} else {
$ref{$key} = [ $elem ];
}Isn't it simpler and easier to understand?
> + } @$headlist;
Why such ugly and ungainy 'map' invocation, instead of IMHO simpler and better here foreach loop?
Show 7 quoted lines
> +
> + foreach $leader (sort(keys %headlists)) {
> + print "<b>$leader</b><br/>\n" unless $leader eq "\000";
> + $list = $headlists{$leader};
> + git_heads_body($list, $head, $from, $to, $extra);
> + }
> +}Wouldn't be it simpler to loop over @$headlist, and if prefix (or to be more exact repository shorthand aka 'remote') changes then run git_heads_body, adjusting $from / $to accordingly, based on current and remembered index? I think we can assume that list is sorted by refname, can't we? If not then perhaps the way by building hash is good idea after all...
Show 13 quoted lines
> +
> sub git_heads_body {
> # uses global variable $project
> my ($headlist, $head, $from, $to, $extra) = @_;
> @@ -4541,7 +4572,7 @@ sub git_summary {
>
> if (@remotelist) {
> git_print_header_div('remotes');
> - git_heads_body(\@remotelist, $head, 0, 15,
> + git_split_heads_body(\@remotelist, $head, 0, 15,
> $#remotelist <= 15 ? undef :
> $cgi->a({-href => href(action=>"heads")}, "..."));
> }Nice.
Footnotes: ========== [1] It is strange that there is no explicit "git remote list"
-- Jakub Narebski Poland