Re: [PATCHv7 1/9] gitweb: refactor author name insertion
From: Jakub Narebski <hidden>
Date: 2016-06-15 22:46:59
On Sat, 27 June 2009, Giuseppe Bilotta wrote:
Collect all author display code in appropriate functions, making it easier to extend these functions on the CGI side. We also move some of the presentation code from hard-coded HTML to CSS, for easier customization.
Very nicely written, compact yet comprehensive commit message. It _might_ be worth adding that accidentally commit date got "atnight" warning too. I consider that improvement, even though for local commit (committer == author) it duplicates information. And it removes (some of) code duplication.
Signed-off-by: Giuseppe Bilotta <redacted>
If not for the fact that due to typo you lost localtime info from 'commitdiff' view, I would ACK it.
--- gitweb/gitweb.css | 5 ++- gitweb/gitweb.perl | 96 +++++++++++++++++++++++++++++++-------------------- 2 files changed, 62 insertions(+), 39 deletions(-)
[...]
quoted hunk ↗ jump to hunk
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 1e7e2d8..9be723c 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl@@ -1469,6 +1469,16 @@ sub format_subject_html { } } +# 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>"; +}
In the future we might want to change it so instead of <$tag class="author"><span title="Joe Random Hacker">Joe Random... </span></$tag> we would use 'title' attribute of $tag element <$tag class="author" title="Joe Random Hacker">Joe Random... </$tag> But I guess this is not worth complicating code required to do this.
quoted hunk ↗ jump to hunk
+ # format git diff header line, i.e. "diff --(git|combined|cc) ..." sub format_git_diff_header_line { my $line = shift;@@ -3214,21 +3224,53 @@ sub git_print_header_div { "\n</div>\n"; } +# Outputs the author name and date in long form sub git_print_authorship { my $co = shift; + my %opts = @_; + my $tag = $opts{-tag} || 'div';
BTW. gitweb is a bit inconsistent here. Sometimes we use $opts{'-tag'}
form, and sometimes $opts{-tag} form like you use here.
(Actually we use '-' prefixing i.e. '-key' not 'key' to make it safe
to write $opts{-key} and -key => 1).
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 ($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'});
+ if ($opts{-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'});
+ }
+ }By the way, this "atnight" warning is duplicated in subroutine below git_print_authorship_rows(), but IMHO it is not something very important. It can be left for later.
+ print "]</$tag>\n";
+}
+
+# Outputs table rows containing the full author or committer information,
+# in the format expected for 'commit' view (& similia).
+# Parameters are a commit hash reference, followed by the list of people
+# to output information for. If the list is empty it defalts to both
+# author and committer.
+sub git_print_authorship_rows {
+ my $co = shift;
+ # too bad we can't use @people = @_ || ('author', 'committer')
+ my @people = @_;
+ @people = ('author', 'committer') unless @people;
+ foreach my $who (@people) {
+ my %ad = parse_date($co->{$who . '_epoch'}, $co->{$who . '_tz'});
Wouldn't
$co->{"${who}_epoch"}
be simpler than
$co->{$who . '_epoch'}
Also it is now %wd (%who_date) or just %date rather than
%ad (%author_date) vs %cd (%committer_date).
Both of those issues are very, very minor.
+ print "<tr><td>$who</td><td>" . esc_html($co->{$who}) . "</td></tr>\n".
+ "<tr>" .
+ "<td></td><td> $ad{'rfc2822'}";+ 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'});
+ }This is a bit of code duplication with git_print_authorship(), but I don't think it is something seriously to worry about.
+ print "</td>" . + "</tr>\n"; }
- print "]</div>\n";
Heh. Strange quirks of diff algorithm... :-)
quoted hunk ↗ jump to hunk
} sub git_print_page_path {@@ -4142,11 +4184,9 @@ sub git_shortlog_body { print "<tr class=\"light\">\n"; } $alternate ^= 1; - my $author = chop_and_escape_str($co{'author_name'}, 10); # git_summary() used print "<td><i>$co{'age_string'}</i></td>\n" . print "<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n" . - "<td><i>" . $author . "</i></td>\n" . - "<td>"; + format_author_html('td', \%co, 10) . "<td>"; print format_subject_html($co{'title'}, $co{'title_short'}, href(action=>"commit", hash=>$commit), $ref); print "</td>\n" .
Nice.
quoted hunk ↗ jump to hunk
@@ -4193,11 +4233,9 @@ sub git_history_body { print "<tr class=\"light\">\n"; } $alternate ^= 1; - # shortlog uses chop_str($co{'author_name'}, 10) - my $author = chop_and_escape_str($co{'author_name'}, 15, 3); print "<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n" . - "<td><i>" . $author . "</i></td>\n" . - "<td>"; + # shortlog: format_author_html('td', \%co, 10) + format_author_html('td', \%co, 15, 3) . "<td>"; # originally git_history used chop_str($co{'title'}, 50) print format_subject_html($co{'title'}, $co{'title_short'}, href(action=>"commit", hash=>$commit), $ref);
Nice. Good, well aligned comment.
quoted hunk ↗ jump to hunk
@@ -4350,9 +4388,8 @@ sub git_search_grep_body { print "<tr class=\"light\">\n"; } $alternate ^= 1; - my $author = chop_and_escape_str($co{'author_name'}, 15, 5); print "<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n" . - "<td><i>" . $author . "</i></td>\n" . + format_author_html('td', \%co, 15, 5) . "<td>" . $cgi->a({-href => href(action=>"commit", hash=>$co{'id'}), -class => "list subject"},
Nice... although this set of diff chunks made me wonder why we use "10" for 'shortlog', "15, 3" for 'history, and "15, 5" for 'search' (grep).
quoted hunk ↗ jump to hunk
@@ -5094,9 +5131,9 @@ sub git_log { " | " . $cgi->a({-href => href(action=>"tree", hash=>$commit, hash_base=>$commit)}, "tree") . "<br/>\n" . - "</div>\n" . - "<i>" . esc_html($co{'author_name'}) . " [$ad{'rfc2822'}]</i><br/>\n" . "</div>\n"; + git_print_authorship(\%co, -tag => 'span'); + print "<br/>\n</div>\n"; print "<div class=\"log_body\">\n"; git_print_log($co{'comment'}, -final_empty_line=> 1);
This preserves layout. Good. I see you choose to correct mentioned issue this way.
quoted hunk ↗ jump to hunk
@@ -5115,8 +5152,6 @@ sub git_commit { $hash ||= $hash_base || "HEAD"; my %co = parse_commit($hash) or die_error(404, "Unknown commit object"); - my %ad = parse_date($co{'author_epoch'}, $co{'author_tz'}); - my %cd = parse_date($co{'committer_epoch'}, $co{'committer_tz'}); my $parent = $co{'parent'}; my $parents = $co{'parents'}; # listref@@ -5183,22 +5218,7 @@ sub git_commit { } print "<div class=\"title_text\">\n" . "<table class=\"object_header\">\n"; - print "<tr><td>author</td><td>" . esc_html($co{'author'}) . "</td></tr>\n". - "<tr>" . - "<td></td><td> $ad{'rfc2822'}"; - 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 "</td>" . - "</tr>\n"; - print "<tr><td>committer</td><td>" . esc_html($co{'committer'}) . "</td></tr>\n"; - print "<tr><td></td><td> $cd{'rfc2822'}" . - sprintf(" (%02d:%02d %s)", $cd{'hour_local'}, $cd{'minute_local'}, $cd{'tz_local'}) . - "</td></tr>\n"; + git_print_authorship_rows(\%co); print "<tr><td>commit</td><td class=\"sha1\">$co{'id'}</td></tr>\n"; print "<tr>" . "<td>tree</td>" .
Nice simplification. Even without making 'commitdiff' use the same authorship info layout like 'commit', and 'tag' reusing this subroutine it might have been worth it.
quoted hunk ↗ jump to hunk
@@ -5579,7 +5599,7 @@ sub git_commitdiff { git_header_html(undef, $expires); git_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash, $formats_nav); git_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash); - git_print_authorship(\%co); + git_print_authorship(\%co, 'localtime' => 1); print "<div class=\"page_body\">\n"; if (@{$co{'comment'}} > 1) { print "<div class=\"log\">\n";
ATTENTION !!!
You should have
- git_print_authorship(\%co);
+ git_print_authorship(\%co, '-localtime' => 1);
here ('-localtime' instead of 'localtime'), otherwise you will loose
localtime information (and "atnight" warning) from 'commitdiff' view.
Thanks again for your diligent work on this patch series!
--
Jakub Narebski
Poland