Re: [PATCHv5 06/12] gitweb: allow extra text after action in page header

[Jakub Narebski <jnareb@gmail.com>]

On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:

> This extra text is intended to 'specify' the action. Therefore, if it's
> present, the action name in the header will be turned into a link to
> the action itself but without specifying any parameter.

What this feature is intended *for*?  I guess it is meant to be used
in the case where there is additional parameter that specifies action,
like e.g. a single remote view, where $action is 'remotes', but there
is extra parameter ('hash_base' is (ab)used for this) that specifies
remote.  Then we want to make 'remotes' in "breadcrumbs" navigation at
top of page to link to generic 'remotes' view.  Isn't it?

But the above is just my guess, covering only case where there is both
$action defined, and 'header_extra' option set.

You need to explain this in the commit message.

> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.perl |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index e70897e..76cf806 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3514,7 +3514,15 @@ EOF
>  	if (defined $project) {
>  		print $cgi->a({-href => href(action=>"summary")}, esc_html($project));
>  		if (defined $action) {
> -			print " / $action";
> +			my $action_print = $action ;
> +			if (defined $opts{'header_extra'}) {

We spell optional named parameter with '-' as prefix, for example
$opts{'-remove_signoff'} in git_print_log(), to be able to use key
without quoting (bareword-like, autoquoting), like $opts{-nohtml}
or $opts{-pad_before}, or $opts{-size}, or $opts{-tag}... though
gitweb is not very consisnte here.

So it should probably be

  +			if (defined $opts{'-header_extra'}) {

or even

  +			if (defined $opts{-header_extra}) {


I also think that we can think of better name for this option than
'header_extra', although what this name could be eludes me.

> +				$action_print = $cgi->a({-href => href(action=>$action)},
> +					esc_html($action));

I don't think we need to run esc_html on action: it is checked that it
is one of specified set of possible values, and it can be ensured that
it neither contains anything than printable characters, not any HTML
special characters.

> +			}
> +			print " / $action_print";
> +		}
> +		if (defined $opts{'header_extra'}) {
> +			print " / $opts{'header_extra'}";

Hmmm...

>  		}
>  		print "\n";
>  	}
> -- 
> 1.7.3.68.g6ec8
> 
> 

-- 
Jakub Narebski
Poland