Re: [PATCH v2 05/11] gitweb: git_split_heads_body function.

[Jakub Narebski <jnareb@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.

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

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

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

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

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