Re: [PATCHv5 07/12] gitweb: remotes view for a single remote

[Jakub Narebski <jnareb@gmail.com>]

On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:

> If the hash parameter is passed to gitweb, remotes will interpret it as
> the name of a remote and limit the view the the heads of that remote.

Errr... I think this commit message needs rewriting to be more clear.
Perhaps:

  When 'remotes' view is passed 'hash' parameter, it would interprete it
  as the name of a remote ...

> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.perl |   25 ++++++++++++++++++++-----
>  1 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 76cf806..7c62701 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -5547,13 +5547,28 @@ sub git_remotes {
>  		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,format_ref_views('remotes'));
> +	my $remote = $input_params{'hash'};

I am not sure about using 'hash' parameter for that.

On one hand it is a hack that allow us to not worry about adding extra
code to evaluate_path_info() subroutine, so that natural path_info URL
of http://git.example.com/repo.git/remotes/<remote> would use <remote>
as name of remote to limit to.

On the other hand it is abusing semantic of 'hash' parameter.  Remote
name is not revision name or object id.  

What makes this issue stronger is the fact that URL is part of API,
and if we make mistake here, we would have to maintain backward 
compatibility (at least if it appears in a released version).

> +
> +	git_header_html(undef, undef, 'header_extra' => $remote);

I don't quite like the name of this parameter, and I am not sure
if I like the API either.

> +	git_print_page_nav('', '',  $head, undef, $head,
> +		format_ref_views($remote ? '' : 'remotes'));

Why this change?

>  	git_print_header_div('summary', $project);
>  
> -	my @remotelist = git_get_heads_list(undef, 'remotes');
> -	if (@remotelist) {
> -		git_heads_body(\@remotelist, $head);
> +	if (defined $remote) {
> +		# only display the heads in a given remote
> +		my @headslist = map {
> +			my $ref = $_ ;
> +			$ref->{'name'} =~ s!^$remote/!!;
> +			$ref
> +		} git_get_heads_list(undef, "remotes/$remote");

Hmmm... do we need this temporary variable?  Does it make anything
more clear?

> +		if (@headslist) {
> +			git_heads_body(\@headslist, $head);
> +		}

This part is the same (modulo name of variable) in both branches of this
conditional.

> +	} else {
> +		my @remotelist = git_get_heads_list(undef, 'remotes');
> +		if (@remotelist) {
> +			git_heads_body(\@remotelist, $head);
> +		}
>  	}
>  	git_footer_html();
>  }
> -- 
> 1.7.3.68.g6ec8
> 
> 

-- 
Jakub Narebski
Poland