[PATCH 0/2] Make git_get_refs_list do work of git_get_references

[PATCH 0/2] Make git_get_refs_list do work of git_get_references

[Jakub Narebski]

This is resend of short series of patches. Second patch in series
was corrected twice ([PATCH 2/2 (take 2)] and [PATCH 2/2 (take 3)]),
but neither of corrections made to git mailing list.

Shortlog:
 [PATCH 1/2] gitweb: Always use git-peek-remote in git_get_references
 [PATCH 2/2] gitweb: Make git_get_refs_list do work of git_get_references

Diffstat:
 gitweb/gitweb.perl |   76 ++++++++++++++++++++++++++++++----------------------
 1 files changed, 44 insertions(+), 32 deletions(-)
-- 
Jakub Narebski
Poland

[PATCH 1/2] gitweb: Always use git-peek-remote in git_get_references

[Jakub Narebski]

Instead of trying to read info/refs file, which might not be present
(we did fallback to git-ls-remote), always use git-peek-remote in
git_get_references.

It is preparation for git_get_refs_info to also return references
info. We cannot use info/refs for git_get_refs_info as the information
contained therein is usually stale.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |   10 ++--------
 1 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index c77270c..034a88c 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -828,16 +828,10 @@ sub git_get_project_owner {
 sub git_get_references {
 	my $type = shift || "";
 	my %refs;
-	my $fd;
 	# 5dc01c595e6c6ec9ccda4f6f69c131c0dd945f8c	refs/tags/v2.6.11
 	# c39ae07f393806ccf406ef966e9a15afc43cc36a	refs/tags/v2.6.11^{}
-	if (-f "$projectroot/$project/info/refs") {
-		open $fd, "$projectroot/$project/info/refs"
-			or return;
-	} else {
-		open $fd, "-|", git_cmd(), "ls-remote", "."
-			or return;
-	}
+	open my $fd, "-|", $GIT, "peek-remote", "$projectroot/$project/"
+		or return;
 
 	while (my $line = <$fd>) {
 		chomp $line;
-- 
1.4.2.1

Re: [PATCH 1/2] gitweb: Always use git-peek-remote in git_get_references

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Instead of trying to read info/refs file, which might not be present
> (we did fallback to git-ls-remote), always use git-peek-remote in
> git_get_references.
>
> It is preparation for git_get_refs_info to also return references
> info. We cannot use info/refs for git_get_refs_info as the information
> contained therein is usually stale.

What the patch does is sane, but I think the last sentence of
the proposed log message is not.  If info/refs is "usually
stale", it is a bug in the repository to have such a stale file.

The real reason for this patch is that a repository served by
gitweb is not necessarily meant to be fetched over HTTP and
info/refs does not have to be there.

Re: [PATCH 1/2] gitweb: Always use git-peek-remote in git_get_references

[Jakub Narebski]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > Instead of trying to read info/refs file, which might not be present
> > (we did fallback to git-ls-remote), always use git-peek-remote in
> > git_get_references.
> >
> > It is preparation for git_get_refs_info to also return references
> > info. We cannot use info/refs for git_get_refs_info as the information
> > contained therein is usually stale.
> 
> What the patch does is sane, but I think the last sentence of
> the proposed log message is not.  If info/refs is "usually
> stale", it is a bug in the repository to have such a stale file.
> 
> The real reason for this patch is that a repository served by
> gitweb is not necessarily meant to be fetched over HTTP and
> info/refs does not have to be there.

If the repository served by gitweb is updated using push, then
info/refs updated using post-update hook is up-to-date. If repository
is "live" repository, updated also using commit, rebase and such,
info/refs is usually stale.

If there were post-commit hook, and it's contents was the default 
post-update hook, info/refs would be never stale. And we could read
from into/refs, and fallback to git-peek-remote if it doesn't exist...
but we don't know if info/refs has current info.
-- 
Jakub Narebski
Poland

Re: [PATCH 1/2] gitweb: Always use git-peek-remote in git_get_references

[Andreas Ericsson]

Jakub Narebski wrote:
> 
> If there were post-commit hook, and it's contents was the default 
> post-update hook, info/refs would be never stale. And we could read
> from into/refs, and fallback to git-peek-remote if it doesn't exist...
> but we don't know if info/refs has current info.

ls -l .git/hooks/post-commit
-rw-r----- 1 exon exon 152 Sep 21 10:54 .git/hooks/post-commit


My post-update hook doesn't maintain info/refs for me. I'm sure that if 
someone set it up to do so they would also set the post-commit hook up 
to do the same.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

[PATCH 2/2] gitweb: Make git_get_refs_list do work of git_get_references

[Jakub Narebski]

Make git_get_refs_list do also work of git_get_references, to avoid
calling git-peek-remote twice.  Change meaning of git_get_refs_list
meaning: it is now type, and not a full path, e.g. we now use
git_get_refs_list("heads") instead of former
git_get_refs_list("refs/heads").

Modify git_summary to use only one call to git_get_refs_list instead
of one call to git_get_references and two to git_get_refs_list.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Benchmarks show that this version is slightly faster than
"before" version:

  2626.133 +/- 98.1 ms after vs 3086.579 +/- 85.0 ms before (summary)
  2233.814 +/- 13.9 ms after vs 2504.025 +/- 46.1 ms before (tags)
   869.533 +/-  7.7 ms after vs  979.281 +/- 88.1 ms before (heads)


 gitweb/gitweb.perl |   66 +++++++++++++++++++++++++++++++++-------------------
 1 files changed, 42 insertions(+), 24 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 034a88c..01fae94 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1119,7 +1119,8 @@ ## .....................................
 ## parse to array of hashes functions
 
 sub git_get_refs_list {
-	my $ref_dir = shift;
+	my $type = shift || "";
+	my %refs;
 	my @reflist;
 
 	my @refs;
@@ -1127,14 +1128,21 @@ sub git_get_refs_list {
 		or return;
 	while (my $line = <$fd>) {
 		chomp $line;
-		if ($line =~ m/^([0-9a-fA-F]{40})\t$ref_dir\/?([^\^]+)$/) {
-			push @refs, { hash => $1, name => $2 };
-		} elsif ($line =~ m/^[0-9a-fA-F]{40}\t$ref_dir\/?(.*)\^\{\}$/ &&
-		         $1 eq $refs[-1]{'name'}) {
-			# most likely a tag is followed by its peeled
-			# (deref) one, and when that happens we know the
-			# previous one was of type 'tag'.
-			$refs[-1]{'type'} = "tag";
+		if ($line =~ m/^([0-9a-fA-F]{40})\trefs\/($type\/?([^\^]+))(\^\{\})?$/) {
+			if (defined $refs{$1}) {
+				push @{$refs{$1}}, $2;
+			} else {
+				$refs{$1} = [ $2 ];
+			}
+
+			if (! $4) { # unpeeled, direct reference
+				push @refs, { hash => $1, name => $3 }; # without type
+			} elsif ($3 eq $refs[-1]{'name'}) {
+				# most likely a tag is followed by its peeled
+				# (deref) one, and when that happens we know the
+				# previous one was of type 'tag'.
+				$refs[-1]{'type'} = "tag";
+			}
 		}
 	}
 	close $fd;
@@ -1150,7 +1158,7 @@ sub git_get_refs_list {
 	}
 	# sort refs by age
 	@reflist = sort {$b->{'epoch'} <=> $a->{'epoch'}} @reflist;
-	return \@reflist;
+	return (\@reflist, \%refs);
 }
 
 ## ----------------------------------------------------------------------
@@ -2114,14 +2122,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) {
@@ -2291,7 +2299,19 @@ sub git_summary {
 
 	my $owner = git_get_project_owner($project);
 
-	my $refs = git_get_references();
+	my ($reflist, $refs) = git_get_refs_list();
+
+	my @taglist;
+	my @headlist;
+	foreach my $ref (@$reflist) {
+		if ($ref->{'name'} =~ s!^heads/!!) {
+			push @headlist, $ref;
+		} else {
+			$ref->{'name'} =~ s!^tags/!!;
+			push @taglist, $ref;
+		}
+	}
+
 	git_header_html();
 	git_print_page_nav('summary','', $head);
 
@@ -2321,17 +2341,15 @@ sub git_summary {
 	git_shortlog_body(\@revlist, 0, 15, $refs,
 	                  $cgi->a({-href => href(action=>"shortlog")}, "..."));
 
-	my $taglist = git_get_refs_list("refs/tags");
-	if (defined @$taglist) {
+	if (@taglist) {
 		git_print_header_div('tags');
-		git_tags_body($taglist, 0, 15,
+		git_tags_body(\@taglist, 0, 15,
 		              $cgi->a({-href => href(action=>"tags")}, "..."));
 	}
 
-	my $headlist = git_get_refs_list("refs/heads");
-	if (defined @$headlist) {
+	if (@headlist) {
 		git_print_header_div('heads');
-		git_heads_body($headlist, $head, 0, 15,
+		git_heads_body(\@headlist, $head, 0, 15,
 		               $cgi->a({-href => href(action=>"heads")}, "..."));
 	}
 
@@ -2542,7 +2560,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);
 	}
@@ -2555,9 +2573,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();
 }
-- 
1.4.2.1

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Make git_get_refs_list do also work of git_get_references, to avoid
> calling git-peek-remote twice.  Change meaning of git_get_refs_list
> meaning: it is now type, and not a full path, e.g. we now use
> git_get_refs_list("heads") instead of former
> git_get_refs_list("refs/heads").
>
> Modify git_summary to use only one call to git_get_refs_list instead
> of one call to git_get_references and two to git_get_refs_list.

Will apply as-is; the following comments might be useful for
further improvements.

> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 034a88c..01fae94 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1127,14 +1128,21 @@ sub git_get_refs_list {
> +		if ($line =~ m/^([0-9a-fA-F]{40})\trefs\/($type\/?([^\^]+))(\^\{\})?$/) {
> +			if (defined $refs{$1}) {
> +				push @{$refs{$1}}, $2;
> +			} else {
> +				$refs{$1} = [ $2 ];
> +			}

Logically this should be "exists $refs{$1}" but defined would
work fine in this particular case, because the elements of the
hash are always an arrayref (this is just an advise to have the
right habit).

> +			if (! $4) { # unpeeled, direct reference
> +				push @refs, { hash => $1, name => $3 }; # without type
> +			} elsif ($3 eq $refs[-1]{'name'}) {
> +				# most likely a tag is followed by its peeled
> +				# (deref) one, and when that happens we know the
> +				# previous one was of type 'tag'.
> +				$refs[-1]{'type'} = "tag";
> +			}
>...
> -	return \@reflist;
> +	return (\@reflist, \%refs);

You are maintaining reflist (an array of hashrefs each element
of which describes name, type, hash and other parse_ref()
information for the ref) and refs (a hash that maps from name to
hash) separatly.  I wonder if this really has the performance
advantage over just compute and return reflist from here and
have the callers who need the mapping to derive it from the list
(i.e.

	my ($reflist) = git_get_refs_list();
	my %refs = map { $_->{name} => $_->{hash} } @$reflist;
).

> @@ -2114,14 +2122,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) {

I guess either would do but I think it is more conventional to
say:

	my $limit = @list;
        for (my $i = $from; $i < $limit; $i++) { ... }

than 

	my $limit = $#list;
        for (my $i = $from; $i <= $limit; $i++) { ... }

At least the former would be easier to read for C types among us.

The remaining hunks are very nice improvements.

> @@ -2291,7 +2299,19 @@ sub git_summary {
> @@ -2321,17 +2341,15 @@ sub git_summary {
> @@ -2542,7 +2560,7 @@ sub git_tags {
> @@ -2555,9 +2573,9 @@ sub git_heads {

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

[Jakub Narebski]

Junio C Hamano wrote:
> Jakub Narebski wrote:
>> +                     if (! $4) { # unpeeled, direct reference
>> +                             push @refs, { hash => $1, name => $3 }; # without type
>> +                     } elsif ($3 eq $refs[-1]{'name'}) {
>> +                             # most likely a tag is followed by its peeled
>> +                             # (deref) one, and when that happens we know the
>> +                             # previous one was of type 'tag'.
>> +                             $refs[-1]{'type'} = "tag";
>> +                     }
>>...
>> -     return \@reflist;
>> +     return (\@reflist, \%refs);
> 
> You are maintaining reflist (an array of hashrefs each element
> of which describes name, type, hash and other parse_ref()
> information for the ref) and refs (a hash that maps from name to
> hash) separatly.  I wonder if this really has the performance
> advantage over just compute and return reflist from here and
> have the callers who need the mapping to derive it from the list
> (i.e.
> 
>         my ($reflist) = git_get_refs_list();
>         my %refs = map { $_->{name} => $_->{hash} } @$reflist;
> ).

The %refs hash contain also map from peeled (dereferenced) object
to ref name. The information about derefs is not present in @reflist.
So the answer is that you can't derive %refs from @reflist. 
-- 
Jakub Narebski
ShadeHawk on #git
Poland

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> The %refs hash contain also map from peeled (dereferenced) object
> to ref name. The information about derefs is not present in @reflist.
> So the answer is that you can't derive %refs from @reflist. 

There we found one candidate for further interface clean-up ;-).