Re: [PATCH v3 2/2] diff: teach diff to read algorithm from diff driver

[John Cai <johncai86@gmail.com>]

Hi Elijah,

On 20 Feb 2023, at 11:21, Elijah Newren wrote:

> On Mon, Feb 20, 2023 at 7:32 AM John Cai <johncai86@gmail.com> wrote:
> [...]
>>> I'm still curious if this should this also include warnings/caveats, such as:
>>>   * The diff attribute specified in .gitattributes will be ignored in
>>> a bare clone
>>>   * The diff attribute specified in .gitattributes will be ignored if
>>> it is only specified in another branch (e.g. on a branch "special-file
>>> diff=patience" recorded in .gitattributes, then checkout master but
>>> run `git log -1 -p $branch`)
>>>   * When a file is renamed, the diff attribute for the pre-image name
>>> is the only one the system pays attention to (thus adding "-R" can
>>> flip which diff algorithm is run for the renamed file).
>>
>> I would be fine with adding that--though originally I was thinking that these
>> can be inferred from the way that gitattributes are documented in [1]. Calling
>> these out would make it more clear though, so I could go either way.
>>
>>>
>>> Also, since I tested the three items above to verify they are valid
>>> warnings, I'm a bit confused.  I thought your intent was to use this
>>> server-side[1], so isn't the bare clone aspect a deal-breaker for your
>>> intended usecase?
>>>
>>> [1] https://lore.kernel.org/git/7852AC7B-7A4E-4DD0-ADEA-CFFD5D16C595@gmail.com/
>>
>> yes, indeed. I was planning on adding bare repository support in a separate
>> patch series, since the additions in [2] allows .gitattributes to be read from a
>> bare repository.
>>
>> 1. https://git-scm.com/docs/gitattributes
>> 2. https://lore.kernel.org/git/0ca8b2458921fc40269b0c43b5ec86eba77d6b54.1673684790.git.karthik.188@gmail.com/
>>
>> thanks!
>> John
>
> Oh, interesting, I didn't know about [2].  So, is the plan to take the
> --source option from that series and add it to diff (perhaps with a
> different name, since log tends to consume diff options and --source
> is already taken)?

Yep, that would be the general idea

>
> And do you expect to get the tree-ish from the two the users are
> already specifying to diff?  If so, which one do you use (the two
> commits being diffed might have differing .gitattributes files)?  If
> not, what does that mean for users of e.g. the GitLab UI who have to
> specify a third tree when diffing?

Good question! Since it seems that when `git-diff(1)` considers diff.<driver>,
it goes with the path of the first one. (might need some confirmation here)

in diff.c:


static void run_diff(struct diff_filepair *p, struct diff_options *o)
{
	const char *pgm = external_diff();
	struct strbuf msg;
	struct diff_filespec *one = p->one;
	struct diff_filespec *two = p->two;
	const char *name;
	const char *other;
	const char *attr_path;

	name  = one->path;
	other = (strcmp(name, two->path) ? two->path : NULL);
	attr_path = name;
	if (o->prefix_length)

I was thinking we would just use the tree-ish of the first one

thanks
John