From: Justin Tobler <jltobler@gmail.com>
Date: Wed, 19 Feb 2025 22:19:44 GMT
Subject: Re: [PATCH v2 2/3] builtin: introduce diff-pairs command
Message-ID: <5uwp2vdm5tzv6n26fu77g4xys5ntjy2bj4hpgiuwpamxkij4zk@77jn55tynwse>
In-Reply-To: <xmqqldubumxf.fsf@gitster.g>

On 25/02/12 08:50AM, Junio C Hamano wrote:
> 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.

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

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