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

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Junio C Hamano <gitster@pobox.com>
Cc: Jeff King <peff@peff.net>,
	git@vger.kernel.org, David Aguilar <davvid@gmail.com>,
	Dennis Kaarsemaker <dennis@kaarsemaker.net>
Subject: Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
Date: Mon, 28 Nov 2016 18:34:17 +0100 (CET)	[thread overview]
Message-ID: <alpine.DEB.2.20.1611281830040.117539@virtualbox> (raw)
In-Reply-To: <xmqqa8cjlekl.fsf@gitster.mtv.corp.google.com>

Hi,

On Mon, 28 Nov 2016, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Sat, Nov 26, 2016 at 02:01:36PM +0100, Johannes Schindelin wrote:
> >
> >> > If you want to control it from outside the test script, you'd need
> >> > something like:
> >> > 
> >> >   if test "$GIT_TEST_DIFFTOOL" = "builtin"
> >> 
> >> That is a bit magic. I first used "GIT_USE_BUILTIN_DIFFTOOL" and it did
> >> not work. My name is arguably more correct (see also Jakub's note about
> >> "naming is hard"), but yours works because there is a "TEST" substring in
> >> it.
> >
> > Yes. You are free to add an exception to the env list in test-lib.sh,
> > but we usually use GIT_TEST_* to avoid having to do so.
> 
> Perhaps
> 
>  - The switch between "do I use builtin, or scripted?" mechanism in
>    1/2 can look at an environment (just like the old "am" rewrite
>    series did), instead of configuration.  This would make the code
>    a lot more simppler (you do not have to worry about the
>    interaction between "setup" and .git/config).
> 
>  - That environment variable can be named GIT_TEST_BUILTIN_DIFFTOOL;
>    after all, people are opting into helping to test the new shiny
>    to make/prove it ready sooner.
> 
>  - The bulk of the existing test for difftool can be moved to a
>    dot-included file (in a way similar to t/annotate-tests are
>    usable to test both annotate and blame-imitating-annotate).
>    Existing PERL prerequisites can all be lost.
> 
>  - Two tests can include that dot-included file; one would
>    explicitly unset that environment (and gives up without PERL
>    prerequisite), while the other explicitly sets it.

If my main worry was the test suite, I would agree with this plan.

However, I have been bitten time and again by problems that occurred only
in production, our test suite (despite taking already waaaaaay too long to
be truly useful in my daily development) was simply not good enough.

So my plan was different: to let end users opt-in to test this new beast
thoroughly, more thoroughly than any review would.

And for that, environment variables are just not an option. I need
something that can be configured in a portable application, so that the
main Git for Windows installation is unaffected.

My original "create a file in libexec/git-core/" was simple, did the job
reliably, and worked also for testing.

It is a pity that you two gentlemen shot it down for being inelegant. And
ever since, we try to find a solution that is as simple, works as
reliably, also for testing, *and* appeases your tastes.

Ciao,
Dscho