Re: [PATCH v6 0/5] config: introduce discovery.bare and protected config

On Tue, Jul 12 2022, Glen Choo wrote:

Yes, I have not been following up here to merely repeat what's been said before, but...

...have been following up because it's still genuinely unclear to me what data or design constraints led to this solution. I.e. in [1] you noted ("[...]" interjection is mine):

	"I don't see how we could implement this [the "walk-up" method]
	without imposing a big penalty to all bare repo users[...]."

[Continued below]

I read that thread over again, and some of the highlights were:

 * brian asking if we can't basically do the "walk up" method:
   https://lore.kernel.org/git/Yk9hONuCIVIq6ieV@camp.crustytoothpaste.net/

 * Taylor wondering how much we need to worry about this attack (among
   other things) & worrying about legitimate "bare repo" workflows being
   broken: https://lore.kernel.org/git/YloTQH35r2xVdPm1@nand.local/ &
   https://lore.kernel.org/git/Ylobp7sntKeWTLDX@nand.local/

But most importantly, here's something I hadn't noticed before:

 * Emily talking about the supposed slowness of the "walk up" method:
   https://lore.kernel.org/git/CAJoAoZkgnnvdymuBsM9Ja3+eYSnyohr=FQZMVX_uzZ_pkQhgaw@mail.gmail.com/

I.e.:

	"wantonly scanning up the filesystem for any gitdir above the
	current one is really expensive. When I tried that approach for
	the purposes of including some shared config between
	superproject and submodules, it slowed down the Git test suite
	by something like 3-5x."

Which I'm now 99.99% certain based on past context[2] is a misstatement or misrecollection about an early version of submodule.superprojectGitDir v.s. what setup.c would do.

I.e. that 3-5x slowness referred to git-submodule.sh shelling out to "git rev-parse", it's not a reference to the expense of the few syscalls we'd need to make to discover a parent git directory.

Did you hear about the directory walking being a performance concern from Emily, or was it an independent discovery?

It seems as though this might have come about because of a misrecollection about the git-rev-parse(1)/git-submodule.sh v.s. setup.c performance with reference to submodule.superprojectGitDir, and that we've now got a design that's optimized to avoid a performance problem that doesn't exist, at the cost of accuracy.

And not to reiterate, but I think the performance isn't a concern per-se, but rather that performance concerns seem to have driven one design over another.

I saw some implementation discussion about how we'd do this with fsck, which is one thing, but I don't really see the trickyness or ambiguity on the client side.

I.e. we know when we'd "find a repo", so that's the criteria we'd use to ignore such a contained repo or not. The only trickyness seems to come about if the approach we pick is one where we conflate embedded bare repos v.s. non-embedded bare repos.

It really just seems like a dead end to me, sorry.

I.e. we know what the security problem is, but the side-effects of this approach are such that we'll probably never turn it on by default.

So that'll mean that the vast majority of users who could benefit from the security mitigation won't even know about the config, or if they do might not have it turned on.

And yes, we might end up with a better design later, but then we'll have to still support this config mechanism, potentially deprecate it etc.

1. https://lore.kernel.org/git/kl6lee1z8mcm.fsf@chooglen-macbookpro.roam.corp.google.com/
2. https://lore.kernel.org/git/211109.86v912dtfw.gmgdl@evledraar.gmail.com/