Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Thu, 1 Dec 2016, Junio C Hamano wrote:

> As to "I have to spawn config", I think it is sensible to start the
> cmd_difftool() wrapper without adding RUN_SETUP to the command table,
> then call git_config_get_bool() to check the configuration only from
> system and per-user files, and then finally either call into
> builtin_difftool() where setup_git_directory() is called, or spawn the
> scripted difftool, as Peff already said.

I just spent a considerable amount of time to figure out that I overlooked
your comment about "only from system and per-user files".

> Your "users opt-in while installing" is not about setting per-repository
> option.

Wow. That would make things really inconsistent.

I know that *I* would want to be able to switch that opt-in feature off
for repositories where I absolutely rely on some rock-solid difftool. And
as a user I would not only be shocked when it would not work as expected,
but I would be *positively* shocked to learn that this is intended, by
design.

Seriously, do you really think it is a good idea to have
git_config_get_value() *ignore* any value in .git/config?

Really?

It caught *me* by surprise, and it definitely makes for a very, very bad
user experience.

And this is much more fundamental than just difftool.useBuiltin.

I see for example that our pager.<cmd> setting is ignoring per-repository
settings. That is, if the user sets pager.<cmd> in ~/.gitconfig and then
tries to override this in a specific repository (which any user would only
do for very good reasons, I am sure), Git would happily ignore that
careful setup and create an angry user.

The only reason this did not blow up in our face yet is that users
apparently do not make use of this feature yet. Which does not make this
behavior (that "git_config_get_value()" happily ignores .git/config) less
broken.

We need to fix this.

Ciao,
Dscho