From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mon, 17 Feb 2025 14:38:11 GMT
Subject: Re: [PATCH v2 2/3] builtin: introduce diff-pairs command
Message-ID: <d6d4230e-7b80-4eec-b218-37717ae2e298@gmail.com>
In-Reply-To: <20250212041825.2455031-3-jltobler@gmail.com>

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.

> +	argc = setup_revisions(argc, argv, &revs, NULL);

I think we should check that there are no options left on the 
commandline after setup_revisions() returns

> +	/* 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?


> +test_expect_success 'split input across multiple diff-pairs' '

This needs a PERL prerequisite I think. I'm a bit unsure what this test 
adds compared to the others.

Best Wishes

Phillip

> +	write_script split-raw-diff "$PERL_PATH" <<-\EOF &&
> +	$/ = "\0";
> +	while (<>) {
> +	  my $meta = $_;
> +	  my $path = <>;
> +	  # renames have an extra path
> +	  my $path2 = <> if $meta =~ /[RC]\d+/;
> +
> +	  open(my $fh, ">", sprintf "diff%03d", $.);
> +	  print $fh $meta, $path, $path2;
> +	}
> +	EOF
> +
> +	git diff-tree -p -M -C -C base new >expect &&
> +
> +	git diff-tree -r -z -M -C -C base new |
> +	./split-raw-diff &&
> +	for i in diff*; do
> +		git diff-pairs -p <$i || return 1
> +	done >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_done