Subject: Re: [PATCH 04/26] diff.c: introduce emit_diff_symbol

From: Brandon Williams <[email-masked]>
Date: Wed, 21 Jun 2017 14:45:51 -0700

On 06/21, Stefan Beller wrote:
> On Wed, Jun 21, 2017 at 12:36 PM, Junio C Hamano <[email-masked]> wrote:
> > Stefan Beller <[email-masked]> writes:
> >
> >> Signed-off-by: Stefan Beller <[email-masked]>
> >> ---
> >>  diff.c | 22 +++++++++++++++++++---
> >>  1 file changed, 19 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/diff.c b/diff.c
> >> index 2f9722b382..89466018e5 100644
> >> --- a/diff.c
> >> +++ b/diff.c
> >> @@ -559,6 +559,24 @@ static void emit_line(struct diff_options *o, const char *set, const char *reset
> >>       emit_line_0(o, set, reset, line[0], line+1, len-1);
> >>  }
> >>
> >> +enum diff_symbol {
> >> +     DIFF_SYMBOL_SEPARATOR,
> >
> > Drop the last comma from enum?
> 
> I looked through out code base and for enums this is
> actually strictly enforced, so I guess I have to play
> by the rules here as I do not want to be the first
> to deviate from an upheld standard.
> 
> This will be painful though as the next ~20 patches
> add more symbols mostly at the end, maybe I need
> to restructure that such that the last symbol stays the same
> throughout the series. Thanks for that thought.

I don't think this is strictly enforced.  If you look at grep.h:197 the
enum 'grep_source_type' has a trailing comma.

> 
> >
> >> +static void emit_diff_symbol(struct diff_options *o, enum diff_symbol s,
> >> +                          const char *line, int len)
> >> +{
> >> +     switch (s) {
> >> +     case DIFF_SYMBOL_SEPARATOR:
> >> +             fprintf(o->file, "%s%c",
> >> +                     diff_line_prefix(o),
> >> +                     o->line_termination);
> >> +             break;
> >
> > As the first patch in the "diff-symbol" subseries of this topic,
> > this change must seriously be justified.  Why is it so important
> > that a printing of an empty line must be moved to a helper function,
> > which later will gain ability to show other kind of lines?
> 
> Ah yes. This got lost in comparison to the currently queued series with
> diff_lines. The justification for the change was in the buffer patch,
> but now we need to have the justification here.
> 
> In the old series, I had copied the same text in all these
> refactoring patches, but thought to delete them in this series. The first
> refactoring patch makes sense though.
> 
> Thanks,
> Stefan

-- 
Brandon Williams