Re: [PATCH v2 8/9] read_early_config(): really discover .git/
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
Re: [PATCH v2 8/9] read_early_config(): really discover .git/
On Fri, Mar 03, 2017 at 03:04:32AM +0100, Johannes Schindelin wrote: > 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
