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

[Junio C Hamano <gitster@pobox.com>]

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