Re: [PATCH 5/8] gitweb: Format diff lines just before printing

[Jakub Narebski <jnareb@gmail.com>]

Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:

> Now we're ready to insert highlightning to diff output.
> 
> The call to untabify() remains in the main loop in print_diff_chunk().
> The motivation is that it must be called before any call to esc_html()
> (because that converts spaces to &nbsp;) and to call it only once.
> 
> This is a refactoring patch.  It's not meant to change gitweb output.

I'm not sure about this patch.

First, in my opinion it doesn't make much sense standalone, and not
squashed with subsequent patch.

Second, it makes format_diff_line an odd duck among all other format_*
subroutines in that it post-processes HTML output, rather than
generating HTML from data.
 
Why "diff refinement highlighting" cannot be part of format_diff_line()?
If it does need additional data, just pass it as additional parameters
to this subroutine.

Another solution could be to borrow idea from stalled and inactive
committags feature, namely that parts that are HTML are to be passed
as scalar reference (\$str), while plain text (unescaped yet) is to be
passed as string ($str).

> -# process patch (diff) line (not to be used for diff headers),
> -# returning HTML-formatted (but not wrapped) line
> +# wrap patch (diff) line into a <div> (not to be used for diff headers),
> +# the line must be esc_html()'ed
>  sub format_diff_line {

I just don't like this error-prone "the line must be esc_html()'ed".

> +# HTML-format diff context, removed and added lines.
> +sub format_ctx_rem_add_lines {
> +	my ($ctx, $rem, $add) = @_;
> +	my (@new_ctx, @new_rem, @new_add);
> +
> +	@new_ctx = map { format_diff_line(esc_html($_, -nbsp=>1), 'ctx') } @$ctx;
> +	@new_rem = map { format_diff_line(esc_html($_, -nbsp=>1), 'rem') } @$rem;
> +	@new_add = map { format_diff_line(esc_html($_, -nbsp=>1), 'add') } @$add;
> +
> +	return (\@new_ctx, \@new_rem, \@new_add);
> +}
> +
>  # Print context lines and then rem/add lines.
>  sub print_diff_lines {
>  	my ($ctx, $rem, $add, $diff_style, $is_combined) = @_;
>  
> +	($ctx, $rem, $add) = format_ctx_rem_add_lines($ctx, $rem, $add);
> +

Nice trick.

-- 
Jakub Narębski