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

[Jonathan Tan <jonathantanmy@google.com>]

On 05/13/2017 09:01 PM, Stefan Beller wrote:
> In 250f79930d (diff.c: split emit_line() from the first char and the rest
> of the line, 2009-09-14) we introduced the local variable 'nofirst' that
> indicates if we have no first sign character. With the given implementation
> we had to use an extra variable unlike reusing 'first' because the lines
> first character could be '\0'.
>
> Change the meaning of the 'first' argument to not mean the first character
> of the line, but rather just containing the sign that is prepended to the
> line. Refactor emit_line to not include the lines first character, but pass
> the complete line as well as a '\0' sign, which now serves as an indication
> not to print a sign.
>
> With this patch other callers hard code the sign (which are '+', '-',
> ' ' and '\\') such that we do not run into unexpectedly emitting an
> error-nous '\0'.

"erroneous"?

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.

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]

>
> 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 | 40 +++++++++++++++++-----------------------
>  1 file changed, 17 insertions(+), 23 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index c2ed605cd0..4269b8dccf 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -517,33 +517,24 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,
>  }
>
>  static void emit_line_0(struct diff_options *o, const char *set, const char *reset,
> -			int first, const char *line, int len)
> +			int sign, const char *line, int len)
>  {
>  	int has_trailing_newline, has_trailing_carriage_return;
> -	int nofirst;
>  	FILE *file = o->file;
>
>  	fputs(diff_line_prefix(o), file);
>
> -	if (len == 0) {
> -		has_trailing_newline = (first == '\n');
> -		has_trailing_carriage_return = (!has_trailing_newline &&
> -						(first == '\r'));
> -		nofirst = has_trailing_newline || has_trailing_carriage_return;
> -	} else {
> -		has_trailing_newline = (len > 0 && line[len-1] == '\n');
> -		if (has_trailing_newline)
> -			len--;
> -		has_trailing_carriage_return = (len > 0 && line[len-1] == '\r');
> -		if (has_trailing_carriage_return)
> -			len--;
> -		nofirst = 0;
> -	}
> +	has_trailing_newline = (len > 0 && line[len-1] == '\n');
> +	if (has_trailing_newline)
> +		len--;
> +	has_trailing_carriage_return = (len > 0 && line[len-1] == '\r');
> +	if (has_trailing_carriage_return)
> +		len--;
>
> -	if (len || !nofirst) {
> +	if (len || sign) {
>  		fputs(set, file);
> -		if (!nofirst)
> -			fputc(first, file);
> +		if (sign)
> +			fputc(sign, file);
>  		fwrite(line, len, 1, file);
>  		fputs(reset, file);
>  	}
> @@ -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.

>
>  static int new_blank_line_at_eof(struct emit_callback *ecbdata, const char *line, int len)
> @@ -4822,9 +4813,12 @@ void diff_flush(struct diff_options *options)
>
>  	if (output_format & DIFF_FORMAT_PATCH) {
>  		if (separator) {
> -			fprintf(options->file, "%s%c",
> -				diff_line_prefix(options),
> -				options->line_termination);
> +			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).

>  			if (options->stat_sep) {
>  				/* attach patch instead of inline */
>  				fputs(options->stat_sep, options->file);
>