Re: [PATCH v2 3/3] builtin/diff-pairs: allow explicit diff queue flush

[Phillip Wood <phillip.wood123@gmail.com>]

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