| Message ID | cover-v6-00.17-00000000000-20211222T035755Z-avarab@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | hook.[ch]: new library to run hooks + simple hook conversion | expand |
Æ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/
On Wed, Dec 22, 2021 at 04:59:26AM +0100, Ævar Arnfjörð Bjarmason wrote: > > 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/ One note, which is not so useful for this time, but might be useful in the future: It seems that in order to understand the purpose of this series and what it's building for, you need to be me or be you (and therefore be aware of the history of the series), or you need to go back through the breadcrumbs and eventually read my huge config-based-hooks 30-patch series (or at least the cover letter). Which sort of defeats the purpose of splitting the series up into smaller reviewable chunks. Next time it might be nice to add a quick summary of the whole goal, which part of that goal this series is, and a link to the longer cover letter/older series? But again, for this time, I think it is probably too late, especially since this series looked pretty good to me. Per-patch reviews to follow momentarily. - Emily > > 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 >
On Fri, Jan 07, 2022 at 01:48:16PM -0800, Emily Shaffer wrote: > > On Wed, Dec 22, 2021 at 04:59:26AM +0100, Ævar Arnfjörð Bjarmason wrote: > > > > 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/ > > One note, which is not so useful for this time, but might be useful in > the future: > > It seems that in order to understand the purpose of this series and what > it's building for, you need to be me or be you (and therefore be aware > of the history of the series), or you need to go back through the > breadcrumbs and eventually read my huge config-based-hooks 30-patch > series (or at least the cover letter). Which sort of defeats the purpose > of splitting the series up into smaller reviewable chunks. > > Next time it might be nice to add a quick summary of the whole goal, > which part of that goal this series is, and a link to the longer cover > letter/older series? But again, for this time, I think it is probably > too late, especially since this series looked pretty good to me. > > Per-patch reviews to follow momentarily. Left a couple nits here and there, but it all looks fine for me. Some comments that I thought I had turned out to be invalid once I read the series a second time... ;) For all patches, but especially Ævar's, Reviewed-by: Emily Shaffer <emilyshaffer@google.com> I think that's stronger than an ACK for mine, but either way, for my own patches: Acked-by: Emily Shaffer <emilyshaffer@google.com> Thanks. I'd be excited to see this go in. - Emily
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;