Re: [PATCH v7 01/17] doc: propose hooks managed by the config

From: Emily Shaffer <emilyshaffer@google.com>
Date: Mar 10, 2021, 19:30 UTC
Message-ID: <YEkeTB5MPjXJQRrO@google.com>
In-Reply-To: <xmqqa6snjv4m.fsf@gitster.c.googlers.com>

On Mon, Feb 01, 2021 at 02:11:53PM -0800, Junio C Hamano wrote:

Show 15 quoted lines

> 
> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > +ways, providing an avenue to deprecate these "legacy" hooks if desired. The
> > +handling is based on a config `hook.runHookDir`, which is checked against a
> > +number of cases:
> 
> Don't we want to also warn when the setting "no" or something
> similar prevents the legacy hook from running, to help users
> who wonder why their hook scripts are not running?  I.e.
> 
> > +- "no": the legacy hook will not be run
> 
> +- "warn-no": Git will print a warning to stderr before ignoring the
> +  legacy hook

Yeah, I think you are right. Jonathan N suggested such an option as "error" (as opposed to warning); I'll add it here plus to the enum and take your description verbatim. Thanks.

Show 13 quoted lines

> 
> > +- "interactive": Git will prompt the user before running the legacy hook
> > +- "warn": Git will print a warning to stderr before running the legacy hook
> > +- "yes" (default): Git will silently run the legacy hook
> 
> > +In case this list is expanded in the future, if a value for `hook.runHookDir` is
> > +given which Git does not recognize, Git should discard that config entry. For
> > +example, if "warn" was specified at system level and "junk" was specified at
> > +global level, Git would resolve the value to "warn"; if the only time the config
> > +was set was to "junk", Git would use the default value of "yes".
> 
> Hmph, instead of complaining "value 'junk' is not recognized" and
> erroring out?  Why?

I think for the exact case you're describing above - where I forgot some useful combination of "run/don't run" and "warn/don't warn" (or, one could forsee, "first" e.g. to run the legacy hook early on instead of late, or "only" e.g. config hooks don't exist, etc etc), we add this flag, and some enterprise (like Jonathan N's team) distributes the new config to folks' system configs before 100% of machines are using the newer Git executable. It's a long shot :) but I'd rather be flexible than inflexible.

I will update the design doc anyway - I ended up implementing it as "complain about 'junk' and then do default behavior".

Show 16 quoted lines

> 
> > +[[stage-3]]
> > +==== Stage 3
> > +
> > +`.git/hooks` is removed from the template and the hook directory is considered
> > +deprecated. To avoid breaking older repos, the default of `hook.runHookDir` is
> > +not changed, and `find_hook()` is not removed.
> 
> Presumably, we'll have documentation somewhere that instructs users
> (who were taught by slashdot and other site to add certain scripts
> under their .git/hooks/) how to do the equivalent without adding
> scripts in .git/hooks/ directory and instead using the config
> mechanism (e.g. "when told to add script X in .git/hooks/, read such
> an instruction as if telling you to do Y instead") by the time this
> happens?  It probably makes sense to do so as part of stage-2, at
> which point the users are _ready_ to migrate.

Hm. Where should we distribute such a documentation? git-hook.txt? githooks.txt? I think it's a good idea, sure.

Show 19 quoted lines

> 
> > +[[security]]
> > +=== Security and repo config
> > +
> > +Part of the motivation behind this refactor is to mitigate hooks as an attack
> > +vector;footnote:[https://lore.kernel.org/git/20171002234517.GV19555@aiede.mtv.corp.google.com/]
> > +however, as the design stands, users can still provide hooks in the repo-level
> > +config, which is included when a repo is zipped and sent elsewhere.  The
> > +security of the repo-level config is still under discussion; this design
> > +generally assumes the repo-level config is secure, which is not true yet. The
> > +goal is to avoid an overcomplicated design to work around a problem which has
> > +ceased to exist.
> 
> I doubt we want to claim anything about security as part of this
> series.  As you say in the paragraph, .git/config and .git/hooks/
> are equally (un)protected and if we decide to punt .git/config
> security, then not moving away from .git/hooks would not hurt
> security-wise, either (in other words, security is not a viable
> motivation behind this series).

Yeah, I think you are right that despite my best efforts to spin it that way, it just isn't a security win :) Ah well...

Show 8 quoted lines

> 
> And if we stop advertising 'security merit' that does not exist,
> what remains?  Isn't the biggest selling point that an identical set
> of hook configuration can be shared among multiple repositories, and
> it allows more than one hook scripts to be triggered by a single
> "hook event"?  There may be other good things we should be able to
> sell the new mechanism to our users, and we do stress on them, which
> is done in the motivation section.  So...

Ok.

I would like to keep this header and make it more clear that we didn't fully succeed in the wish to make zip attacks impossible, but I will remove it from the motivations section at the top. I also added a little more clarity (I hope) on the other pieces in the motivation section.

Show 15 quoted lines

> 
> > +.Comparison of alternatives
> > +|===
> > +|Feature |Config-based hooks |Hook directories |Status quo
> 
> Sorry, but I did not find this table particularly convincing.
> 
> The only thing I sense is a hand-wavy desire that "we could make it
> better than everybody else if we work on it in this area", which can
> apply equally for other approaches---they could enhance what they
> already have (e.g. "discoverability & documentation").
> 
> As a list of "these are the points we aspire to do better than other
> people", I think it is an excellent idea to have a table like this
> here in the documentation.  But that is not a "comparison".

Ok. I'll see what I can do to frame it better.

Show 27 quoted lines

> 
> > +[[execution-ordering]]
> > +=== Execution ordering
> > +
> > +We may find that config order is insufficient for some users; for example,
> > +config order makes it difficult to add a new hook to the system or global config
> > +which runs at the end of the hook list. A new ordering schema should be:
> > +
> > +1) Specified by a `hook.order` config, so that users will not unexpectedly see
> > +their order change;
> > +
> > +2) Either dependency or numerically based.
> > +
> > +Dependency-based ordering is prone to classic linked-list problems, like a
> > +cycles and handling of missing dependencies. But, it paves the way for enabling
> > +parallelization if some tasks truly depend on others.
> > +
> > +Numerical ordering makes it tricky for Git to generate suggested ordering
> > +numbers for each command, but is easy to determine a definitive order.
> 
> OK.
> 
> Have we decided what we do for hooks whose interface is to feed
> their input from their standard input?  The current system, I think,
> just feeds the single hook by writing into a pipe to it, but if we
> were to drive multiple hooks, we'd need to write the same thing to
> each of these hook programs?

Yep. There are a few ways to do it:
1. Provide a file to be used as stdin. This way we just hook up that
file's fd to each hook process's stdin fd. (Example:
https://lore.kernel.org/git/20201222000435.1529768-11-emilyshaffer@google.com
in builtin/am.c:run_post_rewrite_hook())
2. Provide a string_list in the run_hooks_opt struct; hook.c treats each
entry in the string_list as a line to stdin, separated by a newline, and
"replays" the list to each hook in order. (Example:
https://lore.kernel.org/git/20201222000435.1529768-12-emilyshaffer@google.com)
3. Set up your own more complicated version of (2) by writing your own
callback to provide "next line of stdin" based on a context pointer. The
context pointer is per-hook-task. (Example:
https://lore.kernel.org/git/20201222000435.1529768-17-emilyshaffer@google.com)

Show 7 quoted lines

> 
> Do we have a plan to deal with hooks whose outcome is not just
> "yes/no", e.g. "proc-receive" hook that munges the list of refs to
> be updated and the new values for them, or "applypatch-msg" that
> munges the incoming proposed commit log message?  Does the second
> hook work on the result of the first hook?  Do the two hooks work on
> the vanilla state and their output have to agree with each other?

The only hook I found this way was 'proc-receive' itself - and in the end, because it's somewhat interactive with two-way communication with the caller, it didn't seem like there was a good way to reason about multiple hooks, like you say. So in the 'proc-receive' case, we disallow multiple hooks. See https://lore.kernel.org/git/20201222000435.1529768-15-emilyshaffer@google.com.

Show 15 quoted lines

> 
> > +[[parallelization]]
> > +=== Parallelization with dependencies
> > +
> > +Currently hooks use a naive parallelization scheme or are run in series.  But if
> > +one hook depends on another's output, then users will want to specify those
> > +dependencies.
> 
> An untold assumption here is that the questions I asked earlier on
> having more than one hooks that is not just yes/no is something
> readers know the same answer, and that answer is "the outcome of the
> first hook is passed along (as if it were the input given by Git
> directly, if the first hook did not exist), to the second hook.  It
> should be spelled out somewhere before the execution ordering
> section, I think.

I'll make an explicit callout about that case, thanks.

 - Emily