Jakub Narebski <jnareb@xxxxxxxxx> wrote:
> Junio C Hamano wrote:
> > Michał Kiedrowicz <michal.kiedrowicz@xxxxxxxxx> writes:
> >
> > > + if (!@$add) {
> > > + # pure removal
> > > +...
> > > + } elsif (!@$rem) {
> > > + # pure addition
> > > +...
> > > + } else {
> > > + # assume that it is change
> > > + print join '',
> >
> > I know this is not a new problem, but if your patch hunk has both
> > '-' and '+' lines, what's there to "assume" that it is a change?
> > Isn't it always?
>
> What I meant here when I was writing it that they are lines that
> changed between two versions, like '!' in original (not unified)
> context format.
>
> We can omit this comment.
OK.
>
> > > - # empty add/rem block on start context block, or
> > > end of chunk
> > > - if ((@rem || @add) && (!$class || $class eq
> > > 'ctx')) { -...
> > > + ## print from accumulator when have some add/rem
> > > lines or end
> > > + # of chunk (flush context lines)
> > > + if (((@rem || @add) && $class eq 'ctx')
> > > || !$class) {
> >
> > This seems to change the condition. Earlier, it held true if
> > (there is anything to show), and (class is unset or equal to ctx).
> > The new code says something different.
>
> Yes it does, as described in the commit message:
>
> [...] It should
> not change the gitweb output, but it **slightly changes its
> behavior**. Before this commit, context is printed on the class
> change. Now, it's printed just before printing added and removed
> lines, and at the end of chunk.
>
> The difference is that context lines are also printed accumulated now.
> Though why this change is required for refactoring could have been
> described in more detail...
I changed that because I wanted to squash both conditions (the one that
checks if @ctx should be printed and the one that prints @add/@rem
lines) and have just one call to print_sidebyside_diff_lines(). Later,
this function is changed to print_diff_lines() and controls whether
'inline' or 'side-by-side' diff should be printed. Having two
conditions and two calls/functions would make the code redundant. Then
I thought that instead of calling twice print_sidebyside_diff_lines()
(for @ctx and @add/@rem lines, like the code from pre-image prints
these lines separatedly), I can just call it once.
I can revert this change to previous behavior but I think that would
make the condition more complicated.
>
> > Also can $class be undef, and if so,
> > doesn't it trigger comparison between undef and 'ctx' by
> > having !$class check at the end of || chain?
>
> Thanks for noticing this (I wonder why testsuite didn't caught it).
> It should be
>
> + ## print from accumulator when have some add/rem
> lines or end
> + # of chunk (flush context lines)
> + if (!$class || ((@rem || @add) && $class eq 'ctx'))
> {
>
OK, I'll fix that.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html