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

[Jeff King <peff@peff.net>]

On Thu, Nov 24, 2016 at 09:55:07PM +0100, Johannes Schindelin wrote:

> +/*
> + * NEEDSWORK: this function can go once the legacy-difftool Perl script is
> + * retired.
> + *
> + * We intentionally avoid reading the config directly here, to avoid messing up
> + * the GIT_* environment variables when we need to fall back to exec()ing the
> + * Perl script.
> + */
> +static int use_builtin_difftool(void) {
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	struct strbuf out = STRBUF_INIT;
> +	int ret;
> +
> +	argv_array_pushl(&cp.args,
> +			 "config", "--bool", "difftool.usebuiltin", NULL);
> +	cp.git_cmd = 1;
> +	if (capture_command(&cp, &out, 6))
> +		return 0;
> +	strbuf_trim(&out);
> +	ret = !strcmp("true", out.buf);
> +	strbuf_release(&out);
> +	return ret;
> +}

FWIW, it should mostly Just Work to use the internal config functions
these days, with the caveat that they will not read repo-level config if
you haven't done repo setup yet.

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.

Of course, your method here is fine, too; I just know you are sensitive
to forking extra processes.

Also, a minor nit: capture_command() might return data in "out" with a
non-zero exit if the command both generates stdout and exits non-zero
itself. I'm not sure that's possible with git-config, though, so it
might not be worth worrying about.

-Peff