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

[Patrick Steinhardt <ps@pks.im>]

On Tue, Feb 11, 2025 at 10:18:25PM -0600, Justin Tobler wrote:
> The diffs queued from git-diff-pairs(1) stdin are not flushed EOF is

I think you meant to say "are flush when stdin is closed" or something
like that.

> reached. To enable greater flexibility, allow control over when the diff
> queue is flushed by writing a single nul byte on stdin between input

s/nul/NUL/

> file pairs. Diff output between flushes is separated by a single line
> terminator.
> 
> Signed-off-by: Justin Tobler <jltobler@gmail.com>
> ---
>  Documentation/git-diff-pairs.adoc |  4 ++++
>  builtin/diff-pairs.c              | 11 +++++++++++
>  t/t4070-diff-pairs.sh             | 22 ++++++++++++++++++++++
>  3 files changed, 37 insertions(+)
> 
> diff --git a/Documentation/git-diff-pairs.adoc b/Documentation/git-diff-pairs.adoc
> index e9ef4a6615..33c0d702f0 100644
> --- a/Documentation/git-diff-pairs.adoc
> +++ b/Documentation/git-diff-pairs.adoc
> @@ -32,6 +32,10 @@ compute diffs progressively over the course of multiple invocations of
>  Each blob pair is fed to the diff machinery individually queued and the output
>  is flushed on stdin EOF.
>  
> +To explicitly flush the diff queue, a single nul byte can be written to stdin
> +between filepairs. Diff output between flushes is separated by a single line
> +terminator.

The same comment as for the previous patch applies here, I think we
should refrain from using jargon like "flushing", "diff queue" or
"filepairs". These are internal implementation details that the user
shouldn't need to worry about. Instead, we should be talking about the
user-visible effects.

> 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);

You can use `fputc(revs.diffopt.line_termination, revs.diffopt.file)`
instead.

> 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' '

s/null/NUL

> +	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
> +'
> +

Patrick