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

From: Jeff King <peff@peff.net>
Date: Feb 7, 2023, 17:56 UTC
Message-ID: <Y+KQtqNPews3vBS8@coredump.intra.peff.net>
In-Reply-To: <8e73793b0db3e84366a9c6441cc0fdc04f9614a5.1675568781.git.gitgitgadget@gmail.com>

On Sun, Feb 05, 2023 at 03:46:21AM +0000, John Cai via GitGitGadget wrote:

Show 23 quoted lines

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

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