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