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

[Junio C Hamano <gitster@pobox.com>]

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

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

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