Re: [PATCH 03/19] diff.c: drop 'nofirst' from emit_line_0

From: Stefan Beller <sbeller@google.com>
Date: May 15, 2017, 18:33 UTC
Message-ID: <CAGZ79kZSixaG2CoO-WV47=-eE96yhtoCm82KFbZ=-=8c-VPXcA@mail.gmail.com>
In-Reply-To: <115f4d30-5c97-9ee9-e56b-3cf955a5454e@google.com>

On Mon, May 15, 2017 at 11:26 AM, Jonathan Tan <jonathantanmy@google.com> wrote:

> "erroneous"?

yep, words are hard.

>
> I also don't understand the meaning of this paragraph - if you mean that
> this patch teaches other callers to hardcode the sign, I don't see any such
> changes in the diff below.

The last two hunks of the patch switch two callers that call with a sign that is hard to reason about.

Show 16 quoted lines

> After reading the patch below, would this commit message be better:
>
> [begin]
> diff.c: teach emit_line_0 to accept sign parameter
>
> Instead of a separate "first" parameter representing the first character of
> the line to be printed, make emit_line_0 take an optional "sign" parameter
> specifically intended to hold the sign of the line. Callers that store the
> sign and line separately can use the "sign" parameter like they used the
> "first" parameter previously, and callers that store the sign and line
> together (or do not have a sign) no longer need to manipulate their
> arguments to fit the requirements of emit_line_0.
>
> (And then mention that you have checked all the callers and that none of
> them send '\n' or '\r' as the sign, as you have done in this version.)
> [end]

That describes the situation better, indeed.

Show 13 quoted lines

>> @@ -556,7 +547,7 @@ static void emit_line_0(struct diff_options *o, const
>> char *set, const char *res
>>  static void emit_line(struct diff_options *o, const char *set, const char
>> *reset,
>>                       const char *line, int len)
>>  {
>> -       emit_line_0(o, set, reset, line[0], line+1, len-1);
>> +       emit_line_0(o, set, reset, 0, line, len);
>>  }
>
>
> Maybe this function is unnecessary now that emit_line_0 can take the line
> directly.

That would produce a lot of code churn, specifically in later patches; but I can remove that function if anyone feels strongly about it.

Show 10 quoted lines

>> +                       char term[2];
>> +                       term[0] = options->line_termination;
>> +                       term[1] = '\0';
>> +
>> +                       emit_line(options, NULL, NULL,
>> +                                 term, 1);
>
>
> If options->line_termination is 0, this is actually a zero-length string
> (not 1).

So passing in !!options->line_termination should be fine?

Thanks, Stefan