From: Stefan Sperling <stsp@stsp.name>
Subject: Re: diff.git: Fix ed script output
To: Tom Jones <thj@freebsd.org>
Cc: gameoftrees@openbsd.org
Date: Tue, 6 Sep 2022 16:46:45 +0200

On Tue, Sep 06, 2022 at 01:33:37PM +0100, Tom Jones wrote:
> The rc check is happening in the look twice, once for diff_output_lines
> and once for the fprintf, so it doesn't need to happen here again.
> 
> I have removed that and made the check for printf to be < 0.

This patch looks very good to me.

However, it breaks a regression test. Can you reproduce this failure?
And, if so, do you have time to figure out why it breaks?

$ make -C test
[...]
cmp: EOF on expect123.diff
FAIL: verify.expect123.diff != expect123.diff
OK: arraylist_test
OK: results_test

Tests failed:
FAIL: verify.expect123.diff != expect123.diff
*** Error 1 in /home/stsp/src/diff/test (Makefile:7 'verify')
$

> diff --git a/lib/diff_output_edscript.c b/lib/diff_output_edscript.c
> index 42d4d5b..e7c8353 100644
> --- a/lib/diff_output_edscript.c
> +++ b/lib/diff_output_edscript.c
> @@ -32,9 +32,9 @@ static int
>  output_edscript_chunk(struct diff_output_info *outinfo,
>      FILE *dest, const struct diff_input_info *info,
>      const struct diff_result *result,
> -    struct diff_chunk_context *cc)
> +    struct diff_chunk_context *cc, off_t *outoff)
>  {
> -	off_t outoff = 0, *offp;
> +	off_t *offp;
>  	int left_start, left_len, right_start, right_len;
>  	int rc;
>  
> @@ -81,17 +81,10 @@ output_edscript_chunk(struct diff_output_info *outinfo,
>  		}
>  	} else {
>  		/* change */
> -		if (left_len == 1 && right_len == 1) {
> -			rc = fprintf(dest, "%dc%d\n", left_start, right_start);
> -		} else if (left_len == 1) {
> -			rc = fprintf(dest, "%dc%d,%d\n", left_start,
> -			    right_start, cc->right.end);
> -		} else if (right_len == 1) {
> -			rc = fprintf(dest, "%d,%dc%d\n", left_start,
> -			    cc->left.end, right_start);
> +		if (left_len == 1) {
> +			rc = fprintf(dest, "%dc\n", left_start);
>  		} else {
> -			rc = fprintf(dest, "%d,%dc%d,%d\n", left_start,
> -			    cc->left.end, right_start, cc->right.end);
> +			rc = fprintf(dest, "%d,%dc\n", left_start, cc->left.end);
>  		}
>  	}
>  	if (rc < 0)
> @@ -100,8 +93,39 @@ output_edscript_chunk(struct diff_output_info *outinfo,
>  		ARRAYLIST_ADD(offp, outinfo->line_offsets);
>  		if (offp == NULL)
>  			return ENOMEM;
> -		outoff += rc;
> -		*offp = outoff;
> +		*outoff += rc;
> +		*offp = *outoff;
> +	}
> +
> +	/* Now write out the new lines in all the joined chunks. */
> +	int c_idx;
> +	for (c_idx = cc->chunk.start; c_idx < cc->chunk.end; c_idx++) {
> +		const struct diff_chunk *c = &result->chunks.head[c_idx];
> +		if (c->left_count && !c->right_count)
> +				continue;
> +		if (c->right_count && !c->left_count) {
> +			rc = diff_output_lines(outinfo, dest,
> +					  c->solved ? "" : "?",
> +					  c->right_start, c->right_count);
> +			if (rc != DIFF_RC_OK)
> +				return rc;
> +
> +			rc = fprintf(dest, ".\n");
> +			if (rc < 0)
> +				return errno;
> +			if (outinfo) {
> +				ARRAYLIST_ADD(offp, outinfo->line_offsets);
> +				if (offp == NULL)
> +					return ENOMEM;
> +				outoff += rc;
> +				*offp = *outoff;
> +			}
> +		}
> +		if (cc->chunk.end == result->chunks.len) {
> +			rc = diff_output_trailing_newline_msg(outinfo, dest, c);
> +			if (rc != DIFF_RC_OK)
> +				return rc;
> +		}
>  	}
>  
>  	return DIFF_RC_OK;
> @@ -121,6 +145,7 @@ diff_output_edscript(struct diff_output_info **output_info,
>  	bool force_text = (flags & DIFF_FLAG_FORCE_TEXT_DATA);
>  	bool have_binary = (atomizer_flags & DIFF_ATOMIZER_FOUND_BINARY_DATA);
>  	int i, rc;
> +	off_t outoff = 0, *offp;
>  
>  	if (!result)
>  		return EINVAL;
> @@ -151,7 +176,11 @@ diff_output_edscript(struct diff_output_info **output_info,
>  		return DIFF_RC_OK;
>  	}
>  
> -	for (i = 0; i < result->chunks.len; i++) {
> +	/*
> +	 * Changes in an ed script are in reverse order, the last change to the
> +	 * file has to be made first to keep state while making edits.
> +	 */
> +	for (i = result->chunks.len-1; i >= 0 ; i--) {
>  		struct diff_chunk *chunk = &result->chunks.head[i];
>  		enum diff_chunk_type t = diff_chunk_type(chunk);
>  		struct diff_chunk_context next;
> @@ -178,13 +207,15 @@ diff_output_edscript(struct diff_output_info **output_info,
>  			continue;
>  		}
>  
> -		rc = output_edscript_chunk(outinfo, dest, info, result, &cc);
> +		rc = output_edscript_chunk(outinfo, dest, info, result, &cc,
> +			&outoff);
>  		if (rc != DIFF_RC_OK)
>  			return rc;
>  		cc = next;
>  	}
>  
>  	if (!diff_chunk_context_empty(&cc))
> -		return output_edscript_chunk(outinfo, dest, info, result, &cc);
> +		return output_edscript_chunk(outinfo, dest, info, result, &cc,
> +			&outoff);
>  	return DIFF_RC_OK;
>  }