Re: [PATCH 4/5] merge-ll: use read_mmfile() to read external merge results
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 29, 2026, 19:22 UTC
- Message-ID
- <xmqqzewzg8w0.fsf@gitster.g>
- In-Reply-To
- <20260929065442.GD1697497@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 51 quoted lines
> 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