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

Hi Peff,

On Thu, 24 Nov 2016, Jeff King wrote:

On Thu, Nov 24, 2016 at 10:56:23PM +0100, Johannes Schindelin wrote:

quoted

quoted

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