Hi Justin
On 12/02/2025 04:18, Justin Tobler wrote:
The diffs queued from git-diff-pairs(1) stdin are not flushed EOF is
reached. To enable greater flexibility, allow control over when the diff
queue is flushed by writing a single nul byte on stdin between input
file pairs. Diff output between flushes is separated by a single line
terminator.
I agree with the comments others have made about the documentation. I
also have some comments on the implementation below.
diff --git a/builtin/diff-pairs.c b/builtin/diff-pairs.c
index 08f3ee81e5..2436ce3013 100644
--- a/builtin/diff-pairs.c
+++ b/builtin/diff-pairs.c
@@ -99,6 +99,17 @@ int cmd_diff_pairs(int argc, const char **argv, const char *prefix,
break;
p = meta.buf;
+ if (!*p) {
+ flush_diff_queue(&revs.diffopt);
+ /*
+ * When the diff queue is explicitly flushed, append an
+ * additional terminator to separate batches of diffs.
+ */
+ fprintf(revs.diffopt.file, "%c",
+ revs.diffopt.line_termination);
As the user has requested an explicit flush we should call
fflush(stdout) here to avoid deadlocking a caller that is waiting to
read the terminator before writing the next batch of input. Ideally the
tests would check that the output is flushed but I think that is quite
hard to do with our test framework.
I think it would be easier for callers to parse the output if we always
printed NUL here. Programming languages generally have a function that
allows you to read all the input until a specific byte is seen. If
flushing always used a NUL terminator the caller could use their
equivalent of read_until(b'\0') to hoover up the output (using '-z' to
do this would change the output of --numstat and embed a NUL between any
stat data and the patch). Using a newline as the terminator here means
the caller needs to look for "\n\n". That string occurs in the output
between the stat data and the patch and can also occur in the patch
hunks if diff.suppressBlankEmpty is set.
Now that we are calling diff_flush() in a loop we need to set .no_free
in our diff options and call diff_free() at the end of the program (see
the comment in diff.h)
Best Wishes
Phillip
+ continue;
+ }
+
if (*p != ':')
die("invalid raw diff input");
p++;
diff --git a/t/t4070-diff-pairs.sh b/t/t4070-diff-pairs.sh
index e0a8e6f0a0..aca228a8fa 100755
--- a/t/t4070-diff-pairs.sh
+++ b/t/t4070-diff-pairs.sh
@@ -77,4 +77,26 @@ test_expect_success 'split input across multiple diff-pairs' '
test_cmp expect actual
'
+test_expect_success 'diff-pairs explicit queue flush' '
+ git diff-tree -r -M -C -C -z base new >input &&
+ printf "\0" >>input &&
+ git diff-tree -r -M -C -C -z base new >>input &&
+
+ git diff-tree -r -M -C -C base new >expect &&
+ printf "\n" >>expect &&
+ git diff-tree -r -M -C -C base new >>expect &&
+
+ git diff-pairs <input >actual &&
+ test_cmp expect actual
+'
+j
+test_expect_success 'diff-pairs explicit queue flush null terminated' '
+ git diff-tree -r -M -C -C -z base new >expect &&
+ printf "\0" >>expect &&
+ git diff-tree -r -M -C -C -z base new >>expect &&
+
+ git diff-pairs -z <expect >actual &&
+ test_cmp expect actual
+'
+
test_done
