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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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

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