git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "brian m. carlson" <sandals@crustytoothpaste.net>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: git@vger.kernel.org, "Jeff King" <peff@peff.net>,
	"Duy Nguyen" <pclouds@gmail.com>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Junio C Hamano" <gitster@pobox.com>,
	"Johannes Sixt" <j6t@kdbg.org>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Phillip Wood" <phillip.wood123@gmail.com>
Subject: Re: [PATCH v2 0/7] Multiple hook support
Date: Tue, 14 May 2019 01:59:28 +0000	[thread overview]
Message-ID: <20190514015928.GG7458@genre.crustytoothpaste.net> (raw)
In-Reply-To: <20190514004920.GB136746@google.com>

[-- Attachment #1: Type: text/plain, Size: 4222 bytes --]

On Mon, May 13, 2019 at 05:51:01PM -0700, Jonathan Nieder wrote:
> Hi,
> 
> brian m. carlson wrote:
> 
> > I've thought a lot about the discussion over whether this series should
> > use the configuration as the source for multiple hooks. Ultimately, I've
> > come to the decision that it's not a good idea. Even adopting the empty
> > entry as a reset marker, the fact that inheritance in the configuration
> > is in-order and can't be easily modified means that it's not likely to
> > be very useful, but it is likely to be quite surprising for the average
> > user.
> 
> Can we discuss this some more?  What would it take to make it likely
> to be useful in your view?

There are two aspects here which I think are worth discussing. Let's
discuss the inheritance issue first.

Order with multiple hooks matters. The best hook as an example for this
is prepare-commit-msg. If I have a hook which I want to run on every
repository, such as a hook that inserts some sort of ID (bug tracker,
Gerrit, etc.), that hook, due to inheritance, *has* to be first, before
any other prepare-commit-msg hooks. If I want a hook that runs before
it, perhaps because a particular repository needs a different
configuration, I have to wipe the list and insert both hooks. I'm now
maintaining two separate locations for the command lines instead of just
inserting a symlink to the global hook and dropping a new hook before
it.

I don't think there's a good way to make it easier unless we radically
customize the way configuration is done. I don't doubt that there are a
small number of configurations where the inheritance behavior is fine—I
believe GitHub's backend is one of them—but overall I think it's hard to
reason about and customize.

The second issue here is that it's surprising. Users don't know how to
reset a configuration option because we don't have a consistent way to
do that. Users will not expect for there to be multiple ways to set
hooks. Users will also not expect that their hooks in their
configuration aren't run if there are hooks in .git/hooks. Tooling that
has so far used .git/hooks will compete with users' global configuration
options, which I guarantee you will be a surprise for users using older
versions of tools.

The new behavior, which puts everything in the same directory
(.git/hooks) is much easier to reason about. I think we're obligated to
consider the experience for the average end user, who may not be
intimately familiar with how Git works but still needs to use it to get
work done.

It also provides a convenient place for hooks to live, which a
config-based option doesn't. We'll need to invoke things using /bin/sh,
so will they all have to live in PATH? What about one-offs that don't
really belong in PATH?

> > I think a solution that sticks with the existing model and builds
> > off a design used by other systems people are familiar with, like cron
> > and run-parts, is going to be a better choice. Moreover, this is the
> > design that people have already built with outside tooling, which is a
> > further argument in favor of it.
> 
> To be clear, the main advantage I see for config versus the .git/hooks
> model is that with the config model, a user doesn't have to search
> throughout the filesystem for .git/hooks directories to update when a
> hook is broken.

I agree this is an advantage if they don't hit the ordering issue. I
think a lot of the common use cases where this approach has benefits can
be handled well with core.hooksPath and hooks that can turn themselves
on or off depending on the repository config.

What might be an interesting approach that would address these concerns
is a core.globalHooks[0] option that points to a set (or sets,
depending) of multiple-hook directories. We then enumerate hooks in sort
order, considering both the global and the local directories as one
unit, perhaps with some way of disabling hooks. I'm not planning on
working on this myself, but I wouldn't be opposed to seeing someone else
work on it.

[0] Better name suggestions are, of course, welcome.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

  reply	other threads:[~2019-05-14  1:59 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-14  0:23 [PATCH v2 0/7] Multiple hook support brian m. carlson
2019-05-14  0:23 ` [PATCH v2 1/7] run-command: add preliminary support for multiple hooks brian m. carlson
2019-05-14 12:46   ` Duy Nguyen
2019-05-15 22:27     ` brian m. carlson
2019-05-29  2:18       ` brian m. carlson
2019-05-14 15:12   ` Johannes Schindelin
2019-05-15 22:44     ` brian m. carlson
2019-05-16 19:11       ` Johannes Sixt
2019-05-17 20:31         ` Johannes Schindelin
2019-05-14  0:23 ` [PATCH v2 2/7] builtin/receive-pack: add " brian m. carlson
2019-05-14  0:23 ` [PATCH v2 3/7] rebase: " brian m. carlson
2019-05-14 12:56   ` Duy Nguyen
2019-05-14 17:58     ` Johannes Sixt
2019-05-15 22:55     ` brian m. carlson
2019-05-16 10:29       ` Duy Nguyen
2019-05-14  0:23 ` [PATCH v2 3/7] sequencer: " brian m. carlson
2019-05-14  0:23 ` [PATCH v2 4/7] builtin/worktree: add support for multiple post-checkout hooks brian m. carlson
2019-05-14  0:23 ` [PATCH v2 5/7] transport: add support for multiple pre-push hooks brian m. carlson
2019-05-14  0:23 ` [PATCH v2 6/7] config: allow configuration of multiple hook error behavior brian m. carlson
2019-05-14 13:20   ` Duy Nguyen
2019-05-15 23:10     ` brian m. carlson
2019-05-16  5:08       ` Jeff King
2019-05-16  5:02   ` Jeff King
2019-05-16 17:19     ` brian m. carlson
2019-05-16 21:52       ` Jeff King
2019-05-14  0:23 ` [PATCH v2 7/7] docs: document multiple hooks brian m. carlson
2019-05-14 13:38   ` Duy Nguyen
2019-05-14  0:51 ` [PATCH v2 0/7] Multiple hook support Jonathan Nieder
2019-05-14  1:59   ` brian m. carlson [this message]
2019-05-14  2:26     ` Jonathan Nieder
2019-05-16  0:42       ` brian m. carlson
2019-05-16  0:51         ` Jonathan Nieder
2019-05-16  4:51     ` Jeff King
2019-05-14 13:30 ` Duy Nguyen

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=20190514015928.GG7458@genre.crustytoothpaste.net \
    --to=sandals@crustytoothpaste.net \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=j6t@kdbg.org \
    --cc=jrnieder@gmail.com \
    --cc=pclouds@gmail.com \
    --cc=peff@peff.net \
    --cc=phillip.wood123@gmail.com \
    /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).