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

From: Jeff King <peff@peff.net>
Date: Nov 24, 2016, 21:08 UTC
Message-ID: <20161124210841.c4yi2nv57tjfejgj@sigill.intra.peff.net>
In-Reply-To: <3a10fd72cf336743891d6bc9a003cfeb5c3f0063.1480019834.git.johannes.schindelin@gmx.de>

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

Show 23 quoted lines

> +/*
> + * 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