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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 19, 2025, 23:19 UTC
Message-ID: <xmqq1pvtfrq0.fsf@gitster.g>
In-Reply-To: <5uwp2vdm5tzv6n26fu77g4xys5ntjy2bj4hpgiuwpamxkij4zk@77jn55tynwse>

Justin Tobler <jltobler@gmail.com> writes:

Show 14 quoted lines

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

FWIW, in our toolset, "-z" is not the default primarily because text format were chosen to help debuggability, which used to really matter in the early days.

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

Abbreviated object names would only be at "might be nice to have" level, I would think. We are talking about tools-to-tools communication after all.

Show 5 quoted lines

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

Another consideration is which side of the pipeline should take the responsibility to invoke the diffcore machinery. We certainly could make it the job for the upstream/frontend, in which case diff-pairs does not have to call into diffcore-rename, BUT it also means the downstream/backend needs to be able to parse two paths (renamed from and renamed to). Or we could make it the job for the downstream, and forbid the upstream/frontend from feeding renamed pairs (i.e. any input with status letter R or C are invalid), in which case diff-pairs can choose to invoke rename detection or not by paying attention to the -M option and invoking diffcore_rename() itself (which should be at no-cost from coding point of view, as it should be just the matter of calling diffcore_std()).

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

Instead of getting expanded into its subpaths?

Show 8 quoted lines

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

No longer needed, as the tip of 'master' now lives in the .adoc world. Hurrah!