Re: [PATCH 4/8] gitweb: Push formatting diff lines to print_diff_chunk()

["Michał Kiedrowicz" <michal.kiedrowicz@gmail.com>]

Jakub Narebski <jnareb@gmail.com> wrote:

> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
> 
> > Now git_patchset_body() only calls diff_line_class() (removed from
> > process_diff_line()). The latter function is renamed to
> > format_diff_line() and is called from print_diff_chunk().
> > 
> > This is a pure code movement, needed for processing raw diff lines in
> > the accumulator in print_diff_chunk(). No behavior change is intended by
> > this change.
> 
> Well, this is not "pure code movement" per se; it is meant to be
> refactoring that doesn't change gitweb output nor behavior.
> 
> If I understand correctly the change is from
> 
>   read
>   format
>   accumulate
>   print
>  
> to
> 
>   read
>   accumulate
>   format
>   print
> 
> Isn't it?

Yeah, this is what I meant :).

> 
> As a note I would add also that process_diff_line got renamed to
> format_diff_line, 

I thought I wrote that ("The latter function is renamed...")?

> and its output changed to returning only
> HTML-formatted line, which bringg it in line with other format_*
> subroutines.
> 

OK.

> > Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
> 
> I think it is a good change even without subsequent patches.
> 
>   Acked-by: Jakub Narębski <jnareb@gmail.com>
> 

Thanks.

> > ---
> >  gitweb/gitweb.perl |   25 ++++++++++++-------------
> >  1 files changed, 12 insertions(+), 13 deletions(-)
> > 
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index d2f75c4..cae9dfa 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -2320,12 +2320,9 @@ sub format_cc_diff_chunk_header {
> >  }
> >  
> >  # process patch (diff) line (not to be used for diff headers),
> > -# returning class and HTML-formatted (but not wrapped) line
> > -sub process_diff_line {
> > -	my $line = shift;
> > -	my ($from, $to) = @_;
> > -
> > -	my $diff_class = diff_line_class($line, $from, $to);
> > +# returning HTML-formatted (but not wrapped) line
> > +sub format_diff_line {
> > +	my ($line, $diff_class, $from, $to) = @_;
> >  
> >  	chomp $line;
> >  	$line = untabify($line);
> > @@ -2343,7 +2340,7 @@ sub process_diff_line {
> >  	$diff_classes .= " $diff_class" if ($diff_class);
> >  	$line = "<div class=\"$diff_classes\">$line</div>\n";
> >  
> > -	return $diff_class, $line;
> > +	return $line;
> >  }
> >  
> >  # Generates undef or something like "_snapshot_" or "snapshot (_tbz2_ _zip_)",
> > @@ -4934,7 +4931,7 @@ sub print_diff_lines {
> >  }
> >  
> >  sub print_diff_chunk {
> > -	my ($diff_style, $is_combined, @chunk) = @_;
> > +	my ($diff_style, $is_combined, $from, $to, @chunk) = @_;
> >  	my (@ctx, @rem, @add);
> >  	my $prev_class = '';
> >  
> > @@ -4954,6 +4951,8 @@ sub print_diff_chunk {
> >  	foreach my $line_info (@chunk) {
> >  		my ($class, $line) = @$line_info;
> >  
> > +		$line = format_diff_line($line, $class, $from, $to);
> > +
> >  		# print chunk headers
> >  		if ($class && $class eq 'chunk_header') {
> >  			print $line;
> > @@ -5107,19 +5106,19 @@ sub git_patchset_body {
> >  
> >  			next PATCH if ($patch_line =~ m/^diff /);
> >  
> > -			my ($class, $line) = process_diff_line($patch_line, \%from, \%to);
> > +			my $class = diff_line_class($patch_line, \%from, \%to);
> >  
> >  			if ($class eq 'chunk_header') {
> > -				print_diff_chunk($diff_style, $is_combined, @chunk);
> > -				@chunk = ( [ $class, $line ] );
> > +				print_diff_chunk($diff_style, $is_combined, \%from, \%to, @chunk);
> > +				@chunk = ( [ $class, $patch_line ] );
> >  			} else {
> > -				push @chunk, [ $class, $line ];
> > +				push @chunk, [ $class, $patch_line ];
> >  			}
> >  		}
> >  
> >  	} continue {
> >  		if (@chunk) {
> > -			print_diff_chunk($diff_style, $is_combined, @chunk);
> > +			print_diff_chunk($diff_style, $is_combined, \%from, \%to, @chunk);
> >  			@chunk = ();
> >  		}
> >  		print "</div>\n"; # class="patch"
> > -- 
> > 1.7.3.4
> > 
> 
> Nice!
>