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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

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

> 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.

> > 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