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

From: Elijah Newren <newren@gmail.com>
Date: Feb 15, 2023, 03:41 UTC
Message-ID: <CABPp-BGmFemkiD1OFrrOdaJt9PjGRp+QHoV_azPqvTtx6CdD9Q@mail.gmail.com>
In-Reply-To: <AF5092D2-A561-4B56-8FB8-25DCFA28F32C@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:
> >>

[...]

Show 16 quoted lines

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

Show 22 quoted lines

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

Show 15 quoted lines

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

Show 8 quoted lines

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