Re: [PATCHv3 04/20] diff.c: teach emit_line_0 to accept sign parameter

From: Jonathan Tan <jonathantanmy@google.com>
Date: May 18, 2017, 23:33 UTC
Message-ID: <20170518163307.657caedc@twelve2.svl.corp.google.com>
In-Reply-To: <20170518193746.486-5-sbeller@google.com>

On Thu, 18 May 2017 12:37:30 -0700 Stefan Beller <sbeller@google.com> wrote:

Show 8 quoted lines

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

Show 9 quoted lines

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

[snip]