Hi Peff,
On Thu, 24 Nov 2016, Jeff King wrote:
> 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.
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.
> 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.
As it is, I spent way too much time on a feature flag *that will go away
real soon*. And not only I spent too much time on it: everybody who even
bothered to think about it spent too much time on it. It is a temporary
feature flag. It will go away. If it is inefficient, or inelegant, it
won't matter in a month from now.
In other words, it was not really necessary to spend all of that time and
all of that brain power to first discuss the shortcomings of having the
presence of a file in exec path as a feature flag, then converting it into
a config setting, only to find out that this *causes* problems that were
not there before.
Frankly, that was not what I was hoping for. I was hoping to get a decent
review of the *difftool* functionality. David did a fine job to provide
that review. All that discussion about the temporary feature flag was
really a side track for me, and I hate to admit that I let myself get
sucked into it, and it cost me quite some time that I would have rather
spent on release engineering the preview based on v2.11.0-rc3 (including
the difftool with *any* type of working opt-in feature flag).
Maybe I will write more explicitly in the next cover letter what I intend
to do, and what parts of the patch series is intended to be throw-away
material (hence "good enough" is good enough, and discussions about it are
really wasting all of our time). Of course, I cannot dictate what other
people find interesting to comment on, but at least I will have a good
excuse to ignore suggestions that only distract from the work that is
really needed.
My only solace is that I did some substantially more intensive testing in
the wake of this discussion, thanks to the test suite breakages incurred
by switching the feature flag to a soon-to-be-abandoned config setting.
That makes me much more confident about the builtin difftool, which is all
I wanted in the first place.
Ciao,
Dscho