Re: [PATCH v2 2/3] builtin: introduce diff-pairs command

From: Justin Tobler <jltobler@gmail.com>
Date: Feb 19, 2025, 22:19 UTC
Message-ID: <5uwp2vdm5tzv6n26fu77g4xys5ntjy2bj4hpgiuwpamxkij4zk@77jn55tynwse>
In-Reply-To: <xmqqldubumxf.fsf@gitster.g>

On 25/02/12 08:50AM, Junio C Hamano wrote:

Show 23 quoted lines

> Justin Tobler <jltobler@gmail.com> writes:
> 
> > +NOTES
> > +----
> > +
> > +`diff-pairs` should handle any input generated by `diff-tree --raw -z`.
> > +It may choke or otherwise misbehave on output from `diff-files`, etc.
> > +
> > +Here's an incomplete list of things that `diff-pairs` could do, but
> > +doesn't (mostly in the name of simplicity):
> > +
> > + - Only `-z` input is accepted, not normal `--raw` input.
> > +
> > + - Abbreviated sha1s are rejected in the input from `diff-tree`; if you
> > +   want to abbreviate the output, you can pass `--abbrev` to
> > +   `diff-pairs`.
> > +
> > + - Pathspecs are not handled by `diff-pairs`; you can limit the diff via
> > +   the initial `diff-tree` invocation.
> 
> Which of the above limitations are fundamental, and which are merely
> due to incomplete implementation that could be improved in the
> future iterations?

Thinking about this some more, I'm a bit unsure whether git-diff-pairs(1) should support "normal" `--raw` input. Furthermore, if we do want to support it, maybe it should be the default?

From my perspective, ultimately I don't think there is much additional value provided by supporting multiple input options for git-diff-pairs(1) since the end result would be the same and its just an intermediate format. As I see it, the benefit of the NUL delimited raw diff ouput format is that it is a bit simpler to parse and likely a bit more efficient as it wouldn't have to deal with unquoting paths with special characters. The benefit of the "normal" raw format is probably that it is the more intuitive default option.

I'm certainly interested in what folks think about this :)

For abbreviated object IDs, supporting them would make the input format more flexible, but it would be simpler to just require the full OID be provided thus making the input format more explicit. My current thinking is to leave this unless others think it would be useful to support.

Regarding pathspec support, being that git-diff-pairs(1) operates solely on the provided set of file pairs produced via some other Git operation, I don't think further limiting would provide much additional value either. If we do want this though, I think support could be added in the future.

Show 8 quoted lines

> Without reading the code deeply, a lot of them
> look like merely due to this iteration being at a WIP state and not
> quite ready for the general public.
> 
> What is especially curious is the reason why it is limited to
> diff-tree (by the way, don't you require '-r' if you are fed
> 'diff-tree' output, or are you prepared to expand tree objects in
> the input yourself?).

The tree objects in the input are not expanded. With `git diff-pairs --raw` these objects are just printed again. With the `--patch` option, they are just ommitted.

> I can guess that the 0{40} object names in the postimage to signal
> paths with working tree changes unadded to the index is something
> this fundamentally cannot work with, but you should be able to grok
> 'diff-index --cached', which does not have that issue, just fine.

I'll rework the documentation in the next version. git-diff-tree(1) is the command I have in mind as the common usecase to use in combination with git-diff-pairs(1), but it is not solely limited to it. As you mentioned, there are other commands that could be used to provide input here.

Show 19 quoted lines

> > diff --git a/Documentation/meson.build b/Documentation/meson.build
> > index ead8e48213..e5ee177022 100644
> > --- a/Documentation/meson.build
> > +++ b/Documentation/meson.build
> > @@ -41,6 +41,7 @@ manpages = {
> >    'git-diagnose.adoc' : 1,
> >    'git-diff-files.adoc' : 1,
> >    'git-diff-index.adoc' : 1,
> > +  'git-diff-pairs.adoc' : 1,
> >    'git-difftool.adoc' : 1,
> >    'git-diff-tree.adoc' : 1,
> >    'git-diff.adoc' : 1,
> 
> This apparently does not apply to 'master' and the base at least
> needs to contain 1f010d6b (doc: use .adoc extension for AsciiDoc
> files, 2025-01-20).  Please clearly mark the series as such in the
> cover letter if the series is not built on top of recent 'master'
> (or 'maint' if it is a series to fix breakage, but it does not apply
> to this series).

Will do

Thanks -Justin