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

Johannes Schindelin [off-list ref] writes:

The config kinda works now. But for what price. It stole 4 hours I did not
have. When the libexec/git-core/use-builtin-difftool solution took me a
grand total of half an hour to devise, implement and test.

And you know what? I still do not really see what is so bad about it.

I was wondering if I should explain myself again, even though I do
not see what good it would do, as clearly my point did not come
across in the other emails.  And then you would just complain that I
am making work for you.  Clearly you do not seem to see why placing
random files in $GIT_EXEC_PATH, which is a place for git subcommand
implementations, is wrong, so I won't repeat it to you again.

But you need to remember that you are not working on a Windows-only
project.  In non-Windows environment, many users would not have
write access to /usr/libexec/git-core directory, but it is not just
easy for them to write into ~/.gitconfig, but that is the way they
are accustomed to, in order to affect the behaviour of Git for them.

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.  Your "users
opt-in while installing" is not about setting per-repository option.

Calling git_config*(), setup_git_directory() and then git_config*()
in this order should be safe, as setup_git_directory() would clear
potentially cached configuration values read by any previous
git_config*() calls, so any configuration enquiry made by
builtin_difftool() would read from all three sources, not just
system and per-user.

So there is no chicken-and-egg issue, either.

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