From: Junio C Hamano Date: Tue, 29 Sep 2026 19:22:39 GMT Subject: Re: [PATCH 4/5] merge-ll: use read_mmfile() to read external merge results Message-ID: In-Reply-To: <20260929065442.GD1697497@coredump.intra.peff.net> Jeff King 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 > --- > 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 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