Re: [PATCH 3/3] gitweb: A bit of code cleanup in git_blame()

[Luben Tuikov <ltuikov@yahoo.com>]

--- On Tue, 12/9/08, Jakub Narebski <jnareb@gmail.com> wrote:
> From: Jakub Narebski <jnareb@gmail.com>
> Subject: [PATCH 3/3] gitweb: A bit of code cleanup in git_blame()
> To: git@vger.kernel.org
> Cc: "Luben Tuikov" <ltuikov@yahoo.com>, "Jakub Narebski" <jnareb@gmail.com>
> Date: Tuesday, December 9, 2008, 2:48 PM
> Among others: 
>  * move variable declaration closer to the place it is set
> and used,
>    if possible,
>  * uniquify and simplify coding style a bit, which includes
> removing
>    unnecessary '()'.
>  * check type only if $hash was defined, as otherwise from
> the way
>    git_get_hash_by_path() is called (and works), we know
> that it is
>    a blob,
>  * use modern calling convention for git-blame,
>  * remove unused variable,
>  * don't use implicit variables ($_),
>  * add some comments
> 
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
Acked-by: Luben Tuikov <ltuikov@yahoo.com>

Looks good.

> ---
> Not stricly necessary... but the code looked not very nice
> 
>  gitweb/gitweb.perl |   65
> ++++++++++++++++++++++++++++++----------------------
>  1 files changed, 37 insertions(+), 28 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 916396a..68aa3f8 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -4575,28 +4575,32 @@ sub git_tag {
>  }
>  
>  sub git_blame {
> -	my $fd;
> -	my $ftype;
> -
> +	# permissions
>  	gitweb_check_feature('blame')
> -	    or die_error(403, "Blame view not
> allowed");
> +		or die_error(403, "Blame view not allowed");
>  
> +	# error checking
>  	die_error(400, "No file name given") unless
> $file_name;
>  	$hash_base ||= git_get_head_hash($project);
> -	die_error(404, "Couldn't find base commit")
> unless ($hash_base);
> +	die_error(404, "Couldn't find base commit")
> unless $hash_base;
>  	my %co = parse_commit($hash_base)
>  		or die_error(404, "Commit not found");
>  	if (!defined $hash) {
>  		$hash = git_get_hash_by_path($hash_base, $file_name,
> "blob")
>  			or die_error(404, "Error looking up file");
> +	} else {
> +		my $ftype = git_get_type($hash);
> +		if ($ftype !~ "blob") {
> +			die_error(400, "Object is not a blob");
> +		}
>  	}
> -	$ftype = git_get_type($hash);
> -	if ($ftype !~ "blob") {
> -		die_error(400, "Object is not a blob");
> -	}
> -	open ($fd, "-|", git_cmd(), "blame",
> '-p', '--',
> -	      $file_name, $hash_base)
> +
> +	# run git-blame --porcelain
> +	open my $fd, "-|", git_cmd(),
> "blame", '-p',
> +		$hash_base, '--', $file_name
>  		or die_error(500, "Open git-blame failed");
> +
> +	# page header
>  	git_header_html();
>  	my $formats_nav =
>  		$cgi->a({-href =>
> href(action=>"blob", -replay=>1)},
> @@ -4610,40 +4614,43 @@ sub git_blame {
>  	git_print_page_nav('','',
> $hash_base,$co{'tree'},$hash_base, $formats_nav);
>  	git_print_header_div('commit',
> esc_html($co{'title'}), $hash_base);
>  	git_print_page_path($file_name, $ftype, $hash_base);
> -	my @rev_color = (qw(light2 dark2));
> +
> +	# page body
> +	my @rev_color = qw(light2 dark2);
>  	my $num_colors = scalar(@rev_color);
>  	my $current_color = 0;
> -	my $last_rev;
> +	my %metainfo = ();
> +
>  	print <<HTML;
>  <div class="page_body">
>  <table class="blame">
> 
> <tr><th>Commit</th><th>Line</th><th>Data</th></tr>
>  HTML
> -	my %metainfo = ();
> -	while (1) {
> -		$_ = <$fd>;
> -		last unless defined $_;
> + LINE:
> +	while (my $line = <$fd>) {
> +		chomp $line;
> +		# the header: <SHA-1> <src lineno> <dst
> lineno> [<lines in group>]
> +		# no <lines in group> for subsequent lines in
> group of lines
>  		my ($full_rev, $orig_lineno, $lineno, $group_size) =
> -		    /^([0-9a-f]{40}) (\d+) (\d+)(?:
> (\d+))?$/;
> +		   ($line =~ /^([0-9a-f]{40}) (\d+) (\d+)(?:
> (\d+))?$/);
>  		if (!exists $metainfo{$full_rev}) {
>  			$metainfo{$full_rev} = {};
>  		}
>  		my $meta = $metainfo{$full_rev};
> -		while (<$fd>) {
> -			last if (s/^\t//);
> -			if (/^(\S+) (.*)$/) {
> +		while (my $data = <$fd>) {
> +			chomp $data;
> +			last if ($data =~ s/^\t//); # contents of line
> +			if ($data =~ /^(\S+) (.*)$/) {
>  				$meta->{$1} = $2;
>  			}
>  		}
> -		my $data = $_;
> -		chomp $data;
> -		my $rev = substr($full_rev, 0, 8);
> +		my $short_rev = substr($full_rev, 0, 8);
>  		my $author = $meta->{'author'};
> -		my %date = parse_date($meta->{'author-time'},
> -		                      $meta->{'author-tz'});
> +		my %date =
> +			parse_date($meta->{'author-time'},
> $meta->{'author-tz'});
>  		my $date = $date{'iso-tz'};
>  		if ($group_size) {
> -			$current_color = ++$current_color % $num_colors;
> +			$current_color = ($current_color + 1) % $num_colors;
>  		}
>  		print "<tr id=\"l$lineno\"
> class=\"$rev_color[$current_color]\">\n";
>  		if ($group_size) {
> @@ -4654,7 +4661,7 @@ HTML
>  			print $cgi->a({-href =>
> href(action=>"commit",
>  			                             hash=>$full_rev,
>  			                            
> file_name=>$file_name)},
> -			              esc_html($rev));
> +			              esc_html($short_rev));
>  			print "</td>\n";
>  		}
>  		my $parent_commit;
> @@ -4683,6 +4690,8 @@ HTML
>  	print "</div>";
>  	close $fd
>  		or print "Reading blob failed\n";
> +
> +	# page footer
>  	git_footer_html();
>  }