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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 23, 2016, 17:29 UTC
Message-ID: <alpine.DEB.2.20.1611231824530.3746@virtualbox>
In-Reply-To: <1479912693.5181.27.camel@kaarsemaker.net>

Hi Dennis,

On Wed, 23 Nov 2016, Dennis Kaarsemaker wrote:

Show 18 quoted lines

> On Tue, 2016-11-22 at 18:01 +0100, Johannes Schindelin wrote:
> > 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