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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 25, 2016, 11:05 UTC
Message-ID: <alpine.DEB.2.20.1611251201580.117539@virtualbox>
In-Reply-To: <20161125031809.mueurvjijlnzy2s5@sigill.intra.peff.net>

Hi Peff,

On Thu, 24 Nov 2016, Jeff King wrote:

Show 18 quoted lines

> On Thu, Nov 24, 2016 at 10:56:23PM +0100, Johannes Schindelin wrote:
> 
> > > I think it would probably be OK to ship with that caveat (people would
> > > probably use --global config, or "git -c" for a quick override), but if
> > > you really wanted to address it, you can do something like what
> > > pager.c:read_early_config() does.
> > 
> > The config setting is already overkill (and does even make something much
> > harder than before: running tests with the builtin difftool used to be as
> > simply as `touch use-builtin-difftool && make -C t t7800-difftool.sh, now
> > I have to edit t7800-difftool.sh to configure difftool.useBuiltin, and
> > without the repo-level config even that would not be working).
> > 
> > Imitating read_early_config() would be overkill deluxe.
> 
> I would have expected it to just be a build-time flag, like:
> 
>   make BUILTIN_DIFFTOOL=Yes test

That works for Git developers.

I want to let as many users as possible test the builtin difftool. Hopefully a lot more users than there are Git developers.

Which means that I need a feature flag in production code, not a build time flag.

> I'm happy with pretty much anything under the reasoning of "this does not
> matter much because it is going away soon".

Yeah, well, I am more happy with anything along the lines of David's review, pointing out flaws in the current revision of the builtin difftool before it bites users ;-)

Ciao, Dscho