Re: [PATCH 2/2 (take 2)] gitweb: Make git_get_refs_list do work of git_get_references

[Jakub Narebski <jnareb@gmail.com>]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > This was supposed to make gitweb performance better, by 
> > (in the case of git_summary) replacing three calls to git-peek-remote
> > (or one reading info/refs and two calls to git-peek-remote) by only 
> > one such a call. ApacheBench shows that after changes summary and tags 
> > views are slower, while heads remains the same or even faster (?).
> 
> That does not give us much confidence in the patch, does it?  If
> it results in cleaner code that is fine, though.

This was because the additional test you proposed for "it is tag if
it is followed by deref" was implemented incorrectly. Corrected in "take 3"
of this patch.

> > @@ -1107,8 +1115,8 @@ sub git_get_refs_list {
> >  		push @reflist, \%ref_item;
> >  	}
> >  	# sort refs by age
> > -	@reflist = sort {$b->{'epoch'} <=> $a->{'epoch'}} @reflist;
> > -	return \@reflist;
> > +	#@reflist = sort {$b->{'epoch'} <=> $a->{'epoch'}} @reflist;
> > +	return (\@reflist, \%refs);
> >  }
> 
> I've already said I do not like commented out lines that do not
> say why they are commented out, but I sense something more
> serious here.  Doesn't this removal of sorting affect the
> callers?

Corrected in "take 3" of this patch.

> > @@ -2072,14 +2080,14 @@ sub git_tags_body {
> >  
> >  sub git_heads_body {
> >  	# uses global variable $project
> > -	my ($taglist, $head, $from, $to, $extra) = @_;
> > +	my ($headlist, $head, $from, $to, $extra) = @_;
> >  	$from = 0 unless defined $from;
> > -	$to = $#{$taglist} if (!defined $to || $#{$taglist} < $to);
> > +	$to = $#{$headlist} if (!defined $to || $#{$headlist} < $to);
> >  
> >  	print "<table class=\"heads\" cellspacing=\"0\">\n";
> >  	my $alternate = 0;
> >  	for (my $i = $from; $i <= $to; $i++) {
> > -		my $entry = $taglist->[$i];
> > +		my $entry = $headlist->[$i];
> >  		my %tag = %$entry;
> >  		my $curr = $tag{'id'} eq $head;
> >  		if ($alternate) {
> 
> Unrelated but indeed is a good clean-up.  It was quite confusing
> before (I think this was a result of cut and paste reuse).

And I see that this cleanup is incomplete (%tag = %$entry). Gaah.
Should I split cleanup patch?


> > @@ -2500,7 +2518,7 @@ sub git_tags {
> >  	git_print_page_nav('','', $head,undef,$head);
> >  	git_print_header_div('summary', $project);
> >  
> > -	my $taglist = git_get_refs_list("refs/tags");
> > +	my ($taglist) = git_get_refs_list("tags");
> >  	if (defined @$taglist) {
> >  		git_tags_body($taglist);
> >  	}
> > @@ -2513,9 +2531,9 @@ sub git_heads {
> >  	git_print_page_nav('','', $head,undef,$head);
> >  	git_print_header_div('summary', $project);
> >  
> > -	my $taglist = git_get_refs_list("refs/heads");
> > -	if (defined @$taglist) {
> > -		git_heads_body($taglist, $head);
> > +	my ($headlist) = git_get_refs_list("heads");
> > +	if (defined @$headlist) {
> > +		git_heads_body($headlist, $head);
> >  	}
> >  	git_footer_html();
> >  }
> 
> It always confuses me when people say:
> 
> 	if (defined @$arrayref)

Gaah. It should be either 
	if (defined $arrayref)
or
	if (@$arrayref)
or (if one is truly paranoid) ;-)
	if (defined $arrayref && ref($arrayref) eq "ARRAY" && @$arrayref)

[...]
> You probably would want to grep for 'defined @' and fix them all
> at once.

Will do. Unless someone will do this first.

-- 
Jakub Narebski
Poland