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

[Elijah Newren <newren@gmail.com>]

Hi John!

On Tue, Feb 14, 2023 at 1:16 PM John Cai <johncai86@gmail.com> wrote:
> On 9 Feb 2023, at 3:44, Elijah Newren wrote:
> > On Mon, Feb 6, 2023 at 12:47 PM John Cai <johncai86@gmail.com> wrote:
> >>
[...]
> It seems like the performance penalty was because I was adding calls to parse
> attribute files. Piggy backing off of the attribute parsing in userdiff.h will
> allow us to not incur this performance penalty:
>
> $ hyperfine -r 5 -L a bin-wrappers/git,git '{a} diff v2.0.0 v2.28.0'
> Benchmark 1: git-bin-wrapper diff v2.0.0 v2.28.0
>   Time (mean ± σ):      1.072 s ±  0.289 s    [User: 0.626 s, System: 0.081 s]
>   Range (min … max):    0.772 s …  1.537 s    5 runs
>
> Benchmark 2: git diff v2.0.0 v2.28.0
>   Time (mean ± σ):      1.003 s ±  0.065 s    [User: 0.684 s, System: 0.067 s]
>   Range (min … max):    0.914 s …  1.091 s    5 runs
>
> Summary
>   'git diff v2.0.0 v2.28.0' ran
>     1.07 ± 0.30 times faster than 'git-bin-wrapper diff v2.0.0 v2.28.0'

Yaay!  Much better.  :-)

I'm curious, though, whether you are showing here a 7% slowdown (which
would still be bad), or just that the feature is correctly choosing a
different (but slower) algorithm for some files, or some kind of mix.

What is the performance difference if you have this feature included,
but don't have any directives in .gitattributes selecting a different
diff algorithm for any files?

> > And on a separate note...
> >
> > There's another set of considerations we might need to include here as
> > well that I haven't seen anyone else in this thread talk about:
>
> These are some great questions. I'll do my best to answer them.
> >
> > * When trying to diff files, do we read the .gitattributes file from
> > the current checkout to determine the diff algorithm(s)?  Or the
> > index?  Or the commit we are diffing against?
> > * If we use the current checkout or index, what about bare clones or
> > diffing between two different commits?
> > * If diffing between two different commits, and the .gitattributes has
> > changed between those commits, which .gitattributes file wins?
> > * If diffing between two different commits, and the .gitattributes has
> > NOT changed, BUT a file has been renamed and the old and new names
> > have different rules, which rule wins?
>
> In the next version I plan on using Peff's suggestion of utilizing the existing
> diff driver scheme [1]. I believe these four questions are addressed if we use
> the existing userdiff.h API, which in turn calls the attr.h API. We check the
> worktree, then fallback to the index.

So...it sounds like we're just ignoring all the special cases listed
above, and living with bugs related to them?  That's not a criticism;
in fact, it might be okay -- after all, that's exactly what the
existing .gitattributes handling does and you are just hooking into
it.

I am a bit concerned, though, that we're increasing the visibility of
the interactions of .gitattributes with respect to these kinds of
cases.  I think external drivers are probably much less used than what
your feature might be, so folks are more likely to stumble into these
cases and complain.  Perhaps those cases are rare enough that we don't
care, but it might be at least worth documenting the issues (both to
manage user expectations and to give people a heads up about the
potential issues.)

(Also, it may be worth mentioning that I tend to focus on unusual
cases for anything that might touch merging; Junio once named one of
my patchsets "en/t6042-insane-merge-rename-testcases".  It's possible
I worry about corner cases more than is justified given their real
world likelihood.)

> By using the userdiff.h API, the behavior will match what users already expect
> when they for instance set an external driver.

s/already expect/already get/

The bugs also affect external drivers; I just suspect external drivers
aren't used enough that users have complained very loudly (yet?).

> 1. https://lore.kernel.org/git/Y+KQtqNPews3vBS8@coredump.intra.peff.net/
>
> >
> > * If per-file diff algorithms are adopted widely enough, will we be
> > forced to change the merge algorithm to also pay attention to them?
> > If it does, more complicated rename cases occur and we need rules for
> > how to handle those.
> > * If the merge algorithm has to pay attention to .gitattributes for
> > this too, we'll have even more corner cases around what happens if
> > there are merge conflicts in .gitattributes itself (which is already
> > kind of ugly and kludged)
>
> I see this feature as a user-experience type convenience feature, so I don't
> believe there's need for the merge machinery to also pay attention to the diff
> algorithm set through gitattrbutes. We can clarify this in the documentation.

That would be awesome; *please* do this.  This is my primary concern
with this patchset.

I've spent an awful lot of time dealing with weird corner cases in the
merge machinery, and this appears to open a big can of worms to me.
It'd be a huge relief if we just agreed that the .gitattributes
handling here is only meant for user-facing diffs and will not be
consulted by the merge machinery.

> > Anyway, I know I'm a bit animated and biased in this area, and I
> > apologize if I'm a bit too much so.  Even if I am, hopefully my
> > comments at least provide some useful context.
>
> No problem! thanks for raising these issues.
>
> thanks
> John