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

From: Stefan Beller <sbeller@google.com>
Date: Jun 21, 2017, 19:46 UTC
Message-ID: <CAGZ79kYUpJxX-wBU=ALPgJwVaA8h_iJRtAu3T7p4J7qmy=U4dg@mail.gmail.com>
In-Reply-To: <xmqq7f05p1z7.fsf@gitster.mtv.corp.google.com>

On Wed, Jun 21, 2017 at 12:36 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 19 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  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.

Show 15 quoted lines

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