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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mar 3, 2017, 15:26 UTC
Message-ID: <alpine.DEB.2.20.1703031620190.3767@virtualbox>
In-Reply-To: <20170303050630.mw6asla65prku3vq@sigill.intra.peff.net>

Hi Peff,

On Fri, 3 Mar 2017, Jeff King wrote:

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

Yes, I made that change in response to a comment you made about an earlier patch in this series.

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

Well, that would be a major design change. I'm not really all that comfortable with that...

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

Exactly. There is semantic meaning in calling read_early_config() vs git_config().

Show 21 quoted lines

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

While I agree that it may sound surprising, I would like to point out that there *is* a semantic difference between `git upload-pack repo` and `git -C repo upload-pack .`: the working directory is different. And given that so much in Git's operations depend on the working directory, it makes sense that those two invocations have slightly different semantics, too.

I also agree, obviously, that this is something that would need a separate patch series to tackle ;-)

Ciao, Dscho