Re: [PATCHv3 04/20] diff.c: teach emit_line_0 to accept sign parameter
- Date: Thu, 18 May 2017 16:33:07 -0700
- From: Jonathan Tan <jonathantanmy@xxxxxxxxxx>
- Subject: Re: [PATCHv3 04/20] diff.c: teach emit_line_0 to accept sign parameter
On Thu, 18 May 2017 12:37:30 -0700
Stefan Beller <sbeller@xxxxxxxxxx> wrote:
> Teach emit_line_0 take a "sign" parameter specifically intended
> to hold the sign of the line instead of a separate "first" parameter
> representing the first character of the line to be printed. 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
I know I suggested the paragraph above, but after rereading your patch
set, I think I finally understand what you're trying to accomplish.
I think it's better to combine patches 4/20, 5/20, and 6/20, with the
following commit message:
diff: introduce more flexible emit function
Currently, diff output is written either through the emit_line_0
function or through the FILE * in struct diff_options directly. To
make it easier to teach diff to buffer its output (which will be done
in a subsequent commit), introduce a more flexible emit() function. In
this commit, direct usages of emit_line_0() are replaced with emit();
subsequent commits will also replace usages of the FILE * with emit().
And the function itself can be documented this way (with the appropriate
Emits the following line or part of line. It is expected that "set"
and "reset", if not NULL, should contain terminal color codes (or
markup denoting color) and that "sign", if not 0, should contain '+',
'-', or ' '. But those are not requirements.
If you do all that, then the buffering patch (19/20) can be improved by
adding this comment somewhere in the file:
Buffer the diff output into ??? instead of immediately writing it to
NEEDSWORK: The contents of the ??? array - in particular, how the diff
output is divided into array elements - is not precisely defined; some
functions may emit a line all at once (resulting in one element)
whereas some others may emit a line piecemeal (resulting in more than
one element). Ideally, the code in this file should be structured so
that we do not have such imprecision, but in the meantime, callers
that request buffering should ensure that the diff output is divided
the way they expect (and have tests to ensure that it remains so).
> With this patch other callers hard code the sign (which are '+', '-',
> ' ' and '\\') such that we do not run into unexpectedly emitting an
> erroneous '\0'.
I still don't understand this paragraph - can you rewrite this in the
> The audit of the caller revealed that the sign cannot be '\n' or '\r',
> so remove that condition for trailing newline or carriage return in
> the sign; the else part of the condition handles the len==0 perfectly,
> so we can drop the if/else construct.
> Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
> diff.c | 39 ++++++++++++++++-----------------------
> 1 file changed, 16 insertions(+), 23 deletions(-)