Hi Niels

On 25/04/2025 16:59, Niels Glodny wrote:

The cleanup_records function marks some lines as changed
before running the actual diff algorithm. For most lines,
this is a good performance optimization, but it also marks
lines that are surrounded by many changed lines as changed
as well. This can cause redundant changes and longer-than-
necessary diffs.

Nicely explained. We normally wrap commit messages at 72 characters

Whether this results in better-looking diffs is subjective.
However, the --minimal flag explicitly requests the shortest
possible diff.

Looking at the diff for 2ff58dec493 (refs: introduce function to batch 
refname availability checks, 2025-03-12) I'd say it is definitely less 
readable with this change but some of the changes in 320f2061b63 (t0602: 
use subshell to ensure working directory unchanged, 2025-02-28) are more 
readable. Anyway as you say if we promise to find the minimal diff then 
that is what we should do.

The performance impact of this change is negligible, and it
results in shorter diffs in about 1.3% of diffs in Git's
history.

Have you got any numbers for the performance change?

I think the premise of this patch is sound, I've left a couple of 
comments below as I think we can simplify the code changes.

Thanks for taking the trouble to add a new test. We have a few test 
helper functions which we could use here.

	test_write_lines x x x x >pre &&
	test_write_lines x x x A B C D x E F G >post &&
	test_expect_code 1 git diff --no-index --minimal pre post >diff &&
	test_grep ! ^[-+]x diff

test_expect_code ensures that the non-zero exit code is from the files 
not matching rather than another error like an invalid option or missing 
file. test_grep will display the file if there are matches which helps 
when debugging test failures.

 > [...]> -static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t 
*xdf1, xdfile_t *xdf2) {

xdlclassifier_t carries a copy of the flag that were interested in so I 
think at the start of xdl_cleanup_records we can add

	int need_min = !!(cf->flags & XDF_NEED_MINIMAL);

and then we don't need to change the signature.

  	for (i = xdf1->dstart, recs = &xdf1->recs[xdf1->dstart]; i <= xdf1->dend; i++, recs++) {
  		rcrec = cf->rcrecs[(*recs)->ha];
  		nm = rcrec ? rcrec->len2 : 0;
-		dis1[i] = (nm == 0) ? 0: (nm >= mlim) ? 2: 1;
+		dis1[i] = (nm == 0) ? 0: (nm >= mlim && !need_min) ? 2: 1;

If we want a minimal diff then we force dis1[i] to be 1 so that this 
line will never be marked as changed before searching for the longest 
common sequence. Well spotted.

Best Wishes

Phillip