Message ID | 20250225233925.1345086-1-jltobler@gmail.com (mailing list archive) |
---|---|
Headers | show |
Series | batch blob diff generation | expand |
Hi Justin On 25/02/2025 23:39, Justin Tobler wrote: > > 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 > 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 >
On 25/02/26 02:58PM, phillip.wood123@gmail.com wrote: > 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. Is the underlying motivation here to provide more feedback if a test fails? I somewhat have a preference for the test to be validating the output as it is actually expected. As you mentioned, getting rid of the NUL bytes wouldn't help with readability much and we probably wouldn't want to replace with `\n`, so maybe a simple "Binary files expect and actual differ" would be the most straightforward. If this is the preferred way to handle it, I can adapt in a followup version though. :) Thanks for the review! -Justin