Re: [PATCH v8 00/37] config-based hooks

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.