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

From: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
Date: Jun 25, 2009, 23:41 UTC
Message-ID: <cb7bb73a0906251641g71250105ob608b557cce7c454@mail.gmail.com>
In-Reply-To: <200906260055.11347.jnareb@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).

Show 13 quoted lines

>> +# 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.

Show 10 quoted lines

> 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.

Show 18 quoted lines

>>       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.

Show 7 quoted lines

> 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