[PATCHv3 0/4] gitweb: remote heads feature

[PATCHv3 0/4] gitweb: remote heads feature

[Giuseppe Bilotta]

A new version of the remote heads feature patchset. This is considerably
stripped down in comparison to the previous version, as it's limited to
implementing the remote_heads feature and the minimal amount of UI changes to
accomodate it cleanly. Other features such as grouping of heads and supporting
detached HEAD will be implemented separately.

*NOTE*: this patchset is based on my previous

"gitweb: fixes to gitweb feature check code"

patch http://article.gmane.org/gmane.comp.version-control.git/101070

Giuseppe Bilotta (4):
  gitweb: introduce remote_heads feature
  gitweb: git_get_heads_list accepts an optional list of refs.
  gitweb: separate heads and remotes lists
  gitweb: link heads and remotes view

 gitweb/gitweb.perl |   74 +++++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 68 insertions(+), 6 deletions(-)

[PATCHv3 1/4] gitweb: introduce remote_heads feature

[Giuseppe Bilotta]

With this feature enabled, remotes are retrieved (and displayed)
when getting (and displaying) the heads list. Typical usage would be for
local repository browsing, e.g. by using git-instaweb (or even a more
permanent gitweb setup), to check the repository status and the relation
between tracking branches and the originating remotes.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |   31 +++++++++++++++++++++++++++++--
 1 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index b0d00ea..e1f81f6 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -329,6 +329,18 @@ our %feature = (
 	'ctags' => {
 		'override' => 0,
 		'default' => [0]},
+
+	# Make gitweb show remotes too in the heads list
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'remote_heads'}{'default'} = [1];
+	# To have project specific config enable override in $GITWEB_CONFIG
+	# $feature{'remote_heads'}{'override'} = 1;
+	# and in project config gitweb.remote_heads = 0|1;
+	'remote_heads' => {
+		'sub' => \&feature_remote_heads,
+		'override' => 0,
+		'default' => [0]},
 );
 
 # retrieve the value of a given feature, as an array
@@ -410,6 +422,18 @@ sub feature_pickaxe {
 	return ($_[0]);
 }
 
+sub feature_remote_heads {
+	my ($val) = git_get_project_config('remote_heads', '--bool');
+
+	if ($val eq 'true') {
+		return (1);
+	} elsif ($val eq 'false') {
+		return (0);
+	}
+
+	return ($_[0]);
+}
+
 # checking HEAD file with -e is fragile if the repository was
 # initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed
 # and then pruned.
@@ -2660,10 +2684,12 @@ sub git_get_heads_list {
 	my $limit = shift;
 	my @headslist;
 
+	my $remote_heads = gitweb_check_feature('remote_heads');
+
 	open my $fd, '-|', git_cmd(), 'for-each-ref',
 		($limit ? '--count='.($limit+1) : ()), '--sort=-committerdate',
 		'--format=%(objectname) %(refname) %(subject)%00%(committer)',
-		'refs/heads'
+		'refs/heads', ( $remote_heads ? 'refs/remotes' : '')
 		or return;
 	while (my $line = <$fd>) {
 		my %ref_item;
@@ -2674,8 +2700,9 @@ sub git_get_heads_list {
 		my ($committer, $epoch, $tz) =
 			($committerinfo =~ /^(.*) ([0-9]+) (.*)$/);
 		$ref_item{'fullname'}  = $name;
-		$name =~ s!^refs/heads/!!;
+		$name =~ s!^refs/(head|remote)s/!!;
 
+		$ref_item{'class'} = $1;
 		$ref_item{'name'}  = $name;
 		$ref_item{'id'}    = $hash;
 		$ref_item{'title'} = $title || '(no commit message)';
-- 
1.5.6.5

[PATCHv3 2/4] gitweb: git_get_heads_list accepts an optional list of refs.

[Giuseppe Bilotta]

git_get_heads_list(limit, class1, class2, ...) can now be used to retrieve
refs/class1, refs/class2 etc. Defaults to ('heads') or ('heads', 'remotes')
depending on the remote_heads option.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index e1f81f6..0512020 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2681,15 +2681,18 @@ sub parse_from_to_diffinfo {
 ## parse to array of hashes functions
 
 sub git_get_heads_list {
-	my $limit = shift;
+	my ($limit, @class) = @_;
+	unless (defined @class) {
+		my $remote_heads = gitweb_check_feature('remote_heads');
+		@class = ('heads', $remote_heads ? 'remotes' : undef);
+	}
+	my @refs = map { "refs/$_" } @class;
 	my @headslist;
 
-	my $remote_heads = gitweb_check_feature('remote_heads');
-
 	open my $fd, '-|', git_cmd(), 'for-each-ref',
 		($limit ? '--count='.($limit+1) : ()), '--sort=-committerdate',
 		'--format=%(objectname) %(refname) %(subject)%00%(committer)',
-		'refs/heads', ( $remote_heads ? 'refs/remotes' : '')
+		@refs
 		or return;
 	while (my $line = <$fd>) {
 		my %ref_item;
-- 
1.5.6.5

[PATCHv3 3/4] gitweb: separate heads and remotes lists

[Giuseppe Bilotta]

We specialize the 'heads' action to only display local branches, and
introduce a 'remotes' action to display the remote branches (only
available when the remotes_head feature is enabled).

Mirroring this, we also split the heads list in summary view into
local and remote lists, each linking to the appropriate action.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |   30 ++++++++++++++++++++++++++++--
 1 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 0512020..6b09918 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -527,6 +527,7 @@ our %actions = (
 	"heads" => \&git_heads,
 	"history" => \&git_history,
 	"log" => \&git_log,
+	"remotes" => \&git_remotes,
 	"rss" => \&git_rss,
 	"atom" => \&git_atom,
 	"search" => \&git_search,
@@ -4467,6 +4468,7 @@ sub git_summary {
 	my %co = parse_commit("HEAD");
 	my %cd = %co ? parse_date($co{'committer_epoch'}, $co{'committer_tz'}) : ();
 	my $head = $co{'id'};
+	my $remote_heads = gitweb_check_feature('remote_heads');
 
 	my $owner = git_get_project_owner($project);
 
@@ -4474,7 +4476,8 @@ sub git_summary {
 	# These get_*_list functions return one more to allow us to see if
 	# there are more ...
 	my @taglist  = git_get_tags_list(16);
-	my @headlist = git_get_heads_list(16);
+	my @headlist = git_get_heads_list(16, 'heads');
+	my @remotelist = $remote_heads ? git_get_heads_list(16, 'remotes') : ();
 	my @forklist;
 	my $check_forks = gitweb_check_feature('forks');
 
@@ -4553,6 +4556,13 @@ sub git_summary {
 		               $cgi->a({-href => href(action=>"heads")}, "..."));
 	}
 
+	if (@remotelist) {
+		git_print_header_div('remotes');
+		git_heads_body(\@remotelist, $head, 0, 15,
+		               $#remotelist <= 15 ? undef :
+		               $cgi->a({-href => href(action=>"remotes")}, "..."));
+	}
+
 	if (@forklist) {
 		git_print_header_div('forks');
 		git_project_list_body(\@forklist, 'age', 0, 15,
@@ -4729,13 +4739,29 @@ sub git_heads {
 	git_print_page_nav('','', $head,undef,$head);
 	git_print_header_div('summary', $project);
 
-	my @headslist = git_get_heads_list();
+	my @headslist = git_get_heads_list(undef, 'heads');
 	if (@headslist) {
 		git_heads_body(\@headslist, $head);
 	}
 	git_footer_html();
 }
 
+sub git_remotes {
+	gitweb_check_feature('remote_heads')
+		or die_error(403, "Remote heads view is disabled");
+
+	my $head = git_get_head_hash($project);
+	git_header_html();
+	git_print_page_nav('','', $head,undef,$head);
+	git_print_header_div('summary', $project);
+
+	my @remotelist = git_get_heads_list(undef, 'remotes');
+	if (@remotelist) {
+		git_heads_body(\@remotelist, $head);
+	}
+	git_footer_html();
+}
+
 sub git_blob_plain {
 	my $type = shift;
 	my $expires;
-- 
1.5.6.5

[PATCHv3 4/4] gitweb: link heads and remotes view

[Giuseppe Bilotta]

Add a link in heads view to remotes view (if the feature is
enabled), and conversely from remotes to heads.
---
 gitweb/gitweb.perl |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 6b09918..95162db 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -4736,7 +4736,10 @@ sub git_tags {
 sub git_heads {
 	my $head = git_get_head_hash($project);
 	git_header_html();
-	git_print_page_nav('','', $head,undef,$head);
+	my $heads_nav = gitweb_check_feature('remote_heads') ?
+		$cgi->a({-href => href(action=>"remotes", -replay=>1)},
+		        "remotes") : undef;
+	git_print_page_nav('','', $head,undef,$head, $heads_nav);
 	git_print_header_div('summary', $project);
 
 	my @headslist = git_get_heads_list(undef, 'heads');
@@ -4752,7 +4755,10 @@ sub git_remotes {
 
 	my $head = git_get_head_hash($project);
 	git_header_html();
-	git_print_page_nav('','', $head,undef,$head);
+	my $heads_nav =
+		$cgi->a({-href => href(action=>"heads", -replay=>1)},
+		        "heads");
+	git_print_page_nav('','', $head,undef,$head, $heads_nav);
 	git_print_header_div('summary', $project);
 
 	my @remotelist = git_get_heads_list(undef, 'remotes');
-- 
1.5.6.5

Re: [PATCHv3 1/4] gitweb: introduce remote_heads feature

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> With this feature enabled, remotes are retrieved (and displayed)
> when getting (and displaying) the heads list.

Wouldn't it be easier to read if you just said: "Include 'remotes' in the
heads_list", because:

 - "heads list" does not sound like a proper English phrase but you are
   referring to the sub "heads_list";

 - it is obvious and unnecessary to say "when getting, they are retrieved,
   when displaying, they are displayed", which is what your parenthesized
   parts of the sentence is about;

I am also suggesting to drop "With this feature enabled"; I do not think
of a case where somebody runs gitweb on a repository with refs/remotes and
does not want to show them.

> Typical usage would be for
> local repository browsing, e.g. by using git-instaweb (or even a more
> permanent gitweb setup), to check the repository status and the relation
> between tracking branches and the originating remotes.

When proofreading what you've written, it is usually a good idea to read
it without anything you wrote in parentheses once, and then re-read it
with parentheses removed (but the stuff in your parentheses kept), and
compare which one you like better.  More often than not, you'd find that
either parenthesized parts are unnecessary, or they are important enough
that you shouldn't put them in parentheses.

In this case, because you made it clear that you are giving just an
example and not trying to be exhaustive by saying "e.g.", I think dropping
the parenthesized part from the description is better.

Also I think the description is better without "to check...originating
remotes.", because:

 - "to check the repository status"?  what status?  it is too broad to be
   a meaningful description;

 - "relation between tracking vs origin" is one thing gitweb is very bad
   at doing, because it flattens the history, compared to things like
   gitk, which you need to compete with especially because you are
   advocating the feature to help local browsing.

> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index b0d00ea..e1f81f6 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -329,6 +329,18 @@ our %feature = (
> ...
> @@ -410,6 +422,18 @@ sub feature_pickaxe {
> ...
> +sub feature_remote_heads {
> ...
> +}

When would somebody want to disable this?  Please explain; I'd like to
understand the motivation behind it.

One argument for making this feature optional I can think of is to retain
backward compatibility because we didn't show them before, but I would say
that is a weak argument.  Before release 1.5.0 made the separate remotes
layout the default, everything was in refs/heads/, so you could even argue
that this "fixes" the gitweb bug introduced in that release that stopped
showing the branches you copied from elsewhere.

> @@ -2660,10 +2684,12 @@ sub git_get_heads_list {
>  	my $limit = shift;
>  	my @headslist;
>  
> +	my $remote_heads = gitweb_check_feature('remote_heads');
> +
>  	open my $fd, '-|', git_cmd(), 'for-each-ref',
>  		($limit ? '--count='.($limit+1) : ()), '--sort=-committerdate',
>  		'--format=%(objectname) %(refname) %(subject)%00%(committer)',
> -		'refs/heads'
> +		'refs/heads', ( $remote_heads ? 'refs/remotes' : '')
>  		or return;
>  	while (my $line = <$fd>) {
>  		my %ref_item;

Imagine a later version of git may introduce 'refs/frotz/nitfol' namespace
hierarchy that is commonly known as the 'xyzzy class' and is also useful
to show.  Wouldn't it be easier to update gitweb to match such a change if
this part of the code were written like this?

	my %head_class = ('refs/heads' => 'head');
	$head_class{'refs/remotes'} = 'remote'
	        if ( this feature is used );
	$head_class{'refs/frotz/nitfol'} = 'xyzzy'
	        if ( the xyzzy class is used);
        open my $fd, ... (keys %head_class);

> @@ -2674,8 +2700,9 @@ sub git_get_heads_list {
>  		my ($committer, $epoch, $tz) =
>  			($committerinfo =~ /^(.*) ([0-9]+) (.*)$/);
>  		$ref_item{'fullname'}  = $name;
> -		$name =~ s!^refs/heads/!!;
> +		$name =~ s!^refs/(head|remote)s/!!;
>  
> +		$ref_item{'class'} = $1;

And then outside the loop, you'd prepare:

	my $headpat = join('|', map { quotemeta($_) } keys %head_class);

and inside the loop you would do:

	if ($name =~ s{^($headpat)/}{}) {
        	$ref_item{'class'} = $head_class{$1};
		...

Only one place to configure the list of classes, and make everybody use
that list instead of hardcoding the assumption that there are two and only
two kinds of things "head" vs "remote".

Re: [PATCHv3 2/4] gitweb: git_get_heads_list accepts an optional list of refs.

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> git_get_heads_list(limit, class1, class2, ...) can now be used to retrieve
> refs/class1, refs/class2 etc. Defaults to ('heads') or ('heads', 'remotes')
> depending on the remote_heads option.
>
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.perl |   11 +++++++----
>  1 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index e1f81f6..0512020 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2681,15 +2681,18 @@ sub parse_from_to_diffinfo {
>  ## parse to array of hashes functions
>  
>  sub git_get_heads_list {
> -	my $limit = shift;
> +	my ($limit, @class) = @_;
> +	unless (defined @class) {
> +		my $remote_heads = gitweb_check_feature('remote_heads');
> +		@class = ('heads', $remote_heads ? 'remotes' : undef);
> +	}
> +	my @refs = map { "refs/$_" } @class;

Makes sense, except that I'd suggest passing a hash of "refs/$path" =>
$class as I illustrated in my comments to [1/4], instead of passing a list
of ("head", "remote"), because that will later allow you to have
multi-level $path that does not necessarily limited to a $class that is a
substring of $path, and doing so does not make the code any more complex.
There is another reason to do so I'll mention in I comment on [3/4].

Re: [PATCHv3 3/4] gitweb: separate heads and remotes lists

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> We specialize the 'heads' action to only display local branches, and
> introduce a 'remotes' action to display the remote branches (only
> available when the remotes_head feature is enabled).
>
> Mirroring this, we also split the heads list in summary view into
> local and remote lists, each linking to the appropriate action.
>
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.perl |   30 ++++++++++++++++++++++++++++--
>  1 files changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 0512020..6b09918 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -527,6 +527,7 @@ our %actions = (
>  	"heads" => \&git_heads,
>  	"history" => \&git_history,
>  	"log" => \&git_log,
> +	"remotes" => \&git_remotes,
>  	"rss" => \&git_rss,
>  	"atom" => \&git_atom,
>  	"search" => \&git_search,
> @@ -4467,6 +4468,7 @@ sub git_summary {
>  	my %co = parse_commit("HEAD");
>  	my %cd = %co ? parse_date($co{'committer_epoch'}, $co{'committer_tz'}) : ();
>  	my $head = $co{'id'};
> +	my $remote_heads = gitweb_check_feature('remote_heads');
>  
>  	my $owner = git_get_project_owner($project);
>  
> @@ -4474,7 +4476,8 @@ sub git_summary {
>  	# These get_*_list functions return one more to allow us to see if
>  	# there are more ...
>  	my @taglist  = git_get_tags_list(16);
> -	my @headlist = git_get_heads_list(16);
> +	my @headlist = git_get_heads_list(16, 'heads');
> +	my @remotelist = $remote_heads ? git_get_heads_list(16, 'remotes') : ();

Wasteful to run one for-each-ref for each list.

You earlier introduced $ref_item{'class'} so that you can differenciate
what you got from git_get_heads_list(); make use of it, perhaps like:

	my @heads_list = git_get_heads_list(16, \%head_class);
        my @headlist = grep { $_->{'class'} eq 'head' } @heads_list;
        my @remotelist = grep { $_->{'class'} eq 'remote' } @heads_list;

By the way, your [2/4] used "heads" and "remotes" as class, while your
[1/4] stored 'head' and 'remote' in $ref_item{'class'}.  Notice the above
suggestion corrects this discrepancy as well.

Re: [PATCHv3 1/4] gitweb: introduce remote_heads feature

[Giuseppe Bilotta]

On Sun, Nov 16, 2008 at 6:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> With this feature enabled, remotes are retrieved (and displayed)
>> when getting (and displaying) the heads list.
>
> Wouldn't it be easier to read if you just said: "Include 'remotes' in the
> heads_list", because:

[snip]

> I am also suggesting to drop "With this feature enabled"; I do not think
> of a case where somebody runs gitweb on a repository with refs/remotes and
> does not want to show them.

[snip]

> When proofreading what you've written, it is usually a good idea to read
> it without anything you wrote in parentheses once, and then re-read it
> with parentheses removed (but the stuff in your parentheses kept), and
> compare which one you like better.  More often than not, you'd find that
> either parenthesized parts are unnecessary, or they are important enough
> that you shouldn't put them in parentheses.

Good points, I'll rewrite the commit messagge following the suggestions.

>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index b0d00ea..e1f81f6 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -329,6 +329,18 @@ our %feature = (
>> ...
>> @@ -410,6 +422,18 @@ sub feature_pickaxe {
>> ...
>> +sub feature_remote_heads {
>> ...
>> +}
>
> When would somebody want to disable this?  Please explain; I'd like to
> understand the motivation behind it.
>
> One argument for making this feature optional I can think of is to retain
> backward compatibility because we didn't show them before, but I would say
> that is a weak argument.  Before release 1.5.0 made the separate remotes
> layout the default, everything was in refs/heads/, so you could even argue
> that this "fixes" the gitweb bug introduced in that release that stopped
> showing the branches you copied from elsewhere.

For example, because the only remote for the tree is the author's own
private tree, or because the only remotes are the mirrors on gitorious
or github. In both cases, there would be no reaso to waste resources,
bandwidth and screen estate loading and displaying the remote
references.

OTOH, it might make sense to make remote_heads enabled by default (and
overridable).

>> @@ -2660,10 +2684,12 @@ sub git_get_heads_list {
>>       my $limit = shift;
>>       my @headslist;
>>
>> +     my $remote_heads = gitweb_check_feature('remote_heads');
>> +
>>       open my $fd, '-|', git_cmd(), 'for-each-ref',
>>               ($limit ? '--count='.($limit+1) : ()), '--sort=-committerdate',
>>               '--format=%(objectname) %(refname) %(subject)%00%(committer)',
>> -             'refs/heads'
>> +             'refs/heads', ( $remote_heads ? 'refs/remotes' : '')
>>               or return;
>>       while (my $line = <$fd>) {
>>               my %ref_item;
>
> Imagine a later version of git may introduce 'refs/frotz/nitfol' namespace
> hierarchy that is commonly known as the 'xyzzy class' and is also useful
> to show.  Wouldn't it be easier to update gitweb to match such a change if
> this part of the code were written like this?
>
>        my %head_class = ('refs/heads' => 'head');
>        $head_class{'refs/remotes'} = 'remote'
>                if ( this feature is used );
>        $head_class{'refs/frotz/nitfol'} = 'xyzzy'
>                if ( the xyzzy class is used);
>        open my $fd, ... (keys %head_class);
>
>> @@ -2674,8 +2700,9 @@ sub git_get_heads_list {
>>               my ($committer, $epoch, $tz) =
>>                       ($committerinfo =~ /^(.*) ([0-9]+) (.*)$/);
>>               $ref_item{'fullname'}  = $name;
>> -             $name =~ s!^refs/heads/!!;
>> +             $name =~ s!^refs/(head|remote)s/!!;
>>
>> +             $ref_item{'class'} = $1;
>
> And then outside the loop, you'd prepare:
>
>        my $headpat = join('|', map { quotemeta($_) } keys %head_class);
>
> and inside the loop you would do:
>
>        if ($name =~ s{^($headpat)/}{}) {
>                $ref_item{'class'} = $head_class{$1};
>                ...
>
> Only one place to configure the list of classes, and make everybody use
> that list instead of hardcoding the assumption that there are two and only
> two kinds of things "head" vs "remote".

Ah, good point. This change, and the one about extending
git_get_heads_list, could also be turned into prelliminary patches to
the actual remote_heads feature.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv3 1/4] gitweb: introduce remote_heads feature

[Junio C Hamano]

"Giuseppe Bilotta" <giuseppe.bilotta@gmail.com> writes:

> For example, because the only remote for the tree is the author's own
> private tree, or because the only remotes are the mirrors on gitorious
> or github. In both cases, there would be no reaso to waste resources,
> bandwidth and screen estate loading and displaying the remote
> references.

Sorry, but you are not making sense.  The above may be a reason not to run
gitweb in such a repository, but if you are viewing such a private tree,
perhaps thru instaweb, wouldn't you be interested in viewing them?

> OTOH, it might make sense to make remote_heads enabled by default (and
> overridable).

Perhaps.  I'll stop commenting on "should this be optional, if so what
should be the default" and let others chime in.  Honestly I do not care
that deeply either way.

Re: [PATCHv3 1/4] gitweb: introduce remote_heads feature

[Giuseppe Bilotta]

On Sun, Nov 16, 2008 at 7:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
> "Giuseppe Bilotta" <giuseppe.bilotta@gmail.com> writes:
>
>> For example, because the only remote for the tree is the author's own
>> private tree, or because the only remotes are the mirrors on gitorious
>> or github. In both cases, there would be no reaso to waste resources,
>> bandwidth and screen estate loading and displaying the remote
>> references.
>
> Sorry, but you are not making sense.  The above may be a reason not to run
> gitweb in such a repository, but if you are viewing such a private tree,
> perhaps thru instaweb, wouldn't you be interested in viewing them?

No, I'm talking about the PUBLIC tree whose only remote is the private
one, or the gitorious/github mirrors, an example of the latter being
the rbot tree http://www.ruby-rbot.org/gitweb/rbot.git

>> OTOH, it might make sense to make remote_heads enabled by default (and
>> overridable).
>
> Perhaps.  I'll stop commenting on "should this be optional, if so what
> should be the default" and let others chime in.  Honestly I do not care
> that deeply either way.

Me either, actually. 8-D


-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv3 2/4] gitweb: git_get_heads_list accepts an optional list of refs.

[Jakub Narebski]

On Sun, 16 Nov 2008, Junio C Hamano wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
> 
> > git_get_heads_list(limit, class1, class2, ...) can now be used to retrieve
> > refs/class1, refs/class2 etc. Defaults to ('heads') or ('heads', 'remotes')
> > depending on the remote_heads option.
> >
> > Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> > ---
> >  gitweb/gitweb.perl |   11 +++++++----
> >  1 files changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index e1f81f6..0512020 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -2681,15 +2681,18 @@ sub parse_from_to_diffinfo {
> >  ## parse to array of hashes functions
> >  
> >  sub git_get_heads_list {
> > -	my $limit = shift;
> > +	my ($limit, @class) = @_;
> > +	unless (defined @class) {
> > +		my $remote_heads = gitweb_check_feature('remote_heads');
> > +		@class = ('heads', $remote_heads ? 'remotes' : undef);
> > +	}
> > +	my @refs = map { "refs/$_" } @class;
> 
> Makes sense, except that I'd suggest passing a hash of "refs/$path" =>
> $class as I illustrated in my comments to [1/4], instead of passing a list
> of ("head", "remote"), because that will later allow you to have
> multi-level $path that does not necessarily limited to a $class that is a
> substring of $path, and doing so does not make the code any more complex.
> There is another reason to do so I'll mention in I comment on [3/4].

By the way, with %head_class hash passed as reference git_get_head_list
could be done in such way, that you can write 

  git_get_heads_list(\%head_class);

instead of longer

  git_get_heads_list(undef, \%head_class);

when there is no limit(er).
-- 
Jakub Narebski
Poland

Re: [PATCHv3 3/4] gitweb: separate heads and remotes lists

[Giuseppe Bilotta]

On Sun, Nov 16, 2008 at 6:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>> -     my @headlist = git_get_heads_list(16);
>> +     my @headlist = git_get_heads_list(16, 'heads');
>> +     my @remotelist = $remote_heads ? git_get_heads_list(16, 'remotes') : ();
>
> Wasteful to run one for-each-ref for each list.
>
> You earlier introduced $ref_item{'class'} so that you can differenciate
> what you got from git_get_heads_list(); make use of it, perhaps like:
>
>        my @heads_list = git_get_heads_list(16, \%head_class);
>        my @headlist = grep { $_->{'class'} eq 'head' } @heads_list;
>        my @remotelist = grep { $_->{'class'} eq 'remote' } @heads_list;
>
> By the way, your [2/4] used "heads" and "remotes" as class, while your
> [1/4] stored 'head' and 'remote' in $ref_item{'class'}.  Notice the above
> suggestion corrects this discrepancy as well.

Although I agree with the head_class idea, I would like to point out
that doing a single git call to retrieve all refs and then selecting
the ones we want and doing multiple git calls are not equivalent.

First of all, with multiple calls we can limit the calls to retrieve
at most 16 refs of each kind, whereas this cannot be done with a
single call for all the refs. I'm not sure however, performance-wise,
if it's faster to filter the first 16 refs of each after retrieving
all of them, or doing multiple calls, especially with lots of refs.

Moreover, gitweb is already doing multiple for-each-ref calls to get
tags and heads. I guess that at this point we could go the extra mile
and include tags in the refs management. Of course, the problem of how
to handle the rate limiting remains.


-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv3 3/4] gitweb: separate heads and remotes lists

[Giuseppe Bilotta]

On Mon, Nov 17, 2008 at 2:11 PM, Giuseppe Bilotta
<giuseppe.bilotta@gmail.com> wrote:
> On Sun, Nov 16, 2008 at 6:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>>> -     my @headlist = git_get_heads_list(16);
>>> +     my @headlist = git_get_heads_list(16, 'heads');
>>> +     my @remotelist = $remote_heads ? git_get_heads_list(16, 'remotes') : ();
>>
>> Wasteful to run one for-each-ref for each list.
>>
>> You earlier introduced $ref_item{'class'} so that you can differenciate
>> what you got from git_get_heads_list(); make use of it, perhaps like:
>>
>>        my @heads_list = git_get_heads_list(16, \%head_class);
>>        my @headlist = grep { $_->{'class'} eq 'head' } @heads_list;
>>        my @remotelist = grep { $_->{'class'} eq 'remote' } @heads_list;
>>
>> By the way, your [2/4] used "heads" and "remotes" as class, while your
>> [1/4] stored 'head' and 'remote' in $ref_item{'class'}.  Notice the above
>> suggestion corrects this discrepancy as well.
>
> Although I agree with the head_class idea, I would like to point out
> that doing a single git call to retrieve all refs and then selecting
> the ones we want and doing multiple git calls are not equivalent.
>
> First of all, with multiple calls we can limit the calls to retrieve
> at most 16 refs of each kind, whereas this cannot be done with a
> single call for all the refs. I'm not sure however, performance-wise,
> if it's faster to filter the first 16 refs of each after retrieving
> all of them, or doing multiple calls, especially with lots of refs.
>
> Moreover, gitweb is already doing multiple for-each-ref calls to get
> tags and heads. I guess that at this point we could go the extra mile
> and include tags in the refs management. Of course, the problem of how
> to handle the rate limiting remains.

Sorry, replying to myself here. After more careful reading and some
more accurate testing, I gather that tags and heads management is
different enough to justify totally separate methods, and the cost of
an additional git call. It *could* still be merged (possibly with a
slightly different format string to accomodate both cases), but we
would have to do sorting ourselves, and such stuff. Not sure it's
worth it (yet).

My perplexity about rate limiting and separate calls for separate head
classes still remains, though.


-- 
Giuseppe "Oblomov" Bilotta