Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
From: Jeff King <hidden>
Date: 2016-11-24 21:08:57
On Thu, Nov 24, 2016 at 09:55:07PM +0100, Johannes Schindelin wrote:
quoted hunk ↗ jump to hunk
+/* + * 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