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 [off-list ref] 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
emit_line_0.

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
formatting):

  /*
  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
  "file". 

  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
imperative tense?
 

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 <redacted>
---
 diff.c | 39 ++++++++++++++++-----------------------
 1 file changed, 16 insertions(+), 23 deletions(-)

[snip]