Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Dec 6, 2016, 13:16 UTC
Message-ID: <alpine.DEB.2.20.1612061411000.117539@virtualbox>
In-Reply-To: <xmqqy3zu43yk.fsf@gitster.mtv.corp.google.com>

Hi Junio,

On Mon, 5 Dec 2016, Junio C Hamano wrote:

Show 9 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Seriously, do you really think it is a good idea to have
> > git_config_get_value() *ignore* any value in .git/config?
> 
> When we do not know ".git/" directory we see is the repository we were
> told to work on, then we should ignore ".git/config" file.  So allowing
> git_config_get_value() to _ignore_ ".git/config" before the program
> calls setup_git_directory() does have its uses.

I think you are wrong. This is yet another inconsistent behavior that violates the Law of Least Surprises.

Show 9 quoted lines

> > We need to fix this.
> 
> I have a feeling that "environment modifications that cannot be undone"
> we used as the rationale in 73c2779f42 ("builtin-am: implement skeletal
> builtin am", 2015-08-04) might be overly pessimistic and in order to
> implement undo_setup_git_directory(), all we need to do may just be
> matter of doing a chdir(2) back to prefix and unsetting GIT_PREFIX
> environment, but I haven't looked into details of the setup sequence
> recently.

The problem is that we may not know enough anymore to "undo setup_git_directory()", as some environment variables may have been set before calling Git. If we simply unset the environment variables, we do it incorrectly. On the other hand, the environment variables *may* have been set by setup_git_directory(). In which case we *do* have to unset them.

The entire setup_git_directory() logic is a bit of a historically-grown garden.

Ciao, Dscho