From: Jakub Narebski <jnareb@gmail.com>
Date: Sat, 27 Jun 2009 16:10:22 GMT
Subject: Re: [PATCHv7 2/9] gitweb: uniform author info for commit and commitdiff
Message-ID: <200906271810.23119.jnareb@gmail.com>
In-Reply-To: <1246104305-15191-3-git-send-email-giuseppe.bilotta@gmail.com>

On Sat, 27 June 2009, Giuseppe Bilotta wrote:
> Switch from
> 
> A U Thor <email@example.com> [date time]
> 
> to
> 
> author	A U Thor <email@example.com>
> 	date time
> committer	C O Mitter <other.email@example.com>
> 	committer date time

I would use:

  Switch from form similar to the one used by 'log' view

  	A U Thor <email@example.com> [date time]

  to the form used in 'commit' view

	author       A U Thor <email@example.com>
  	 	     date time
  	committer    C O Mitter <other.email@example.com>
  	             date time

(i.e. use spaces and not tabs to align).  But this is minor
issue, not worth worrying about IMVHO.

> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

I am still not sure about this change.  On one hand side it is unifying
of 'commit' and 'commitdiff' view; most other web interfaces have single
view equivalent to git-show, which displays both commit info, and the
diff.  (And 'commitdiff' with 'hp' parameter i.e. between two commits
has to be redesigned anyway, so this issue doesn't enter this 
consideration).

On the other hand side IIRC 'commitdiff' uses short (one-line) 
authorship info because the main point is the diff, and multi-line
author and commit info like the one used in 'commit' view takes
a bit of vertical space.  Also one can use similarity between
'log' and 'commitdiff' views (git-log and git-show) as a counter
for argument that 'commitdiff' has to look like 'commit'.

But otherwise I quite like this patch.

> ---
>  gitweb/gitweb.perl |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 9be723c..0d8005d 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -5599,7 +5599,11 @@ 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, 'localtime' => 1);
> +		print "<div class=\"title_text\">\n" .
> +		      "<table class=\"object_header\">\n";
> +		git_print_authorship_rows(\%co);
> +		print "</table>".
> +		      "</div>\n";
>  		print "<div class=\"page_body\">\n";
>  		if (@{$co{'comment'}} > 1) {
>  			print "<div class=\"log\">\n";

Nice and short, thanks to earlier (re)factoring.

BTW. after this change the -localtime part of git_print_authorship()
subroutine is unused... just saying ;-)  Not something terribly 
important.

-- 
Jakub Narebski
Poland