Re: [PATCH 4/5] merge-ll: use read_mmfile() to read external merge results

Jeff King [off-list ref] writes:

Lets see if I understand why we can safely ignore errors.

If the external driver claims that it successfully merged (i.e.,
status = run_command(&child) returns 0), and yet read_mmfile() fails
(e.g., perhaps the driver unlinks "%A"), read_mmfile() will leave
result->ptr and result->size as initialized, and we return
LL_MERGE_OK from this function.  The result is eventually relayed to
the caller of ll_merge(), like merge-ort.c:merge_3way(), or
apply.c:three_way_merge().  Both have something like

	status = ll_merge(&result, path,
			  &base_file, "base",
			  &our_file, "ours",
			  &their_file, "theirs",
			  state->repo->index,
			  &merge_opts);
	if (status == LL_MERGE_BINARY_CONFLICT)
		warning("Cannot merge binary files: %s (%s vs. %s)",
			path, "ours", "theirs");
	free(base_file.ptr);
	free(our_file.ptr);
	free(their_file.ptr);
	if (status < 0 || !result.ptr) {
		free(result.ptr);
		return -1;
	}

to treat that result.ptr==NULL is just as bad as any error from
ll_merge() (i.e., status < 0).

merge-blobs.c:merge_blobs() does not check the !result.ptr
condition, and its sole caller builtin/merge-tree.c:result() passes
the NULL to show_diff(), which uses a <NULL, 0> mmfile_t as one side
of xdi_diff(), which the callee is prepared to handle, so this is OK.

rerere.c:try_merge() does not check the !result.ptr condition, and
its caller rerere.c:merge() ends up calling

	fwrite(NULL, (size_t)0, 1, f)

which may happen to work on most systems, but is not exactly kosher.

Perhaps something like this on top might make it safer?  Not even
compile tested and I haven't thought through the ramifications to
rerere.c:merge() code path, that used to take such a bogus merge
result as successful merge and relied on the fwrite(NULL) becoming
a no-op to produce an empty file.

 merge-ll.c | 16 +++++++++++++---
 merge-ll.h |  4 ++++
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git c/merge-ll.c w/merge-ll.c
index 7fab7c5438..518c05636f 100644
--- c/merge-ll.c
+++ w/merge-ll.c

@@ -405,6 +405,7 @@ enum ll_merge_result ll_merge(mmfile_t *result_buf,
 	const char *ll_driver_name = NULL;
 	int marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
 	const struct ll_merge_driver *driver;
+	enum ll_merge_result result;
 
 	if (!opts)
 		opts = &default_opts;

@@ -434,9 +435,18 @@ enum ll_merge_result ll_merge(mmfile_t *result_buf,
 	if (opts->extra_marker_size) {
 		marker_size += opts->extra_marker_size;
 	}
-	return driver->fn(driver, result_buf, path, ancestor, ancestor_label,
-			  ours, our_label, theirs, their_label,
-			  opts, marker_size);
+	result = driver->fn(driver, result_buf, path, ancestor, ancestor_label,
+			    ours, our_label, theirs, their_label,
+			    opts, marker_size);
+	if (!result_buf.ptr && result == LL_MERGE_OK) {
+		/*
+		 * Forbid the driver from giving bogus result and claim
+		 * that the merge succeeded.
+		 */
+		result = LL_MERGE_ERROR;
+		result_buf.size = 0;
+	}
+	return result;
 }
 
 int ll_merge_marker_size(struct index_state *istate, const char *path)

diff --git c/merge-ll.h w/merge-ll.h
index f95332c682..c5e11f397e 100644
--- c/merge-ll.h
+++ w/merge-ll.h

@@ -23,6 +23,10 @@
  *
  * - Call `ll_merge()`.
  *
+ * - Notice a merge error by checking return value from ll_merge().  If the
+ *   .ptr member of the result is NULL, that may indicate that we failed to
+ *   read the merge results from an external merge driver.
+ *
  * - Read the merged content from `result_buf.ptr` and `result_buf.size`.
  *
  * - Release buffers when finished.  A simple