Re: [PATCH 2/2] difftool: add a feature flag for the builtin vs scripted version

Hi Dennis,

On Wed, 23 Nov 2016, Dennis Kaarsemaker wrote:

On Tue, 2016-11-22 at 18:01 +0100, Johannes Schindelin wrote:

quoted

The original idea was to use an environment variable
GIT_USE_BUILTIN_DIFFTOOL, but the test suite resets those variables, and
we do want to use that feature flag to run the tests with, and without,
the feature flag.

Besides, the plan is to add an opt-in flag in Git for Windows'
installer. If we implemented the feature flag as an environment
variable, we would have to modify the user's environment, in order to
make the builtin difftool the default when called from Git Bash, Git CMD
or third-party tools.

Why is this not a normal configuration variable (as in git config
difftool.builtin true or something)? It doesn't make much sense to me
to introduce a way of configuring git by introducing magic files, when
a normal configuration variable would do just fine, and the GfW
installer can also set such variables, like it does for the crlf config
I believe.

I considered that. Adding a config setting would mean we simply test for
it in git-difftool.perl and call the builtin if the setting is active,
right?

The downside is that we actually *do* go through Perl to do that. Only to
go back to a builtin. Which is exactly the thing I intended to avoid.

If we do not go through Perl, we have to set up the git directory and
parse the config in git.c *just* to figure out whether we want to
magically forward difftool to builtin-difftool. That is not only ugly, but
has potential side effects I was not willing to risk.

In any case, this feature flag will be there only for one or two Git for
Windows releases, to give early adopters a chance to send me bug reports
about any regressions.

To be crystal-clear: I never expected this patch to enter git.git.

In that light, I am okay with taking the heat for introducing a temporary,
Git for Windows-only feature flag that is implemented as a "does the file
<xyz> exist?" test.

Ciao,
Dscho