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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 7, 2023, 21:44 UTC
Message-ID: <230207.867cwt2kzh.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqq4jrxyxl2.fsf@gitster.g>

On Tue, Feb 07 2023, Junio C Hamano wrote:

Show 10 quoted lines

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> To clarify, I'm not suggesting that we ever read arbitrary parts of the
>> "diff.<driver>.<key>" config space, but that we could whitelist one set
>> of "diff.<driver>.<known-key>"="<known-values>".
>
> When the value names the path to an executable or the command line
> to invoke a program, there is no "portable" value that is useful.
> Whitelisting macOS only program only because its pathname is one of
> the known values does not help me running something else.

We're not talking about invoking an executable of any kind, but a mechanism to pick up a "diff.algorithm" setting referring to one of our built-in algorithms, either via the in-repo .gitattributes, as in John's original proposal:

	*.json diff-algorithm=histogram

Or via some hybrid .gitattributes/.gitconfig mechanism, where (if I understand Jeff's suggestion correctly) the two would contain:

	# In .gitattributes
	*.json diff=json 
	# In .gitconfig
	[diff "json"]
	algorithm = histogram

In terms of implementation this would be pretty much what we do with "submodule.<name>.update", where "Allowed values here are checkout, rebase, merge or none.".

I don't see where you're getting this suggestion of "[a] diff program suitable to compare two JSON files" from something I said.

That is a thing we generally support, but I've only mentioned that arbitrary command execution (e.g. in [1]) as something we explicitly *don't* want to support in this context.

The "submodule.<name>.update" example is particularly relevant because it also supports arbitrary command execution with "!"-prefixed values (the rest being the arbitrary command).

But that's something we specifically exclude when reading the in-repo version, it's only allowed in the user-controlled config.

I'm not saying that we should be going for the read-just-the-one-key in-repo .gitconfig approach here, just *if* we like that interface better the implementation should be relatively straightforward, assuming that we like the proposal in principle, and are just bikeshedding about what the mecanhism to enable it would be.

The advantage of that being that it more naturally slots into existing config, e.g. "diff.<driver>.binary=<bool>", this being just another "diff.<driver>.<known-key>".

And that the way to do that securely is something we're doing for several "submodule.*" keys, so if we pick that approach tweaking or stealing ideas from that code should be relatively straightforward.

1. https://lore.kernel.org/git/230206.865yce7n1w.gmgdl@evledraar.gmail.com/