Re: [PATCHv6 1/8] gitweb: refactor author name insertion

[Giuseppe Bilotta <giuseppe.bilotta@gmail.com>]

2009/6/26 Jakub Narebski <jnareb@gmail.com>:
> Do I understand it correctly that this was meant as pure refactoring,
> i.e. that none of gitweb output should have changed?  Because you made
> a mistake, and 'log' view is broken (and it doesn't look like it did
> before).  See comments below for cause and (simple) solution.

Thanks for noticing. And yes, the problem is indeed that I forgot to
specify tag => span in git log view (I'm keeping div the default
because that's what was there in the first place in
git_print_authorship).

>> +# format the author name of the given commit with the given tag
>> +# the author name is chopped and escaped according to the other
>> +# optional parameters (see chop_str).
>> +sub format_author_html {
>> +     my $tag = shift;
>> +     my $co = shift;
>> +     my $author = chop_and_escape_str($co->{'author_name'}, @_);
>> +     return "<$tag class=\"author\">" . $author . "</$tag>\n";
>> +}
>
> Good... although I wonder if we should not get rid of chop_and_escape_str
> altogether, and for example add title attribute (if needed due to having
> to do shortening) directly to $tag, and not to inner <span> element.

This would require some additional refactoring, and I don't have a
clear idea on how to best implement it right now. I'm afraid it'll
have to wait for another time.

> Should "\n" be in returned string? Just asking.

You're right, we probably don't want to force the newline there. The
real question is, do we want the callers to put a newline there? I'm
thinking no, because it's mostly used in table cells and a newline is
better done at the row level, but I'm not sure either way. I'll just
remove it for the time being, it should only have effects at the
sourcecode level, not on the layout.

> Usually though we use %opts and not %params for the name of this
> hash, and we use CGI-like keys prefixed by '-', for example
> '-z' in parse_ls_tree_line(), '-nbsp' in esc_html, '-nohtml' in
> quot_cec(),  '-remove_title', '-remove_signoff' and '-final_empty_line'
> in git_print_log().  git_commitdiff() uses %params, but it doesn't
> have non-optional parameters (still, I guess we should use %opts
> for consistency), and it uses '-format' and '-single' as names.
>
> href() subroutine uses %params... but those are not extra named
> optional parameters to subroutine; they are CGI query parameters.

I'll adjust the code accordingly. BTW the %params in git_commitdiff is
my fault too, IIRC.

>>       my %ad = parse_date($co->{'author_epoch'}, $co->{'author_tz'});
>> -     print "<div class=\"author_date\">" .
>> +     print "<$tag class=\"author_date\">" .
>>             esc_html($co->{'author_name'}) .
>>             " [$ad{'rfc2822'}";
>> +     if ($params{'localtime'}) {
>> +             if ($ad{'hour_local'} < 6) {
>> +                     printf(" (<span class=\"atnight\">%02d:%02d</span> %s)",
>> +                            $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
>> +             } else {
>> +                     printf(" (%02d:%02d %s)",
>> +                            $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'});
>> +             }
>> +     }
>> +     print "]</$tag>\n";
>> +}
>
> Gaah, git has chosen to show this diff a bit strangely...

Oh, very funny indeed. I hadn't realized it went that way. Wonder if
the patience diff would have helped here.

> By the way, what about author / tagger info used in 'tag' view?

I totally forgot about that.

> Wouldn't it be better to factor out generating table rows for single
> author / committer / tagger header (field) info?

Good idea. I'm not sure the tagger field has all the relevant data. I'll check.

> I'd rather use here (as mentioned in comment about git_print_full_authorship
> subroutine) something like the following:
>
> +       git_print_authorship_header(\%co, 'author');
> +       git_print_authorship_header(\%co, 'committer');
>
> Or something like that.  But this might be a matter of taste.

I renamed the sub to git_print_authorship_rows, and I'm making it
accept a list of people to print info for. I'll make it default to
both author and committer though.


-- 
Giuseppe "Oblomov" Bilotta