On 06/21, Stefan Beller wrote:
> On Wed, Jun 21, 2017 at 12:36 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> > Stefan Beller <sbeller@xxxxxxxxxx> writes:
> >
> >> Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
> >> ---
> >> 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
