Re: [PATCH 4/5] merge-ll: use read_mmfile() to read external merge results
flat view
From: Patrick Steinhardt <hidden>
Date: 2026-09-30 15:33:09
On Tue, Sep 29, 2026 at 02:54:42AM -0400, Jeff King wrote:
quoted hunk ↗ jump to hunk
diff --git a/merge-ll.c b/merge-ll.c index dfed6411a8..7fab7c5438 100644 --- a/merge-ll.c +++ b/merge-ll.c@@ -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);
So we do lose the NUL-termination that `xmallocz()` gave us, as `read_mmfile()` doesn't do that. You reinstate that in the last patch though, which makes me lean more into the direction of having that last optional patch. If so though, we may want to reorder it to come first.
quoted hunk ↗ jump to hunk
- 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]);
One change in behaviour that wasn't called out is that this will now make us write an error message in case we failed reading the file. That could be a good change, but that's hard to say. Patrick