From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 10 Nov 2025 10:06:45 GMT
Subject: Re: [PATCH v2 07/12] diff: update the way rewrite diff handles incomplete lines
Message-ID: <aRG5NSvzeie_J3x9@pks.im>
In-Reply-To: <20251105213052.1499224-8-gitster@pobox.com>

On Wed, Nov 05, 2025 at 01:30:47PM -0800, Junio C Hamano wrote:
> The diff_symbol based output framework uses one DIFF_SYMBOL_* enum
> value per the kind of output lines of "git diff", which corresponds
> to one output line from the xdiff machinery used internally.  Most
> notably, DIFF_SYMBOL_PLUS and DIFF_SYMBOL_MINUS that correspond to
> "+" and "-" lines are designed to always take a complete line, even

"complete line" as in newline-terminated? I only recognized that this is
what you meant in the next paragraph, so it might be useful to clarify
here already what you mean.

> diff --git a/diff.c b/diff.c
> index 347cd9c6e9..99298720f4 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1786,22 +1777,36 @@ static void emit_rewrite_lines(struct emit_callback *ecbdata,
>  	const char *endp = NULL;
>  
>  	while (0 < size) {
> -		int len;
> +		int len, plen;
> +		char *pdata = NULL;
>  
>  		endp = memchr(data, '\n', size);
>  		len = endp ? (endp - data + 1) : size;
> +		plen = len;
> +
> +		if (!endp) {
> +			plen = len + 1;
> +			pdata = xmalloc(plen + 2);
> +			memcpy(pdata, data, len);
> +			pdata[len] = '\n';
> +			pdata[len + 1] = '\0';
> +		}
>  		if (prefix != '+') {
>  			ecbdata->lno_in_preimage++;
> -			emit_del_line(ecbdata, data, len);
> +			emit_del_line(ecbdata, pdata ? pdata : data, plen);
>  		} else {
>  			ecbdata->lno_in_postimage++;
> -			emit_add_line(ecbdata, data, len);
> +			emit_add_line(ecbdata, pdata ? pdata : data, plen);
>  		}
> +		free(pdata);
>  		size -= len;
>  		data += len;
>  	}
> -	if (!endp)
> -		emit_diff_symbol(ecbdata->opt, DIFF_SYMBOL_NO_LF_EOF, NULL, 0, 0);
> +	if (!endp) {
> +		static const char nneof[] = "\\ No newline at end of file\n";
> +		ecbdata->last_line_kind = prefix;
> +		emit_incomplete_line(ecbdata, nneof, sizeof(nneof) - 1);
> +	}
>  }

Okay. I was wondering at first how this would get executed for both
pre- and postimage if it's not part of the loop anymore. But this is
mostly showing my complete ignorance for the "diff" subsystem, as we end
up calling `emit_rewrite_lines()` itself once for each image.

Patrick