git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
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>,
	"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: Thu, 16 May 2019 00:51:40 -0400	[thread overview]
Message-ID: <20190516045140.GA7241@sigill.intra.peff.net> (raw)
In-Reply-To: <20190514015928.GG7458@genre.crustytoothpaste.net>

On Tue, May 14, 2019 at 01:59:28AM +0000, brian m. carlson wrote:

> 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.

This part confuses me. In the config based scheme you describe, you have
to mention the original hook again. But isn't that exactly what creating
a symlink in your hooks directory is doing?

And in fact, I think a config based scheme can be a lot more flexible in
the end, simply because the parser _does_ see all of the hooks (whereas
in the hook directory scheme, we only ever look in one directory). So we
can have an option to sort them before running: alphabetically,
system-to-repo order, repo-to-system order, etc.

> 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.

I don't agree here. If the rule is "config takes precedence", and "if
config is absent "behave as if .git/hooks/whatever was in the config",
then the transition is easy to explain. And nobody sees any change at
all until they decide to set the config. It would probably also be
prudent to issue a warning if there's config _and_ an on-disk hook file
(or even run them both, though then you open up the question of
ordering).

Which isn't to say it's impossible for a person to get confused (new
tool sets the config, disabling their old hook?). But I think any
transition to multi-hook support (including your directory scheme) is
going to have some possibility that tools automatically setting things
up behind the scenes is going to confuse a user.

> 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?

Actually, my biggest beef with the current hooks mechanism is that it's
an _inconvenient_ place for them to live.

  - it's not version controlled, and putting a repository inside another
    repository is awkward (though it at least works for the bare case)

  - touching the filesystem is awkward and expensive if you have a large
    number of repositories whose hooks you want to update

  - it requires a manual step to modify the filesystem after a fresh
    clone (as opposed to putting things in ~/.gitconfig). Technically
    one can solve this by modifying .../share/git/templates, but that
    has its own issues.

In my world-view config-based hooks would just be run with SHELL_PATH,
just like our other config options. If you don't want one-offs in your
PATH, then use absolute paths (just like you would have to for
symlinks). If you really don't know where to put one-off hooks, perhaps
we could document a base directory from which the hook script is run,
and you could put it in a directory in .git and use a relative path? You
could even call that directory "hooks", but I suspect that would be too
confusing. :)

-Peff

  parent reply	other threads:[~2019-05-16  4:51 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
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 [this message]
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=20190516045140.GA7241@sigill.intra.peff.net \
    --to=peff@peff.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=phillip.wood123@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).