From: Jeff King <peff@peff.net>
To: "brian m. carlson" <sandals@crustytoothpaste.net>
Cc: Jonathan Nieder <jrnieder@gmail.com>,
git@vger.kernel.org, Duy Nguyen <pclouds@gmail.com>,
Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH 0/5] Multiple hook support
Date: Tue, 30 Apr 2019 17:39:02 -0400 [thread overview]
Message-ID: <20190430213902.GC16290@sigill.intra.peff.net> (raw)
In-Reply-To: <20190424004948.728326-1-sandals@crustytoothpaste.net>
On Wed, Apr 24, 2019 at 12:49:43AM +0000, brian m. carlson wrote:
> I've talked with some people about this approach, and they've indicated
> they would prefer a configuration-based approach.
I think I'm some people. :)
I agree with the thoughts that Jonathan pointed out in [1], but I wanted
to raise a few points that are more directly related to hook features:
1. Config is resolved at run-time, making it much easier to have
system or user-level hooks (as opposed to our current system of
on-disk files, which require copying or symlinking hooks ahead of
time into each repository you want to impact).
2. Config values let you easily run hooks from multiple sources (e.g.,
a hook specified in /etc/gitconfig, one in ~/.gitconfig, and then a
repo-level hook in .git/config). Even with a "hook.d" feature like
this, you are back to doing lots of symlinks within the ".d"
directory to get this behavior.
I specifically worry that adding ".d" directories is a step in the
wrong direction because our solution will probably make this point
_worse_ than whatever custom trampolines people are already using.
3. A well-designed config schema can leave room for more
configuration. E.g., one of the big questions with multi-hooks is
the error semantics. But what if we had:
[hook "pre-receive"]
command = my-hook-cmd
command = another-hook-cmd
# stop running and return failure at first non-zero exit
errorBehavior = stop-on-first
# ...or run all and return error if _any_ failed
errorBehavior = report-any-error
# ...or run and report if any _accepted_
errorBehavior = report-any-success
Those are just off the top of my head. But my point is that by
staking out a config section for each hook, it gives us a place to
naturally add new config options. And we can do it on a per-hook
basis, which I think will be important since each hook has its own
semantics.
Now that's not _strictly_ necessary. We could still have
"hook.pre-receive.errorBehavior" and just assume
"hook.pre-receive.command" is "$GIT_DIR/hooks/pre-receive". But I
think doing the whole thing from config makes the behavior simple
and consistent (and the backwards compatibility is easy -- if they
aren't using the new command config, we really do behave "as if"
they had set it to the file in the hooks directory).
So I agree with your general sentiment that the multi-hook support
is conceptually orthogonal to switching to a config-based system. But I
think it's worth considering whether we want to do something
config-based first:
- if we introduce it later, it saves us from having _three_ ways to do
the same thing
- I think it provides a more natural way to express the options that
will inevitably grow once we have multi-hook support
-Peff
[1] https://public-inbox.org/git/20190424023438.GE98980@google.com/
prev parent reply other threads:[~2019-04-30 21:39 UTC|newest]
Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-24 0:49 [PATCH 0/5] Multiple hook support brian m. carlson
2019-04-24 0:49 ` [PATCH 1/5] run-command: add preliminary support for multiple hooks brian m. carlson
2019-04-24 2:27 ` Junio C Hamano
2019-04-24 18:48 ` Johannes Sixt
2019-04-25 0:55 ` Junio C Hamano
2019-04-25 9:39 ` Ævar Arnfjörð Bjarmason
2019-04-25 10:04 ` Junio C Hamano
2019-04-25 19:40 ` Johannes Sixt
2019-04-26 20:58 ` brian m. carlson
2019-04-26 21:53 ` Johannes Sixt
2019-04-24 22:32 ` brian m. carlson
2019-04-24 0:49 ` [PATCH 2/5] builtin/receive-pack: add " brian m. carlson
2019-04-24 0:49 ` [PATCH 3/5] sequencer: " brian m. carlson
2019-04-24 9:51 ` Phillip Wood
2019-04-24 22:46 ` brian m. carlson
2019-04-25 14:59 ` Phillip Wood
2019-04-24 0:49 ` [PATCH 4/5] builtin/worktree: add support for multiple post-checkout hooks brian m. carlson
2019-04-24 0:49 ` [PATCH 5/5] transport: add support for multiple pre-push hooks brian m. carlson
2019-04-24 2:09 ` [PATCH 0/5] Multiple hook support Junio C Hamano
2019-04-24 2:22 ` brian m. carlson
2019-04-24 2:41 ` Junio C Hamano
2019-04-24 8:14 ` Ævar Arnfjörð Bjarmason
2019-04-24 2:34 ` Jonathan Nieder
2019-04-24 7:43 ` Ævar Arnfjörð Bjarmason
2019-04-24 8:22 ` Ævar Arnfjörð Bjarmason
2019-04-24 23:07 ` brian m. carlson
2019-04-24 23:26 ` Jonathan Nieder
2019-04-25 10:08 ` How to undo previously set configuration? (again) Ævar Arnfjörð Bjarmason
2019-04-25 10:43 ` Duy Nguyen
2019-04-25 11:58 ` Ævar Arnfjörð Bjarmason
2019-04-26 15:18 ` Ævar Arnfjörð Bjarmason
2019-04-25 14:36 ` Jonathan Nieder
2019-04-25 14:43 ` Barret Rhoden
2019-04-25 15:27 ` Ævar Arnfjörð Bjarmason
2019-04-25 15:25 ` Ævar Arnfjörð Bjarmason
2019-04-26 2:13 ` Junio C Hamano
2019-04-26 9:36 ` Duy Nguyen
2019-04-30 21:14 ` Jeff King
2019-05-01 11:41 ` Duy Nguyen
2019-05-01 12:18 ` Ævar Arnfjörð Bjarmason
2019-05-01 12:32 ` Duy Nguyen
2019-05-01 21:09 ` Jeff King
2019-05-01 21:15 ` Jeff King
2019-04-24 8:10 ` [PATCH 0/5] Multiple hook support Ævar Arnfjörð Bjarmason
2019-04-24 9:55 ` Phillip Wood
2019-04-24 18:29 ` Bryan Turner
2019-04-24 9:49 ` Duy Nguyen
2019-04-24 22:49 ` brian m. carlson
2019-04-24 23:40 ` brian m. carlson
2019-04-25 0:08 ` Duy Nguyen
2019-04-30 21:39 ` Jeff King [this message]
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:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190430213902.GC16290@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=Johannes.Schindelin@gmx.de \
--cc=git@vger.kernel.org \
--cc=jrnieder@gmail.com \
--cc=pclouds@gmail.com \
--cc=sandals@crustytoothpaste.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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).