git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCHv7 1/9] gitweb: refactor author name insertion

From
Jakub Narebski <jnareb@gmail.com>
Date
Jun 27, 2009, 14:24 UTC
Message-ID
<200906271624.52372.jnareb@gmail.com>
In-Reply-To
<1246104305-15191-2-git-send-email-giuseppe.bilotta@gmail.com>
On Sat, 27 June 2009, Giuseppe Bilotta wrote:
Show 5 quoted lines
> 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 <giuseppe.bilotta@gmail.com>

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(-)
[...]
Show 17 quoted lines
> 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.
Show 13 quoted lines
> +
>  # 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).

Show 21 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 ($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.

Show 15 quoted lines
> +	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'}";
Show 7 quoted lines
> +		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... :-)
Show 16 quoted lines
>  }
>  
>  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.
Show 14 quoted lines
> @@ -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.
Show 11 quoted lines
> @@ -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).

Show 12 quoted lines
> @@ -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.
Show 33 quoted lines
> @@ -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.

Show 9 quoted lines
> @@ -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
Previous: Giuseppe BilottaNext: Giuseppe Bilotta
Message 27 of 31 in “[PATCHv7 0/9] gitweb: avatar support”
  1. Giuseppe BilottaJun 27, 2009
  2. 1/9 gitweb: refactor author name insertionGiuseppe Bilotta, Jun 27, 2009
  3. 2/9 gitweb: uniform author info for commit and commitdiffGiuseppe Bilotta, Jun 27, 2009
  4. 3/9 gitweb: use git_print_authorship_rows in 'tag' view tooGiuseppe Bilotta, Jun 27, 2009
  5. 4/9 gitweb: right-align date cell in shortlogGiuseppe Bilotta, Jun 27, 2009
  6. 5/9 gitweb: (gr)avatar supportGiuseppe Bilotta, Jun 27, 2009
  7. 6/9 gitweb: gravatar url cacheGiuseppe Bilotta, Jun 27, 2009
  8. 7/9 gitweb: picon avatar providerGiuseppe Bilotta, Jun 27, 2009
  9. 8/9 gitweb: use picon for gravatar fallbackGiuseppe Bilotta, Jun 27, 2009
  10. 9/9 gitweb: add alt text to avatar imgGiuseppe Bilotta, Jun 27, 2009
  11. Jakub NarebskiJun 28, 2009
  12. Giuseppe BilottaJun 28, 2009
  13. Jakub NarebskiJun 28, 2009
  14. Jakub NarebskiJun 28, 2009
  15. Giuseppe BilottaJun 28, 2009
  16. Jakub NarebskiJun 27, 2009
  17. Jakub NarebskiJun 27, 2009
  18. Giuseppe BilottaJun 27, 2009
  19. Jakub NarebskiJun 27, 2009
  20. Jakub NarebskiJun 27, 2009
  21. Giuseppe BilottaJun 27, 2009
  22. Jakub NarebskiJun 27, 2009
  23. Giuseppe BilottaJun 27, 2009
  24. Jakub NarebskiJun 27, 2009
  25. Junio C HamanoJun 27, 2009
  26. Giuseppe BilottaJun 27, 2009
  27. Jakub NarebskiJun 27, 2009
  28. Giuseppe BilottaJun 27, 2009
  29. Giuseppe BilottaJun 27, 2009
  30. Jakub NarebskiJun 29, 2009
  31. Giuseppe BilottaJun 29, 2009

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.