Jakub Narebski <jnareb@xxxxxxxxx> wrote:
> Michal Kiedrowicz wrote:
> > Jakub Narebski <jnareb@xxxxxxxxx> wrote:
> >> Junio C Hamano wrote:
> >>> Michał Kiedrowicz <michal.kiedrowicz@xxxxxxxxx> writes:
>
> >>>> - # 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.
>
> No, I think that this change is good idea if it simplifies code flow.
> But it really should be described in commit message, not only "what"
> (which you did describe), but also "whys".
>
Sure, I'll try to put my explanation to the commit message.
--
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