From: Tom Jones <thj@freebsd.org>
Subject: Re: diff.git: Fix ed script output
To: gameoftrees@openbsd.org
Date: Tue, 6 Sep 2022 13:33:37 +0100

On Tue, Sep 06, 2022 at 11:00:13AM +0200, Stefan Sperling wrote:
> On Tue, Sep 06, 2022 at 09:38:59AM +0100, Tom Jones wrote:
> > +			rc = fprintf(dest, ".\n");
> > +			if (rc != DIFF_RC_OK)
> > +				return rc;
> 
> DIFF_RC_OK is not a valid return code of fprintf.
> 
> > +			if (outinfo) {
> > +				ARRAYLIST_ADD(offp, outinfo->line_offsets);
> > +				if (offp == NULL)
> > +					return ENOMEM;
> > +				outoff += rc;
> > +				*offp = *outoff;
> > +			}
> > +		}
> > +		if (rc)
> > +			return rc;
> 
> The check for success (rc < 0 means error) should happen earlier, before
> the new line offset is computed based on the return code (amount of bytes
> written to the file if not < 0).

You are right, sorry for that. 

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.

- Tom
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;
 }