Re: [PATCH 1/4] diff.c: emit_line_0 to take string instead of first sign

From: Stefan Beller <sbeller@google.com>
Date: Aug 13, 2018, 18:19 UTC
Message-ID: <CAGZ79kY8-GEWjK5TjkAzv1VoyLLXBD9t-B48UwPcWkogvD0MFA@mail.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.1808131340420.71@tvgsbejvaqbjf.bet>

On Mon, Aug 13, 2018 at 4:42 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 9 quoted lines

> > +/*
> > + * Emits
> > + * <set_sign> <first> <reset> <set> <second> <reset> LF
> > + * if they are present. 'first' is a NULL terminated string,
> > + * 'second' is a buffer of length 'len'.
> > + */
>
> That does not make it clear what the role of `first` or `second` is. Could
> you clarify that?

For now it is just "first string" and "second string", where the first is used for signs and indicators, and the second is allowed to have '\0' in it as we give the length as a parameter. This doc tried to be neutral w.r.t. the purpose of the first/second string.

> (TBH I am not so sure myself what roles they serve. Previously, it was
> kind of obvious to me that `first` tried to specify the diff marker, if
> any. But now...?)

Originally I thought we'd split the line into

  "++" "line"

for a range diff, but this is not the case.

As the next patch introduces configurable strings, we'd need this patch. I consider changing the next patch to allow only giving a single character instead, such that we can keep 'first', which indicates the sign character. (So maybe I'd even call it 'sign').