Re: [PATCH v2 1/1] difftool: add the builtin

Hi Junio,

On Wed, 23 Nov 2016, Junio C Hamano wrote:

Junio C Hamano [off-list ref] writes:

quoted

... I do not think you can safely add these two bits here until the
migration completes.

I accidentally removed a more useful bit I wrote after the above
sentence while editing.

The NEEDSWORK comment in 73c2779f42 ("builtin-am: implement skeletal
builtin am", 2015-08-04) mentions why it calls setup-git-directory
and setup-work-tree instead of letting run_builtin() do so; perhaps
you can do something similar here to fix this.

This is the Catch-22 I mentioned a couple times: if you insist on a config
setting, the config has to be read. For that to work,
setup_git_directory() has to be called.

So no matter what you do, if you want to have conditional code that
depends on the config, and that wants setup_git_directory() *not* to be
called before, you are simply out of luck.

Sadly, I now bought into your comment that using a file in exec-path as a
feature flag is a bad thing, and that we have to use a config setting. So
now I have to spend more time on fixing something that was not a problem
in my original patches.

However, this exchange has something else in it, apart from creating
unneeded work for me.

What you really accidentally did was to identify a fundamental problem
with the builtin difftool: when called from a subdirectory, the RUN_SETUP
flag would make it chdir() to the top-level directory, and the
subsequently spawned Git processes would get the wrong idea about relative
paths.

Thank you for that,
Dscho