On Fri, Mar 12 2021, Ævar Arnfjörð Bjarmason wrote: A small correction to one of my comments: > On Thu, Mar 11 2021, Emily Shaffer wrote: > 2. You're sticking full paths in the git config key, which is > case-insensitive, and a feature of this format is being able to > configure/override previously configured hooks. > > So the behavior of this feature depends on git's interaction with > the case-sensitivity of filesystems, and not just one fs, any fs > we're walking in our various config sources, and where the hook > itself lives. > > As recent CVEs have shown that's a big can of worms, particularly > for something whose goal is to address the security aspect of > running hooks from other config. > > Arguably the case-sensitivity issue is just confusing since we > canonicalize it anyway. But once you add in FS path canonicalization > it becomes a real big can of worms. See the .gitmodules fsck code. > > Even if it wasn't for that it's relatively nastier to edit/maintain > full paths and the appropriate escaping in the double-quoted key in > the config file v.s. having it as an optionally quoted value. So the "case-insensitive" part of that *mostly* doesn't apply. I'd forgotten that we don't consider the "LeVeL" part of "ThReE.LeVeL.KeY" to be case-insensitive, but the other two components are, as discussed in git-config(1)'s docs. I say "mostly" because that's tolower()'s idea of case normalization, which may or may not match the FS's, but anyway, I think that's probably splitting hairs, but I worry more about the path normalization aspect noted in the last two paragraphs there. > 3. We're left with this "*.command = cmd", and "*.skip = true" > special-case syntax. I can't see any reason for why it's needed over > simply having "*.command = true" clobber earlier hooks as noted in > the proposed docs above. > > And that doesn't require any magic to support, just like our > existing "core.pager=cat" case. > > I mean, I suppose it's magical in that we might otherwise error on > non-consumed stdin (do we?), anyway, documenting it as a synonym for > "cat >/dev/null" would get around that :) > > 4. It makes the common case of having the same hooks for N commands > needlessly verbose, if you can just support "type" (or whatever we > should call it) you can add that N times... > > 5. At the end of this series we're left with the start of the docs > saying: > > You can list and run configured hooks with this command. Later, > you will be able to add and modify hooks with this command. > > But those patches have yet to land, and looking at the design > document I'm skeptical of that being a good addition v.s. just > adding the same thing to "git config". > > As just one exmaple; surely "git config edit <name>" would need to > run around and find config files to edit, then open them in a loop > for you, no? > > Which we'd eventually want for "git config" in general with an > --edit-regexp option or whatever, which brings us (well, at least > me) back to "then let's just add it to git-config?". > > 6. The whole 'git hook' config special-casing doesn't help other > commands or the security issue that seemed to have prompted (at > least some of) its existence > > In the design doc we mention the "core.pager = rm -rf /" case for a > .git/config. > > This series doesn't implement, but the design docs note a future > want for solving that issue for the hooks. > > To me that's another case where we should just have general config > syntax, not something hook-specific, e.g. if I could do this in my > ~/.gitconfig: > > ;; We consider 'config.ignore' in reverse order, so e.g setting > ;; it in. ~/.gitconfig will ignore any such keys for repo-level > ;; config > [config "ignore"] > key = core.pager > keyRegexp = "^hook\." > > We'd address both any hook security concerns, as well as core.pager > etc. We could then just have e.g. some syntax sugar of: > > [include] > path = built-in://gimme-safe-config > > Which would just be a thin layer of magit to include > <path-to-git-prefix>/config-templates/gimme-safe-config or whatever. > > We'd thus address the issue for all config types without > hook-specific magic. > > Anyway. I'm very willing to be convinced otherwise. I just think that > for a first-draft implementation leaving aside 'hook.<command>.command' > and the whole 'list' thing makes sense. > > We can consider the core code changes relatively separately from any > future aspirations, particularly with a 40-some patch series, and the > end-state of *this series* IMO not really justifying, that part of the > implementation, and thus requiring reviewers to look ahead beyond the > 40-some patches. Emily: *Bump* on being interesed in what you think about the rest of this though.
