Re: [PATCH v2 3/3] builtin/diff-pairs: allow explicit diff queue flush
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Feb 17, 2025, 14:38 UTC
- Message-ID
- <30c13b14-945d-4984-bb49-9fd93a4dedc9@gmail.com>
- In-Reply-To
- <20250212041825.2455031-4-jltobler@gmail.com>
Hi Justin
On 12/02/2025 04:18, Justin Tobler wrote:
Show 5 quoted lines
> 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.
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);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
Show 37 quoted lines
> + 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