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

From: Jeff King <peff@peff.net>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: Junio C Hamano <gitster@pobox.com>,
	git@vger.kernel.org, David Aguilar <davvid@gmail.com>,
	Dennis Kaarsemaker <dennis@kaarsemaker.net>
Subject: Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
Date: Tue, 6 Dec 2016 08:36:50 -0500	[thread overview]
Message-ID: <20161206133650.t7gkg4f6wzw3zxki@sigill.intra.peff.net> (raw)
In-Reply-To: <alpine.DEB.2.20.1612061411000.117539@virtualbox>

On Tue, Dec 06, 2016 at 02:16:35PM +0100, Johannes Schindelin 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.

There are surprises in this code any way you turn.  If we have not
called setup_git_directory(), then how does git_config_get_value() know
if we are in a repository or not?

Should it blindly look at ".git/config"? Now your program behaves
differently depending on whether you are in the top-level of the working
tree.

Should it speculatively do repo discovery, and use any discovered config
file? Now some commands respect config that they shouldn't (e.g.,
running "git init foo.git" from inside another repository will
accidentally pick up the value of core.sharedrepository from wherever
you happen to run it).

So I think the caller of the config code has to provide some kind of
context about how it is expecting to run and how the value will be used.

Right now if setup_git_directory() or similar hasn't been called, the
config code does not look. Ideally there would be a way for a caller to
say "I am running early and not even sure yet if we are in a repo;
please speculatively try to find repo config for me".

The pager code does this manually, and without great accuracy; see the
hack in pager.c's read_early_config(). I think the way forward is:

  1. Make that an optional behavior in git_config_with_options() so
     other spots can reuse it (probably alias lookup, and something like
     your difftool config).

  2. Make it more accurate. Right now it blindly looks in .git/config,
     but it should be able to do the usual repo-detection (_without_
     actually entering the repo) to try to find a possible config file.

-Peff