list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Albert Cui <>
To: "Ævar Arnfjörð Bjarmason" <>
Cc: Albert Cui via GitGitGadget <>,
Subject: Re: [PATCH] hooks: propose repository owner configured hooks
Date: Mon, 5 Apr 2021 17:35:22 -0700	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Fri, Mar 19, 2021 at 3:27 AM Ævar Arnfjörð Bjarmason
<> wrote:
> On Thu, Mar 18 2021, Albert Cui via GitGitGadget wrote:
> > We propose adding native Git functionality to allow project maintainers to
> > specify hooks that a user ought to install and utilize in their development
> > workflows. This patch documents the requirements we propose for this feature
> > as well as a design sketch for implementation.
> I like this goal. It's something I proposed (off-list) before and did a
> small write-up of here:
> As far as I recall the response in the room at the time was the expected
> combination of "sure that would be neat" and "let's see the
> patches". I.e. I don't think there's hard objections to the existence of
> such a facility, but of course the devel is in the details, and
> obviously I never followed-up with actually trying to implement it.
> With config-based hooks this'll be much easier for the hook case, and
> I've tried to help that along[A]. I'd be interested in reviewing any
> effort in this area as well. The ".githooks/*" case in that proposal
> goes away with config-based hooks (since they wouldn't be special
> anymore, just config).

Thanks for the context and the support. Agreed, I think this is a
natural evolution of config-based hooks.

> > +* As part of the fetch subcommand, Git prompts users to install the configs
> > +contained there.
> > +
> > +    ** User responses to that prompt could be "sticky" - e.g. a user could reply
> > +    "no (this time)", "no (never)", "yes (this time)", or "yes (always)".
> > +    Always/never indicate that the user trusts the remote this config is coming
> > +    from, and should not apply to configs fetched from other remotes.
> As noted in the proposal I linked I think anyone thinking about this
> would do well to examine the trade-off Emacs's implementation of this
> uses, since it manages to safely open arbitrary files that'll
> potentially run arbitrary code on-open, but only if the user opts-in.

In [0], I proposed only allowing pre-clone opt-in to suggested hooks
if allowlisted in the config for the given remote, which seems similar
to your previous proposal. Extending this idea to any config settings
seems very reasonable. I'd love other people's thoughts about this.

In your proposal, you wrote:

> It would work really well with includeIf, e.g. I could clone all my work repos to a "safe" area in ~/work which is allowed to set more options, e.g. aliases.

It seems much safer to do this for a given remote, opposed to a local
file path, no?

> > +Later, we might want to do this before the initial clone is performed; that
> > +workflow looks like:
> > +
> > +* During clone, perform ls-refs as normal
> > +
> > +* If the server has a "magic" config branch, fetch only that config branch.
> ...the reason for the magic branch is this before-clone use-case?
> Unless there's a really strong use-case for that I'd think the
> per-branch .gitconfig would be a better trade-off, but even then there
> would be ways to make that work obviously in the many-many case, and
> still e.g. have a branch advertise a config blob for its "main" branch
> as a special case or something.
> I also suspect an unstated constraint of having this in a magic branch
> is the limitation of some git hosting provider's ACL
> implementations. More on that later...

Mentioned in [0], the primary motivation for a magic config branch
that lived outside of the worktree was "since the configuration is
separate from the code base, it allows you to go back in history or to
older branches while preserving "improvements" to the hooks
configuration e.g. maybe the project has shifted to using a newer
version of a linter."

> > +Future Work
> > +~~~~~~~~~~~
> > +
> > +* Extending this to allow repository owners to specify specific configurations
> > +in general e.g. this repository should use partial-clone with these parameters.
> I don't see why such a proposal should narrowly confine itself to hooks.
> Once we have config-based hooks then hooks are just configuration.
> If we're going to pick up arbitrary configuration from remotes/repos on
> request then we'd be starting with the most dangerous configuration.

Summarizing from [0]: Based on this feedback, I'm hearing "we should
have a design for suggested configuration in general," but I don't
think that necessitates that we should pursue generic configuration
before hooks configuration.

> I think a better way to start such an effort incrementally would be:
> 1. Audit git's config parser for the safety of parsing arbitrary config,
>    we parse .gitmodules now, do we feel it's safe enough to parse
>    arbitrary config (probably, but worth checking).
> 2. Add reflection to git's own config variables. Right now we have this
>    in the binary generated via a grep from the documentation. Similar to
>    Emacs's implementation we should/could categorize all our known
>    config variables by safety.

To clarify, are you saying, today, git's config variables are pulled
from the documentation? I.e. the documentation is the source of truth
for what config variables are supported? o.0

>    Hooks are the least safe, something like a diff.context=N setting the
>    repo wants to suggest a -U<n> setting much safer (just tweaking our
>    existing diff algorithm) etc. But even in those cases we'd want to
>    proceed slowly, e.g. is that config parsing for that -U<n> defensive
>    enough to be safe for arbitrary data?

To clarify, this proposal is just to set the hooks config that
config-based hooks enabled e.g. running `git config --add
hook.pre-commit.command pylint` on behalf of the user, so I'm not sure
what "arbitrary data" you're referring to. At least, any problems I'd
think we'd already address with config-based hooks.

> 3. Users should be able to e.g. configure "yes, for any repo I clone
>    they can tweak 'safe'" variables. This would reduce user dialog
>    fatigue, and thus increase safety. I.e. a repo who wants to set a
>    thing like hooks would stand out, but something that e.g. wants to
>    change the diff order would pass on existing global configuration.
> 4. Smarter ACL that magic remote+magic branch: It seems like an obvious
>    thing to me to want that if I clone e.g. a random clone of git.git
>    that I'd want to setup config for it IFF the .gitconfig in it is
>    reachable from a tag GPG signed by <approved key>.
>    Git's a distributed system, so while I don't think it's bad to have
>    some feature like "I always trust config from this remote" (e.g. a
>    corporate environment where you know its .gitconfig is
>    guarded/audited) we should think about more distributed-friendly
>    solutions first and if possible guide users towards those.

This seems like an OK alternative to allow-listing based on remote,
but are you expecting users to add a GPG key to their .gitconfig?
Remote URLs seem much more user friendly (think IP address vs URL).


  reply	other threads:[~2021-04-06  0:35 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-18 22:03 Albert Cui via GitGitGadget
2021-03-18 22:29 ` Junio C Hamano
2021-03-18 23:45   ` Albert Cui
2021-03-19  1:28 ` brian m. carlson
2021-03-19 10:27 ` Ævar Arnfjörð Bjarmason
2021-04-06  0:35   ` Albert Cui [this message]
2021-04-07 22:47     ` Ævar Arnfjörð Bjarmason
2021-06-21 19:36       ` Jonathan Tan
2021-06-21 20:35         ` Ævar Arnfjörð Bjarmason
2021-03-26  1:43 ` [PATCH v2] hooks: propose project " Albert Cui via GitGitGadget
2021-03-29 23:20   ` Emily Shaffer
2021-04-01 20:02     ` Albert Cui
2021-03-30 15:24   ` Derrick Stolee
2021-04-05 22:45     ` Albert Cui
2021-04-05 23:09       ` Junio C Hamano
2021-04-05 23:40         ` Albert Cui
2021-04-06  0:13           ` Junio C Hamano
2021-04-06  0:27             ` Albert Cui
2021-04-06 23:15       ` brian m. carlson
2021-04-07  7:53         ` Ævar Arnfjörð Bjarmason
2021-04-07 13:09           ` Derrick Stolee
2021-04-07 18:40             ` Albert Cui
2021-04-07 20:02               ` Junio C Hamano
2021-04-07 22:23                 ` Ævar Arnfjörð Bjarmason
2021-04-15 16:52             ` Ed Maste
2021-04-15 19:41               ` Junio C Hamano
2021-04-15 20:37                 ` Ed Maste
2021-04-15 20:50                   ` Junio C Hamano
2021-04-15 22:28                   ` brian m. carlson
2021-04-02  9:59   ` Ævar Arnfjörð Bjarmason
2021-04-05 23:42     ` Albert Cui
2021-04-02 10:30   ` Ævar Arnfjörð Bjarmason
2021-04-03  0:58     ` Albert Cui
2021-04-24  1:38   ` [PATCH v3] " Albert Cui via GitGitGadget
2021-04-28  2:48     ` Junio C Hamano
2021-05-05 19:11     ` [PATCH v4] " Albert Cui via GitGitGadget
2021-06-03  3:31       ` Jonathan Tan
2021-06-03 20:16         ` Albert Cui
2021-06-03 22:10           ` Jonathan Tan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \
    --subject='Re: [PATCH] hooks: propose repository owner configured hooks' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).