Re: [PATCH v2 2/2] diff: teach diff to read gitattribute diff-algorithm

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 15, 2023, 03:20 UTC
Message-ID: <xmqqwn4j1uon.fsf@gitster.g>
In-Reply-To: <xmqq4jrn3ac7.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 12 quoted lines

>> diff --git a/diff.c b/diff.c
>> index 92a0eab942e..24da439e56f 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -4456,15 +4456,11 @@ static void run_diff_cmd(const char *pgm,
>>  	const char *xfrm_msg = NULL;
>>  	int complete_rewrite = (p->status == DIFF_STATUS_MODIFIED) && p->score;
>>  	int must_show_header = 0;
>> +	struct userdiff_driver *drv = userdiff_find_by_path(o->repo->index, attr_path);
>
> Do we run this look-up unconditionally, even when .allow_external
> bit is not set?  Why?

Ah, this is perfectly fine. It used to be that this codepath can tell that there is no need to check the diff driver when it is told never to use any external diff driver. Now, even when it is computing the diff internally, it needs to check the diff driver to find out the favoured algorithm for the path.

Strictly speaking, if we are told NOT to use external diff driver, and if we are told NOT to pay attention to algorithm given by the diff driver, then we know we can skip the overhead of attribute look-up. I.e. we could do this to avoid attribute look-up:

	struct userdiff_driver *drv = NULL;

	if (o->flags.allow_external || !o->ignore_driver_algorithm)
		drv = userdiff_find_by_path(...);

	if (drv && o->flags.allow_external && drv->external)
		pgm = drv->external;
	...
	if (pgm)
		... do the external diff thing ...
	if (one && two) {
		if (drv && !o->ignore_driver_algorithm && drv->algorithm)
			set_diff_algo(...)

I was not sure if it would be worth it before writing the above down, but the resulting flow does not look _too_ bad.

Show 10 quoted lines

>> @@ -4583,6 +4584,10 @@ static void run_diffstat(struct diff_filepair *p, struct diff_options *o,
>>  	const char *name;
>>  	const char *other;
>>  
>> +	struct userdiff_driver *drv = userdiff_find_by_path(o->repo->index, p->one->path);
>> +	if (drv && drv->algorithm)
>> +		set_diff_algorithm(o, drv->algorithm);
>
> Interesting.  Does external diff play a role, like in run_diff_cmd()
> we saw earlier?

As whoever wrote "diffstat" did not think of counting output from external diff driver, of course in this codepath external diff would not appear. So what we see is very much expected.

Just move the blank line we see before these new lines one line down, so that the variable decls are grouped together, with a blank line before the first executable statement. I.e.

	const char *name;
	const char *other;
+       struct userdiff_driver *drv;
+
+	drv = userdiff_find_by_path(...);
+	if (drv && drv->algorithm)
+		set_diff_algorithm(o, drv->algorithm);

Shouldn't this function refrain from setting algorithm from the driver when the algorithm was given elsewhere? E.g.

	$ git show --histogram --stat
	
or something?  IOW, shouldn't it also pay attention to
o->ignore_driver_algorithm bit, just like run_diff_cmd() did?