Re: [PATCH v6 00/17] hook.[ch]: new library to run hooks + simple hook conversion

[Glen Choo <chooglen@google.com>]

Ævar Arnfjörð Bjarmason         <avarab@gmail.com> writes:

> A trivial update to v5 to rebase it past conflicts with topics that
> recently landed on "master". For v5 see:
> https://lore.kernel.org/git/cover-v5-00.17-00000000000-20211123T114206Z-avarab@gmail.com/
>
> Emily Shaffer (14):
>   hook: add 'run' subcommand
>   gc: use hook library for pre-auto-gc hook
>   am: convert {pre,post}-applypatch to use hook.h
>   rebase: convert pre-rebase to use hook.h
>   am: convert applypatch-msg to use hook.h
>   merge: convert post-merge to use hook.h
>   hooks: convert non-worktree 'post-checkout' hook to hook library
>   hooks: convert worktree 'post-checkout' hook to hook library
>   send-email: use 'git hook run' for 'sendemail-validate'
>   git-p4: use 'git hook' to run hooks
>   commit: convert {pre-commit,prepare-commit-msg} hook to hook.h
>   read-cache: convert post-index-change to use hook.h
>   receive-pack: convert push-to-checkout hook to hook.h
>   run-command: remove old run_hook_{le,ve}() hook API
>
> Ævar Arnfjörð Bjarmason (3):
>   hook API: add a run_hooks() wrapper
>   hook API: add a run_hooks_l() wrapper
>   git hook run: add an --ignore-missing flag
>
>  .gitignore                 |   1 +
>  Documentation/git-hook.txt |  45 +++++++++++++
>  Documentation/githooks.txt |   4 ++
>  Makefile                   |   1 +
>  builtin.h                  |   1 +
>  builtin/am.c               |   6 +-
>  builtin/checkout.c         |   3 +-
>  builtin/clone.c            |   3 +-
>  builtin/gc.c               |   3 +-
>  builtin/hook.c             |  84 +++++++++++++++++++++++
>  builtin/merge.c            |   2 +-
>  builtin/rebase.c           |   3 +-
>  builtin/receive-pack.c     |   7 +-
>  builtin/worktree.c         |  26 +++----
>  command-list.txt           |   1 +
>  commit.c                   |  15 +++--
>  git-p4.py                  |  70 ++-----------------
>  git-send-email.perl        |  22 +++---
>  git.c                      |   1 +
>  hook.c                     | 131 ++++++++++++++++++++++++++++++++++++
>  hook.h                     |  57 ++++++++++++++++
>  read-cache.c               |   3 +-
>  reset.c                    |   3 +-
>  run-command.c              |  33 ---------
>  run-command.h              |  17 -----
>  t/t1800-hook.sh            | 134 +++++++++++++++++++++++++++++++++++++
>  t/t9001-send-email.sh      |   4 +-
>  27 files changed, 522 insertions(+), 158 deletions(-)
>  create mode 100644 Documentation/git-hook.txt
>  create mode 100644 builtin/hook.c
>  create mode 100755 t/t1800-hook.sh
>
> Range-diff against v5:
>  1:  4ca52feebb8 =  1:  ba6fd47482e hook: add 'run' subcommand
>  2:  6275b97a306 =  2:  cfba5c139e7 hook API: add a run_hooks() wrapper
>  3:  b5b3051b2e5 =  3:  a4cca074bcb gc: use hook library for pre-auto-gc hook
>  4:  c88eb5d4c25 =  4:  ce57ce1adcb am: convert {pre,post}-applypatch to use hook.h
>  5:  1d8f7b7e4c1 =  5:  d6162fbef80 hook API: add a run_hooks_l() wrapper
>  6:  d49a1444345 =  6:  4c1a8951fc5 rebase: convert pre-rebase to use hook.h
>  7:  191fdad0165 =  7:  d8aa5e8345f am: convert applypatch-msg to use hook.h
>  8:  119b92fbeae =  8:  6f8d3754b4f merge: convert post-merge to use hook.h
>  9:  359ba416e84 !  9:  d3107034806 hooks: convert non-worktree 'post-checkout' hook to hook library
>     @@ builtin/checkout.c
>       #include "ll-merge.h"
>       #include "lockfile.h"
>       #include "merge-recursive.h"
>     -@@ builtin/checkout.c: struct branch_info {
>     +@@ builtin/checkout.c: static void branch_info_release(struct branch_info *info)
>       static int post_checkout_hook(struct commit *old_commit, struct commit *new_commit,
>       			      int changed)
>       {
> 10:  b7599be95a7 ! 10:  bff7c1513ca hooks: convert worktree 'post-checkout' hook to hook library
>     @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refnam
>      -		const char *hook = find_hook("post-checkout");
>      -		if (hook) {
>      -			const char *env[] = { "GIT_DIR", "GIT_WORK_TREE", NULL };
>     --			cp.git_cmd = 0;
>     +-			struct child_process cp = CHILD_PROCESS_INIT;
>      -			cp.no_stdin = 1;
>      -			cp.stdout_to_stderr = 1;
>      -			cp.dir = path;
>     --			cp.env = env;
>     --			cp.argv = NULL;
>     +-			strvec_pushv(&cp.env_array, env);
>      -			cp.trace2_hook_name = "post-checkout";
>      -			strvec_pushl(&cp.args, absolute_path(hook),
>      -				     oid_to_hex(null_oid()),
> 11:  f1c84d7f627 = 11:  7d9c0a73568 git hook run: add an --ignore-missing flag
> 12:  4e0f94d9102 = 12:  8ea3b250dff send-email: use 'git hook run' for 'sendemail-validate'
> 13:  e858f332a62 = 13:  a184afd1ffd git-p4: use 'git hook' to run hooks
> 14:  9a5956cc028 = 14:  1a43e50617f commit: convert {pre-commit,prepare-commit-msg} hook to hook.h
> 15:  6fd47c4c499 = 15:  08b7e63ba5b read-cache: convert post-index-change to use hook.h
> 16:  b201ea46f4b = 16:  c47b36ab41a receive-pack: convert push-to-checkout hook to hook.h
> 17:  281d17b04db ! 17:  7b99a4b633c run-command: remove old run_hook_{le,ve}() hook API
>     @@ run-command.c: int async_with_fork(void)
>      -	strvec_push(&hook.args, p);
>      -	while ((p = va_arg(args, const char *)))
>      -		strvec_push(&hook.args, p);
>     --	hook.env = env;
>     +-	if (env)
>     +-		strvec_pushv(&hook.env_array, (const char **)env);
>      -	hook.no_stdin = 1;
>      -	hook.stdout_to_stderr = 1;
>      -	hook.trace2_hook_name = name;
> -- 
> 2.34.1.1146.gb52885e7c44


The Google folks picked up this series for our "review club" sessions
(which is open to the public, as you know ;)). As Junio mentioned in the
last "What's Cooking" [1], he would like serious review on the patches
you contributed, so that is where we focused most of our attention.

The code itself looks quite good, but one concern that came up was that
the motivation behind this series doesn't seem immediately obvious to
first time readers. This series has existed in some form for a very long
time; the first patches for config-based hooks I could find date back to
2019! [2]. So keeping everyone on the same page is definitely hard, but
it's worthwhile if it means that someone can pick up the series without
having to be familiar with the prior work.

With that in mind, your original cover letter [3] does a good job at
describing the "what" of the series - what it does (extend the hook API
and replace calls), what it is (an incremental restart of an ejected
topic). However, it's hard to recreate the motivation behind this series
based on the cover letter and its papertrail. Much of the Google team is
fairly familiar with the feature, but newcomers (including myself) have
a hard time piecing together the story based on this thread alone.

I found myself asking some questions when reading this cover letter, and
here are some suggestions to make this friendlier for new readers:

* Why do we want config based hooks in the first place?

  Convincing us that we want this is Emily's job, not yours, but it
  helps to know how this fits in with the whole thing. As a convenience,
  linking to Emily's original series would save the reader a lot of
  thread-following.

* How does this series contribute to config based hooks?
* Why did we need this reroll in the first place?
* How does this series meet the goals of the reroll? 

  These are pretty difficult to pin down without being familiar with the
  original series too. Emily probably knows the anwers to these, so you
  have at least one person who can review the series based on these
  questions, but it's pretty challenging for others to figure this out.

  This question is probably best answered in the cover letter, or at the
  very least, links to suggested reading and the order in which to read
  them.

* What benefit does this series bring irrespective of config based
  hooks?

  This is best answered in the cover letter as well, but the payoff is
  way bigger than the previous set of questions. If you can sell
  reviewers on the idea that this series independently mergeable, is
  good for the project on its own (I think that it is!), _and_ it
  contributes to config based hooks, then reviewers have a good reason
  to look at it even if they aren't particularly invested in config
  based hooks. I think this is even more important because we want
  reviews saying "yes this series is good for the project".

TL;DR this is basically a long-winded way of saying that for a
long-running work in progress like this one, new reviewers are probably
scared off by having to recreate the context leading up to this series,
and anything we can do to lessen that burden will be greatly
appreciated :)

[1] https://lore.kernel.org/git/xmqq1r1lfwyq.fsf@gitster.g
[2] https://lore.kernel.org/git/20191210023335.49987-1-emilyshaffer@google.com/
[3] https://lore.kernel.org/git/cover-00.13-00000000000-20211012T131934Z-avarab@gmail.com/