Re: [PATCHv5 06/12] gitweb: allow extra text after action in page header
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Sep 26, 2010, 18:11 UTC
- Message-ID
- <201009262011.28211.jnareb@gmail.com>
- In-Reply-To
- <1285344167-8518-7-git-send-email-giuseppe.bilotta@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.
Show 17 quoted lines
>
> 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.
Show 5 quoted lines
> + }
> + print " / $action_print";
> + }
> + if (defined $opts{'header_extra'}) {
> + print " / $opts{'header_extra'}";Hmmm...
Show 7 quoted lines
> } > print "\n"; > } > -- > 1.7.3.68.g6ec8 > >
-- Jakub Narebski Poland