2010/9/26 Jakub Narebski <jnareb@xxxxxxxxx>:
> 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?
Yes, this is exactly the intended purpose of this feature. It is of
course possible to extend other views to have a similar behavior (I'm
thinking in particular about log views here).
> 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.
I will clarify this in the next rehash of the patch.
>> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@xxxxxxxxx>
>> ---
>> 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.
I will add the dash to the option. Naming it header_extra keeps the
meaning of this extra text generic, but considering that the intended
use is mostly for the single-remote view (or similar, if/when they are
added) we could call it something related (I can only think of
'main_argument' right now but I think this would suck more than
header_extra).
>> + $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.
Ok, I will remove it.
>> + }
>> + print " / $action_print";
>> + }
>> + if (defined $opts{'header_extra'}) {
>> + print " / $opts{'header_extra'}";
>
> Hmmm...
You don't sound very convinced. I had some doubts myself about whether
the slash should be inserted autmatically or whether it should be up
to the caller to include it in header_extra, but I'm not sure this is
what you are perplexed about.
--
Giuseppe "Oblomov" Bilotta
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html