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

From: John Cai <johncai86@gmail.com>
Date: Feb 9, 2023, 16:34 UTC
Message-ID: <20230209163400.xi7j3ivd4k6gqoc7@jcai-ubuntu>
In-Reply-To: <Y+KQtqNPews3vBS8@coredump.intra.peff.net>

On 23/02/07 12:56PM, Jeff King wrote:

Show 35 quoted lines

> On Sun, Feb 05, 2023 at 03:46:21AM +0000, John Cai via GitGitGadget wrote:
> 
> > +`diff-algorithm`
> > +^^^^^^^^^^^^^^^^
> > +
> > +The attribute `diff-algorithm` affects which algorithm Git uses to generate
> > +diffs. This allows defining diff algorithms per file extension. Precedence rules
> > +are as follows, in order from highest to lowest:
> > +
> > +*Command line option*
> > +
> > +Pass in the `--diff-algorithm` command line option int git-diff(1)
> > +
> > +*Git attributes*
> > +
> > +------------------------
> > +*.json	diff-algorithm=histogram
> > +------------------------
> > +
> > +*Git config*
> > +
> > +----------------------------------------------------------------
> > +[diff]
> > +	algorithm = histogram
> > +----------------------------------------------------------------
> 
> From the user's perspective, this is weirdly inconsistent with the
> existing diff attributes, which would be more like:
> 
>   # in .gitattributes
>   *.json diff=json 
> 
>   # in config
>   [diff "json"]
>   algorithm = histogram

Thanks for this suggestion, Peff. What I like about this is that it builds off of the existing diff.<driver> scheme rather than inventing another one. Additionally, we won't get hit with a performance penalty since we already read gitattrbitues to see if a driver has been set or not.

Thinking out loud, if we add "algorithm" as a key for diff.<driver>, it would be mutually exclusive with "command" where "command" takes precedence, correct?

Show 21 quoted lines

> 
> I know why one might choose the scheme you did; it kicks in if the repo
> sets the algorithm, without users having to set up any extra config.
> Which is sort of nice, if we assume that malicious actors don't have any
> incentive to pick the algorithm. In theory they don't, though I saw Ævar
> mention possible DoS elsewhere in the thread.
> 
>   Side note: It's also possible that algorithm selection could be
>   required to trigger a separate security bug (say, a buffer overflow in
>   the patience code or something), so restricting that works in a
>   belt-and-suspenders way. But that somehow feels like like the wrong
>   side of the paranoia-vs-feature line.
> 
> So I dunno. I recognize that this scheme fulfills your immediate needs
> better, but I fear that we'll be stuck with a weird split between "diff"
> and "diff-*" attributes forever. In the long run, having a way for the
> repo to say "and here is some config I recommend to you" would give you
> the best of both, but that is a challenging topic that has been
> discussed and punted on for many years.
> 
> -Peff