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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 24, 2016, 10:38 UTC
Message-ID: <alpine.DEB.2.20.1611241123340.117539@virtualbox>
In-Reply-To: <xmqqshqhlthu.fsf@gitster.mtv.corp.google.com>

Hi Junio,

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

Show 12 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > ... 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