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

> Hi Justin
> 
> On 12/02/2025 04:18, Justin Tobler wrote:
> > Through git-diff(1), a single diff can be generated from a pair of blob
> > revisions directly. Unfortunately, there is not a mechanism to compute
> > batches of specific file pair diffs in a single process. Such a feature
> > is particularly useful on the server-side where diffing between a large
> > set of changes is not feasible all at once due to timeout concerns.
> > 
> > To facilitate this, introduce git-diff-pairs(1) which takes the
> > null-terminated raw diff format as input on stdin and produces diffs in
> > other formats. As the raw diff format already contains the necessary
> > metadata, it becomes possible to progressively generate batches of diffs
> > without having to recompute rename detection or retrieve object context.
> > Something like the following:
> > 
> > 	git diff-tree -r -z -M $old $new |
> > 	git diff-pairs -p
> > 
> > should generate the same output as `git diff-tree -p -M`. Furthermore,
> > each line of raw diff formatted input can also be individually fed to a
> > separate git-diff-pairs(1) process and still produce the same output.
> 
> I like the idea of this, I've left a few comments mainly around the UI.
> 
> > +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.
> 
> I think only accepting NUL terminated input is fine, but if we want to
> accept other formats we should  have a plan for how to do that in a
> backwards compatible way as we cannot use `-z` to distinguish between input
> formats.

> > +	const char * const usage[] = {
> > +		N_("git diff-pairs [diff-options]"),
> 
> Normally the option summary printed by "git foo -h" is generated by the
> option parser. In this case we don't define any options and use
> setup_revisions() instead so we need to provide the option summary
> ourselves. Looking at diff-files.c we can add
> 
> 	"\n"
> 	COMMON_DIFF_OPTIONS_HELP;
> 
> to do that.

> > +	/* Don't allow pathspecs at all. */
> > +	if (revs.prune_data.nr)
> > +		usage_with_options(usage, options);
> 
> It is not just pathspecs that we want to reject but all revision related
> options. Looking at diff-files.c we can do
> 
> 	if (rev.pending.nr ||
> 	    rev.min_age != -1 || rev.max_age != -1 ||
> 	    rev.max_count != -1)
> 		usage_with_option(usage, options);
> 
> To catch some of that but it still accepts things like "--first-parent",
> "--merges" and "--ancestry-path". We may just have to live with that as I
> don't think it is worth expanding a huge amount of effort to prevent them.

> > +	if (!revs.diffopt.output_format)
> > +		revs.diffopt.output_format = DIFF_FORMAT_RAW;
> 
> This matches the other diff plumbing commands but I'm not sure it is the
> most helpful default for a command that is supposed to transform raw diffs
> into another format. Maybe we should default to DIFF_FORMAT_PATCH?