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