Re: [PATCH v3 0/3] batch blob diff generation

From: phillip.wood123@gmail.com <phillip.wood123@gmail.com>
Date: Feb 26, 2025, 14:58 UTC
Message-ID: <1e41249a-5241-4cd8-8a6a-3c9163fb0ea0@gmail.com>
In-Reply-To: <20250225233925.1345086-1-jltobler@gmail.com>

Hi Justin

On 25/02/2025 23:39, Justin Tobler wrote:

Show 25 quoted lines

> 
> Changes since V2:
> 
>      - Pathspecs are not supported and thus rejected when provided as
>        arguments. It should be possible in a future series to add support
>        though.
> 
>      - Tree objects present in `diff-pairs` input are rejected. Support
>        for tree objects could be added in the future, but for now they
>        are rejected to enable to future support in a backwards compatible
>        manner.
> 
>      - The -z option is required by git-diff-pairs(1). The NUL-delimited
>        raw diff format is the only accepted form of input. Consequently,
>        NUL-delimited output is the only option in the `--raw` mode.
> 
>      - git-diff-pairs(1) defaults to patch output instead of raw output.
>        This better fits the intended usecase of the command.
> 
>      - A NUL-byte is now always used as the delimiter between batches of
>        file pair diffs when queued diffs are explicitly computed by
>        writing a NUL-byte on stdin.
> 
>      - Several other small cleanups and fixes along with documentation
>        changes.

This addresses all my comments on the previous version, thank you. I do wonder if tying the input line termination to the output line termination is a good idea for a program that aims to to transform one diff format into another. Having said that this series is aimed at machine consumption of the output so it probably isn't a big problem.

I also think we might want to massage the output in the tests so that we're not running test_cmp on files containing NUL bytes. Using

     git diff-tree -z ... | tr '\0' Q >actual

would get rid of the NULs but does not improve the readability of the raw diffs that much as everything is still on a single line. Using '\n' instead of 'Q' would give us mulit-line output but we would lose confidence that the original output was actually NUL terminated.

Best Wishes

Phillip

Show 36 quoted lines

> Changes since V1:
> 
>      - Changed from git-diff-blob(1) to git-diff-pairs(1) based on a
>        previously submitted series.
> 
>      - Instead of each line containing a pair of blob revisions, the raw
>        diff format is used as input which already has diff status and
>        object context embedded.
> 
> -Justin
> 
> [1]: <20161201204042.6yslbyrg7l6ghhww@sigill.intra.peff.net>
> 
> Justin Tobler (3):
>    diff: return diff_filepair from diff queue helpers
>    builtin: introduce diff-pairs command
>    builtin/diff-pairs: allow explicit diff queue flush
> 
>   .gitignore                        |   1 +
>   Documentation/git-diff-pairs.adoc |  60 +++++++++
>   Documentation/meson.build         |   1 +
>   Makefile                          |   1 +
>   builtin.h                         |   1 +
>   builtin/diff-pairs.c              | 206 ++++++++++++++++++++++++++++++
>   command-list.txt                  |   1 +
>   diff.c                            |  70 +++++++---
>   diff.h                            |  25 ++++
>   git.c                             |   1 +
>   meson.build                       |   1 +
>   t/meson.build                     |   1 +
>   t/t4070-diff-pairs.sh             |  83 ++++++++++++
>   13 files changed, 432 insertions(+), 20 deletions(-)
>   create mode 100644 Documentation/git-diff-pairs.adoc
>   create mode 100644 builtin/diff-pairs.c
>   create mode 100755 t/t4070-diff-pairs.sh
>