From: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
Date: Sat, 11 Feb 2012 23:38:21 GMT
Subject: Re: [PATCH 5/8] gitweb: Format diff lines just before printing
Message-ID: <20120212003821.276f2c5a@gmail.com>
In-Reply-To: <m37gztthrx.fsf@localhost.localdomain>

Jakub Narebski <jnareb@gmail.com> wrote:

> 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.

True. I wanted to separate "preparation" from "adding new feature" but
maybe went few steps too far.

> 
> 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).

I'll look into it.

> 
> > -# 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.
>