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 12:32, Elijah Newren wrote:

> On Mon, Feb 20, 2023 at 8:49 AM John Cai <johncai86@gmail.com> wrote:
>>
>> 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
>
> That would certainly simplify, but it'd be pretty important to
> document.  (Incidentally, this kind of decision was my reason for
> asking about all those special cases earlier, i.e. how to handle diff
> between different commits, how to handle renames, how to handle bare
> repositories, etc.)

Good point--that would be good to document.

>
> This kind of decision probably also means you'd need a variety of
> testcases where .gitattributes is different in every commit & the
> index & the working tree, and then you start testing several of the
> possible pairings to make sure the right .gitattributes file is used
> (e.g. (commit, commit), (commit, index), (index, commit), (worktree,
> index), etc.)
>
> However, I'm curious again.  You brought this up because you want to
> use it in GitLab, yet configuration of using this option as it appears
> in this series requires changing _both_ .gitattributes and
> .git/config.  How will users of the GitLab UI do the configuration
> necessary for the git-config side to take effect?

Good question--we would likely need to add some pre-baked configuration server
side with a driver config that users could then tap into with their
gitattributes files.