Re: [PATCH v2 8/9] read_early_config(): really discover .git/

From: Jeff King <peff@peff.net>
Date: Mar 3, 2017, 05:06 UTC
Message-ID: <20170303050630.mw6asla65prku3vq@sigill.intra.peff.net>
In-Reply-To: <921faef822901715fa877d6969255ce00d80b925.1488506615.git.johannes.schindelin@gmx.de>

On Fri, Mar 03, 2017 at 03:04:32AM +0100, Johannes Schindelin wrote:

Show 16 quoted lines

> Earlier, we punted and simply assumed that we are in the top-level
> directory of the project, and that there is no .git file but a .git/
> directory so that we can read directly from .git/config.
> 
> However, that is not necessarily true. We may be in a subdirectory. Or
> .git may be a gitfile. Or the environment variable GIT_DIR may be set.
> 
> To remedy this situation, we just refactored the way
> setup_git_directory() discovers the .git/ directory, to make it
> reusable, and more importantly, to leave all global variables and the
> current working directory alone.
> 
> Let's discover the .git/ directory correctly in read_early_config() by
> using that new function.
> 
> This fixes 4 known breakages in t7006.

Yay, this is much nicer.

I think the "dirty hack..." comment in read_early_config() can be condensed (or go away) now.

I think we _could_ do away with read_early_config() entirely, and just have the regular config code do this lookup when we're not already in a repo. But then we'd really need to depend on the "creating_repository" flag being managed correctly.

I think I prefer the idea that a few "early" spots like pager and alias config need to use this special function to access the config. That's less likely to cause surprises when some config option is picked up before we have run setup_git_directory().

There is one surprising case that I think we need to deal with even now, though. If I do:

  git init repo
  git -C repo config pager.upload-pack 'echo whoops'
  git upload-pack repo
  cd repo && git upload-pack .

the first one is OK, but the second reads and executes the pager config from the repo, even though we usually consider upload-pack to be OK to run in an untrusted repo. This _isn't_ a new thing in your patch, but just something I noticed while we are here.

And maybe it is a non-issue. The early-config bits all happen via the git wrapper, and normally we use the direct dashed "git-upload-pack" to access the other repositories. Certainly I have been known to use "git -c ... upload-pack" while debugging various things.

So I dunno. You really have to jump through some hoops for it to matter, but I just wonder if the git wrapper should be special-casing upload-pack, too.

-Peff