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

From: Patrick Steinhardt <ps@pks.im>
Date: Feb 12, 2025, 09:23 UTC
Message-ID: <Z6xopCvDXlX82lQ8@pks.im>
In-Reply-To: <20250212041825.2455031-4-jltobler@gmail.com>

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/

Show 21 quoted lines

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

Show 16 quoted lines

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

Show 22 quoted lines

> 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

Show 8 quoted lines

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