Re: [PATCH 2/2 (take 2)] gitweb: Make git_get_refs_list do work of git_get_references
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Sep 17, 2006, 10:00 UTC
- Message-ID
- <200609171158.48358.jnareb@gmail.com>
- In-Reply-To
- <7vfyeq6dn3.fsf@assigned-by-dhcp.cox.net>
Junio C Hamano wrote:
Show 10 quoted lines
> 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.
Show 14 quoted lines
> > @@ -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.
Show 21 quoted lines
> > @@ -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?
Show 26 quoted lines
> > @@ -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