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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> After running an external merge driver, ll_ext_merge() reads the result
> back from a temporary file. We can do the same thing with much less code
> by using read_mmfile().
>
> As a bonus, note that read_mmfile() correctly uses xsize_t() to detect
> the case when we'd truncate the result.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  merge-ll.c | 21 +++++----------------
>  1 file changed, 5 insertions(+), 16 deletions(-)
>
> diff --git a/merge-ll.c b/merge-ll.c
> index dfed6411a8..7fab7c5438 100644
> --- a/merge-ll.c
> +++ b/merge-ll.c
> @@ -201,8 +201,7 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
>  	struct strbuf cmd = STRBUF_INIT;
>  	const char *format = fn->cmdline;
>  	struct child_process child = CHILD_PROCESS_INIT;
> -	int status, fd, i;
> -	struct stat st;
> +	int status, i;
>  	enum ll_merge_result ret;
>  	assert(opts);
>  
> @@ -241,20 +240,10 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
>  	child.use_shell = 1;
>  	strvec_push(&child.args, cmd.buf);
>  	status = run_command(&child);
> -	fd = open(temp[1], O_RDONLY);
> -	if (fd < 0)
> -		goto bad;
> -	if (fstat(fd, &st))
> -		goto close_bad;
> -	result->size = st.st_size;
> -	result->ptr = xmallocz(result->size);
> -	if (read_in_full(fd, result->ptr, result->size) != result->size) {
> -		FREE_AND_NULL(result->ptr);
> -		result->size = 0;
> -	}
> - close_bad:
> -	close(fd);
> - bad:
> +
> +	/* We can ignore errors; result is left NULL/0 in that case. */
> +	read_mmfile(result, temp[1]);
> +
>  	for (i = 0; i < 3; i++)
>  		unlink_or_warn(temp[i]);
>  	strbuf_release(&cmd);

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