{"thread":{"id":"51084","subject":"[PATCH v2 0/7] Multiple hook support","startedAt":"2019-05-14T00:23:51Z","lastAt":"2019-05-29T02:18:29Z","messageCount":34,"participants":["brian m. carlson","Jonathan Nieder","Duy Nguyen","Johannes Schindelin","Johannes Sixt","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":7},"messages":[{"id":"375444","messageId":"20190514002332.121089-1-sandals@crustytoothpaste.net","threadId":"51084","inReplyTo":null,"subject":"[PATCH v2 0/7] Multiple hook support","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-05-14T00:23:24Z","receivedAt":"2019-05-14T00:23:51Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"This series introduces multiple hook support.\n\nI've thought a lot about the discussion over whether this series should\nuse the configuration as the source for multiple hooks. Ultimately, I've\ncome to the decision that it's not a good idea. Even adopting the empty\nentry as a reset marker, the fact that inheritance in the configuration\nis in-order and can't be easily modified means that it's not likely to\nbe very useful, but it is likely to be quite surprising for the average\nuser. I think a solution that sticks with the existing model and builds\noff a design used by other systems people are familiar with, like cron\nand run-parts, is going to be a better choice. Moreover, this is the\ndesign that people have already built with outside tooling, which is a\nfurther argument in favor of it.\n\nI have adopted one configuration-based option, which is the per-hook\nerrorBehavior option that Peff suggested. I think this reduces concerns\nover what the best error handling strategy is and is a good thing to\nhave as part of a minimum viable product. I picked the names that Peff\nchose, but if people like different names better, they can be changed.\n\nJust as a preview of what's coming down the line, I plan to build on\nthis series to notify hooks when --quiet and --dry-run options have been\nspecified to commands so that they may honor them if they choose.\n\nChanges from v1:\n* Adopted several improvements from Duy's series, including an improved\n find_hooks prototype and a helper function.\n* Switched to existence checks instead of executability checks for\n determining whether to invoke multiple hooks.\n* Adjusted the commit message for patch 3.\n* Added error behavior control using the names Peff provided in his\n comment.\n* Added documentation.\n\nbrian m. carlson (7):\n run-command: add preliminary support for multiple hooks\n builtin/receive-pack: add support for multiple hooks\n rebase: add support for multiple hooks\n builtin/worktree: add support for multiple post-checkout hooks\n transport: add support for multiple pre-push hooks\n config: allow configuration of multiple hook error behavior\n docs: document multiple hooks\n\n Documentation/config.txt | 2 +\n Documentation/config/hook.txt | 19 ++\n Documentation/githooks.txt | 9 +\n builtin/am.c | 20 +--\n builtin/commit.c | 2 +-\n builtin/receive-pack.c | 78 ++++----\n builtin/worktree.c | 44 +++--\n config.c | 27 +++\n run-command.c | 175 +++++++++++++++---\n run-command.h | 22 ++-\n sequencer.c | 59 ++++---\n sequencer.h | 2 +\n t/lib-hooks.sh | 274 +++++++++++++++++++++++++++++\n t/t5403-post-checkout-hook.sh | 8 +\n t/t5407-post-rewrite-hook.sh | 15 ++\n t/t5516-fetch-push.sh | 30 ++++\n t/t5571-pre-push-hook.sh | 19 ++\n t/t7503-pre-commit-hook.sh | 15 ++\n t/t7505-prepare-commit-msg-hook.sh | 9 +\n transport.c | 29 +--\n 20 files changed, 730 insertions(+), 128 deletions(-)\n create mode 100644 Documentation/config/hook.txt\n create mode 100644 t/lib-hooks.sh\n\n"},{"id":"375445","messageId":"20190514002332.121089-2-sandals@crustytoothpaste.net","threadId":"51084","inReplyTo":"20190514002332.121089-1-sandals@crustytoothpaste.net","subject":"[PATCH v2 1/7] run-command: add preliminary support for multiple hooks","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-05-14T00:23:25Z","receivedAt":"2019-05-14T00:23:53Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"A variety of types of software take advantage of Git's hooks. However,\nif a user would like to integrate multiple pieces of software which use\na particular hook, they currently must manage those hooks themselves,\nwhich can be burdensome. Sometimes various pieces of software try to\noverwrite each other's hooks, leading to problems.\n\nTo solve this problem, introduce a framework for running multiple hooks\nusing a \".d\" directory named similarly to the hook, running each hook in\norder sorted by name. Wire this framework up for those functions using\nrun_hook_le or run_hook_ve. To preserve backwards compatibility, ensure\nthat multiple hooks run only if there is no hook using the current hook\nstyle.\n\nIf we are running multiple hooks and one of them exits nonzero, don't\nexecute the remaining hooks and return that exit code immediately. This\nallows hooks to fail fast and it avoids having to deal with what happens\nif multiple hooks fail with different exit statuses.\n\nCreate a test framework for testing multiple hooks with different\ncommands. This is necessary because not all hooks use run_hook_ve or\nrun_hook_le and we'll want to ensure all the various hooks work without\nneeding to write lots of duplicative test code.\n\nTest the pre-commit hook to verify that the run_hook_ve implementation\nworks correctly.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: brian m. carlson \n---\n builtin/commit.c | 2 +-\n run-command.c | 173 +++++++++++++++++++++++++++++--------\n run-command.h | 15 ++++\n t/lib-hooks.sh | 172 ++++++++++++++++++++++++++++++++++++\n t/t7503-pre-commit-hook.sh | 15 ++++\n 5 files changed, 342 insertions(+), 35 deletions(-)\n create mode 100644 t/lib-hooks.sh\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 833ecb316a..29bf80e0d1 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -943,7 +943,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\treturn 0;\n \t}\n \n-\tif (!no_verify && find_hook(\"pre-commit\")) {\n+\tif (!no_verify && find_hooks(\"pre-commit\", NULL)) {\n \t\t/*\n \t\t * Re-read the index as pre-commit hook could have updated it,\n \t\t * and write it out as a tree. We must do this before we invoke\ndiff --git a/run-command.c b/run-command.c\nindex 3449db319b..eb075ac86b 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1308,53 +1308,143 @@ int async_with_fork(void)\n #endif\n }\n \n+/*\n+ * Return 1 if a hook exists at path (which may be modified) using access(2)\n+ * with check (which should be F_OK or X_OK), 0 otherwise. If strip is true,\n+ * additionally consider the same filename but with STRIP_EXTENSION added.\n+ * If check is X_OK, warn if the hook exists but is not executable.\n+ */\n+static int has_hook(struct strbuf *path, int strip, int check)\n+{\n+\tif (access(path->buf, check) < 0) {\n+\t\tint err = errno;\n+\n+\t\tif (strip) {\n+#ifdef STRIP_EXTENSION\n+\t\t\tstrbuf_addstr(path, STRIP_EXTENSION);\n+\t\t\tif (access(path->buf, check) >= 0)\n+\t\t\t\treturn 1;\n+\t\t\tif (errno == EACCES)\n+\t\t\t\terr = errno;\n+#endif\n+\t\t}\n+\n+\t\tif (err == EACCES && advice_ignored_hook) {\n+\t\t\tstatic struct string_list advise_given = STRING_LIST_INIT_DUP;\n+\n+\t\t\tif (!string_list_lookup(&advise_given, path->buf)) {\n+\t\t\t\tstring_list_insert(&advise_given, path->buf);\n+\t\t\t\tadvise(_(\"The '%s' hook was ignored because \"\n+\t\t\t\t\t \"it's not set as executable.\\n\"\n+\t\t\t\t\t \"You can disable this warning with \"\n+\t\t\t\t\t \"`git config advice.ignoredHook false`.\"),\n+\t\t\t\t path->buf);\n+\t\t\t}\n+\t\t}\n+\t\treturn 0;\n+\t}\n+\treturn 1;\n+}\n+\n const char *find_hook(const char *name)\n {\n \tstatic struct strbuf path = STRBUF_INIT;\n \n \tstrbuf_reset(&path);\n \tstrbuf_git_path(&path, \"hooks/%s\", name);\n-\tif (access(path.buf, X_OK) < 0) {\n-\t\tint err = errno;\n-\n-#ifdef STRIP_EXTENSION\n-\t\tstrbuf_addstr(&path, STRIP_EXTENSION);\n-\t\tif (access(path.buf, X_OK) >= 0)\n-\t\t\treturn path.buf;\n-\t\tif (errno == EACCES)\n-\t\t\terr = errno;\n-#endif\n-\n-\t\tif (err == EACCES && advice_ignored_hook) {\n-\t\t\tstatic struct string_list advise_given = STRING_LIST_INIT_DUP;\n-\n-\t\t\tif (!string_list_lookup(&advise_given, name)) {\n-\t\t\t\tstring_list_insert(&advise_given, name);\n-\t\t\t\tadvise(_(\"The '%s' hook was ignored because \"\n-\t\t\t\t\t \"it's not set as executable.\\n\"\n-\t\t\t\t\t \"You can disable this warning with \"\n-\t\t\t\t\t \"`git config advice.ignoredHook false`.\"),\n-\t\t\t\t path.buf);\n-\t\t\t}\n-\t\t}\n-\t\treturn NULL;\n+\tif (has_hook(&path, 1, X_OK)) {\n+\t\treturn path.buf;\n \t}\n-\treturn path.buf;\n+\treturn NULL;\n }\n \n-int run_hook_ve(const char *const *env, const char *name, va_list args)\n+int find_hooks(const char *name, struct string_list *list)\n {\n-\tstruct child_process hook = CHILD_PROCESS_INIT;\n-\tconst char *p;\n+\tstruct strbuf path = STRBUF_INIT;\n+\tDIR *d;\n+\tstruct dirent *de;\n \n-\tp = find_hook(name);\n-\tif (!p)\n+\t/*\n+\t * We look for a single hook. If present, return it, and skip the\n+\t * individual directories.\n+\t */\n+\tstrbuf_git_path(&path, \"hooks/%s\", name);\n+\tif (has_hook(&path, 1, X_OK)) {\n+\t\tif (list)\n+\t\t\tstring_list_append(list, path.buf);\n+\t\treturn 1;\n+\t}\n+\n+\tif (has_hook(&path, 1, F_OK))\n \t\treturn 0;\n \n-\targv_array_push(&hook.args, p);\n-\twhile ((p = va_arg(args, const char *)))\n-\t\targv_array_push(&hook.args, p);\n-\thook.env = env;\n+\tstrbuf_reset(&path);\n+\tstrbuf_git_path(&path, \"hooks/%s.d\", name);\n+\td = opendir(path.buf);\n+\tif (!d) {\n+\t\tif (list)\n+\t\t\tstring_list_clear(list, 0);\n+\t\treturn 0;\n+\t}\n+\twhile ((de = readdir(d))) {\n+\t\tif (!strcmp(de->d_name, \".\") || !strcmp(de->d_name, \"..\"))\n+\t\t\tcontinue;\n+\t\tstrbuf_reset(&path);\n+\t\tstrbuf_git_path(&path, \"hooks/%s.d/%s\", name, de->d_name);\n+\t\tif (has_hook(&path, 0, X_OK)) {\n+\t\t\tif (list)\n+\t\t\t\tstring_list_append(list, path.buf);\n+\t\t\telse\n+\t\t\t\treturn 1;\n+\t\t}\n+\t}\n+\tclosedir(d);\n+\tstrbuf_reset(&path);\n+\tif (!list->nr) {\n+\t\treturn 0;\n+\t}\n+\n+\tstring_list_sort(list);\n+\treturn 1;\n+}\n+\n+int for_each_hook(const char *name,\n+\t\t int (*handler)(const char *name, const char *path, void *),\n+\t\t void *data)\n+{\n+\tstruct string_list paths = STRING_LIST_INIT_DUP;\n+\tint i, ret = 0;\n+\n+\tfind_hooks(name, &paths);\n+\tfor (i = 0; i < paths.nr; i++) {\n+\t\tconst char *p = paths.items[i].string;\n+\n+\t\tret = handler(name, p, data);\n+\t\tif (ret)\n+\t\t\tbreak;\n+\t}\n+\n+\tstring_list_clear(&paths, 0);\n+\treturn ret;\n+}\n+\n+struct hook_data {\n+\tconst char *const *env;\n+\tstruct string_list *args;\n+};\n+\n+static int do_run_hook_ve(const char *name, const char *path, void *cb)\n+{\n+\tstruct hook_data *data = cb;\n+\tstruct child_process hook = CHILD_PROCESS_INIT;\n+\tstruct string_list_item *arg;\n+\n+\targv_array_push(&hook.args, path);\n+\tfor_each_string_list_item(arg, data->args) {\n+\t\targv_array_push(&hook.args, arg->string);\n+\t}\n+\n+\thook.env = data->env;\n \thook.no_stdin = 1;\n \thook.stdout_to_stderr = 1;\n \thook.trace2_hook_name = name;\n@@ -1362,6 +1452,21 @@ int run_hook_ve(const char *const *env, const char *name, va_list args)\n \treturn run_command(&hook);\n }\n \n+int run_hook_ve(const char *const *env, const char *name, va_list args)\n+{\n+\tstruct string_list arglist = STRING_LIST_INIT_DUP;\n+\tstruct hook_data data = {env, &arglist};\n+\tconst char *p;\n+\tint ret = 0;\n+\n+\twhile ((p = va_arg(args, const char *)))\n+\t\tstring_list_append(&arglist, p);\n+\n+\tret = for_each_hook(name, do_run_hook_ve, &data);\n+\tstring_list_clear(&arglist, 0);\n+\treturn ret;\n+}\n+\n int run_hook_le(const char *const *env, const char *name, ...)\n {\n \tva_list args;\ndiff --git a/run-command.h b/run-command.h\nindex a6950691c0..1b3677fcac 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -4,6 +4,7 @@\n #include \"thread-utils.h\"\n \n #include \"argv-array.h\"\n+#include \"string-list.h\"\n \n struct child_process {\n \tconst char **argv;\n@@ -68,6 +69,20 @@ int run_command(struct child_process *);\n * overwritten by further calls to find_hook and run_hook_*.\n */\n extern const char *find_hook(const char *name);\n+/*\n+ * Returns the paths to all hook files, or NULL if all hooks are missing or\n+ * disabled.\n+ * Returns 1 if there are hooks; 0 otherwise. If hooks is not NULL, stores the\n+ * names of the hooks into them in the order they should be executed.\n+ */\n+int find_hooks(const char *name, struct string_list *hooks);\n+/*\n+ * Invokes the handler function once for each hook. Returns 0 if all hooks were\n+ * successful, or the exit status of the first failing hook.\n+ */\n+int for_each_hook(const char *name,\n+\t\t int (*handler)(const char *name, const char *path, void *),\n+\t\t void *data);\n LAST_ARG_MUST_BE_NULL\n extern int run_hook_le(const char *const *env, const char *name, ...);\n extern int run_hook_ve(const char *const *env, const char *name, va_list args);\ndiff --git a/t/lib-hooks.sh b/t/lib-hooks.sh\nnew file mode 100644\nindex 0000000000..721250aea0\n--- /dev/null\n+++ b/t/lib-hooks.sh\n@@ -0,0 +1,172 @@\n+create_multihooks () {\n+\tmkdir -p \"$MULTIHOOK_DIR\"\n+\tfor i in \"a $1\" \"b $2\" \"c $3\"\n+\tdo\n+\t\techo \"$i\" | (while read script ex\n+\t\tdo\n+\t\t\tmkdir -p \"$MULTIHOOK_DIR\"\n+\t\t\twrite_script \"$MULTIHOOK_DIR/$script\" <<-EOF\n+\t\t\tmkdir -p \"$OUTPUTDIR\"\n+\t\t\ttouch \"$OUTPUTDIR/$script\"\n+\t\t\texit $ex\n+\t\t\tEOF\n+\t\tdone)\n+\tdone\n+}\n+\n+# Run the multiple hook tests.\n+# Usage: test_multiple_hooks [--ignore-exit-status] HOOK COMMAND [SKIP-COMMAND]\n+# HOOK: the name of the hook to test\n+# COMMAND: command to test the hook for; takes a single argument indicating test\n+# name.\n+# SKIP-COMMAND: like $1, except the hook should be skipped.\n+# --ignore-exit-status: the command does not fail if the exit status from the\n+# hook is nonzero.\n+test_multiple_hooks () {\n+\tlocal must_fail cmd skip_cmd hook\n+\tif test \"$1\" = \"--ignore-exit-status\"\n+\tthen\n+\t\tshift\n+\telse\n+\t\tmust_fail=\"test_must_fail\"\n+\tfi\n+\thook=\"$1\"\n+\tcmd=\"$2\"\n+\tskip_cmd=\"$3\"\n+\n+\tHOOKDIR=\"$(git rev-parse --absolute-git-dir)/hooks\"\n+\tOUTPUTDIR=\"$(git rev-parse --absolute-git-dir)/../hook-output\"\n+\tHOOK=\"$HOOKDIR/$hook\"\n+\tMULTIHOOK_DIR=\"$HOOKDIR/$hook.d\"\n+\trm -f \"$HOOK\" \"$MULTIHOOK_DIR\" \"$OUTPUTDIR\"\n+\n+\ttest_expect_success \"$hook: with no hook\" '\n+\t\t$cmd foo\n+\t'\n+\n+\tif test -n \"$skip_cmd\"\n+\tthen\n+\t\ttest_expect_success \"$hook: skipped hook with no hook\" '\n+\t\t\t$skip_cmd bar\n+\t\t'\n+\tfi\n+\n+\ttest_expect_success 'setup' '\n+\t\tmkdir -p \"$HOOKDIR\" &&\n+\t\twrite_script \"$HOOK\" <<-EOF\n+\t\tmkdir -p \"$OUTPUTDIR\"\n+\t\ttouch \"$OUTPUTDIR/simple\"\n+\t\texit 0\n+\t\tEOF\n+\t'\n+\n+\ttest_expect_success \"$hook: with succeeding hook\" '\n+\t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n+\t\t$cmd more &&\n+\t\ttest -f \"$OUTPUTDIR/simple\"\n+\t'\n+\n+\tif test -n \"$skip_cmd\"\n+\tthen\n+\t\ttest_expect_success \"$hook: skipped but succeeding hook\" '\n+\t\t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n+\t\t\t$skip_cmd even-more &&\n+\t\t\t! test -f \"$OUTPUTDIR/simple\"\n+\t\t'\n+\tfi\n+\n+\ttest_expect_success \"$hook: with both simple and multiple hooks, simple success\" '\n+\t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n+\t\tcreate_multihooks 0 1 0 &&\n+\t\t$cmd yet-more &&\n+\t\ttest -f \"$OUTPUTDIR/simple\" &&\n+\t\t! test -f \"$OUTPUTDIR/a\" &&\n+\t\t! test -f \"$OUTPUTDIR/b\" &&\n+\t\t! test -f \"$OUTPUTDIR/c\"\n+\t'\n+\n+\ttest_expect_success 'setup' '\n+\t\trm -fr \"$MULTIHOOK_DIR\" &&\n+\n+\t\t# now a hook that fails\n+\t\twrite_script \"$HOOK\" <<-EOF\n+\t\tmkdir -p \"$OUTPUTDIR\"\n+\t\ttouch \"$OUTPUTDIR/simple\"\n+\t\texit 1\n+\t\tEOF\n+\t'\n+\n+\ttest_expect_success \"$hook: with failing hook\" '\n+\t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n+\t\t$must_fail $cmd another &&\n+\t\ttest -f \"$OUTPUTDIR/simple\"\n+\t'\n+\n+\tif test -n \"$skip_cmd\"\n+\tthen\n+\t\ttest_expect_success \"$hook: skipped but failing hook\" '\n+\t\t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n+\t\t\t$skip_cmd stuff &&\n+\t\t\t! test -f \"$OUTPUTDIR/simple\"\n+\t\t'\n+\tfi\n+\n+\ttest_expect_success \"$hook: with both simple and multiple hooks, simple failure\" '\n+\t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n+\t\tcreate_multihooks 0 1 0 &&\n+\t\t$must_fail $cmd more-stuff &&\n+\t\ttest -f \"$OUTPUTDIR/simple\" &&\n+\t\t! test -f \"$OUTPUTDIR/a\" &&\n+\t\t! test -f \"$OUTPUTDIR/b\" &&\n+\t\t! test -f \"$OUTPUTDIR/c\"\n+\t'\n+\n+\ttest_expect_success \"$hook: multiple hooks, all successful\" '\n+\t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n+\t\trm -f \"$HOOK\" &&\n+\t\tcreate_multihooks 0 0 0 &&\n+\t\t$cmd content &&\n+\t\ttest -f \"$OUTPUTDIR/a\" &&\n+\t\ttest -f \"$OUTPUTDIR/b\" &&\n+\t\ttest -f \"$OUTPUTDIR/c\"\n+\t'\n+\n+\ttest_expect_success \"$hook: hooks after first failure not executed\" '\n+\t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n+\t\tcreate_multihooks 0 1 0 &&\n+\t\t$must_fail $cmd more-content &&\n+\t\ttest -f \"$OUTPUTDIR/a\" &&\n+\t\ttest -f \"$OUTPUTDIR/b\" &&\n+\t\t! test -f \"$OUTPUTDIR/c\"\n+\t'\n+\n+\ttest_expect_success POSIXPERM \"$hook: non-executable hook not executed\" '\n+\t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n+\t\tcreate_multihooks 0 1 0 &&\n+\t\tchmod -x \"$MULTIHOOK_DIR/b\" &&\n+\t\t$cmd things &&\n+\t\ttest -f \"$OUTPUTDIR/a\" &&\n+\t\t! test -f \"$OUTPUTDIR/b\" &&\n+\t\ttest -f \"$OUTPUTDIR/c\"\n+\t'\n+\n+\ttest_expect_success POSIXPERM \"$hook: multiple hooks not executed if simple hook present\" '\n+\t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\" && rm -f \\\"$HOOK\\\"\" &&\n+\t\twrite_script \"$HOOK\" <<-EOF &&\n+\t\tmkdir -p \"$OUTPUTDIR\"\n+\t\ttouch \"$OUTPUTDIR/simple\"\n+\t\texit 0\n+\t\tEOF\n+\t\tcreate_multihooks 0 1 0 &&\n+\t\tchmod -x \"$HOOK\" &&\n+\t\t$cmd other-things &&\n+\t\t! test -f \"$OUTPUTDIR/simple\" &&\n+\t\t! test -f \"$OUTPUTDIR/a\" &&\n+\t\t! test -f \"$OUTPUTDIR/b\" &&\n+\t\t! test -f \"$OUTPUTDIR/c\"\n+\t'\n+\n+\ttest_expect_success 'cleanup' '\n+\t\trm -fr \"$MULTIHOOK_DIR\"\n+\t'\n+}\ndiff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh\nindex 984889b39d..d63d059e04 100755\n--- a/t/t7503-pre-commit-hook.sh\n+++ b/t/t7503-pre-commit-hook.sh\n@@ -3,6 +3,7 @@\n test_description='pre-commit hook'\n \n . ./test-lib.sh\n+. \"$TEST_DIRECTORY/lib-hooks.sh\"\n \n test_expect_success 'with no hook' '\n \n@@ -136,4 +137,18 @@ test_expect_success 'check the author in hook' '\n \tgit show -s\n '\n \n+commit_command () {\n+\techo \"$1\" >>file &&\n+\tgit add file &&\n+\tgit commit -m \"$1\"\n+}\n+\n+commit_no_verify_command () {\n+\techo \"$1\" >>file &&\n+\tgit add file &&\n+\tgit commit --no-verify -m \"$1\"\n+}\n+\n+test_multiple_hooks pre-commit commit_command commit_no_verify_command\n+\n test_done\n"},{"id":"375446","messageId":"20190514002332.121089-3-sandals@crustytoothpaste.net","threadId":"51084","inReplyTo":"20190514002332.121089-1-sandals@crustytoothpaste.net","subject":"[PATCH v2 2/7] builtin/receive-pack: add support for multiple hooks","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-05-14T00:23:26Z","receivedAt":"2019-05-14T00:23:55Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"Add support for multiple hooks for the pre-receive, post-receive,\nupdate, post-update, and push-to-checkout hooks. Add tests for these\nhooks using the multiple hook test framework.\n\nBecause the invocations of test_multiple_hooks contain multiple test\nassertions, they (and the cd commands that surround them) must occur\noutside of a subshell, or a failing test will not be noticed.\n\nSigned-off-by: brian m. carlson \n---\n builtin/receive-pack.c | 78 ++++++++++++++++++++++++++----------------\n t/t5516-fetch-push.sh | 30 ++++++++++++++++\n 2 files changed, 78 insertions(+), 30 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 29f165d8bd..5940f6969a 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -669,6 +669,8 @@ static void prepare_push_cert_sha1(struct child_process *proc)\n \t}\n }\n \n+typedef int (*feed_fn)(void *, const char **, size_t *);\n+\n struct receive_hook_feed_state {\n \tstruct command *cmd;\n \tint skip_broken;\n@@ -676,34 +678,36 @@ struct receive_hook_feed_state {\n \tconst struct string_list *push_options;\n };\n \n-typedef int (*feed_fn)(void *, const char **, size_t *);\n-static int run_and_feed_hook(const char *hook_name, feed_fn feed,\n-\t\t\t struct receive_hook_feed_state *feed_state)\n+struct receive_hook_data {\n+\tfeed_fn fn;\n+\tstruct receive_hook_feed_state *state;\n+};\n+\n+static int do_run_and_feed_hook(const char *name, const char *path, void *cbp)\n {\n-\tstruct child_process proc = CHILD_PROCESS_INIT;\n+\tstruct receive_hook_data *data = cbp;\n+\tstruct child_process proc;\n \tstruct async muxer;\n \tconst char *argv[2];\n-\tint code;\n-\n-\targv[0] = find_hook(hook_name);\n-\tif (!argv[0])\n-\t\treturn 0;\n+\tint code = 0;\n \n+\targv[0] = path;\n \targv[1] = NULL;\n \n+\tchild_process_init(&proc);\n \tproc.argv = argv;\n \tproc.in = -1;\n \tproc.stdout_to_stderr = 1;\n-\tproc.trace2_hook_name = hook_name;\n+\tproc.trace2_hook_name = name;\n \n-\tif (feed_state->push_options) {\n+\tif (data->state->push_options) {\n \t\tint i;\n-\t\tfor (i = 0; i < feed_state->push_options->nr; i++)\n+\t\tfor (i = 0; i < data->state->push_options->nr; i++)\n \t\t\targv_array_pushf(&proc.env_array,\n \t\t\t\t\"GIT_PUSH_OPTION_%d=%s\", i,\n-\t\t\t\tfeed_state->push_options->items[i].string);\n+\t\t\t\tdata->state->push_options->items[i].string);\n \t\targv_array_pushf(&proc.env_array, \"GIT_PUSH_OPTION_COUNT=%d\",\n-\t\t\t\t feed_state->push_options->nr);\n+\t\t\t\t data->state->push_options->nr);\n \t} else\n \t\targv_array_pushf(&proc.env_array, \"GIT_PUSH_OPTION_COUNT\");\n \n@@ -734,7 +738,7 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,\n \twhile (1) {\n \t\tconst char *buf;\n \t\tsize_t n;\n-\t\tif (feed(feed_state, &buf, &n))\n+\t\tif (data->fn(data->state, &buf, &n))\n \t\t\tbreak;\n \t\tif (write_in_full(proc.in, buf, n) < 0)\n \t\t\tbreak;\n@@ -748,6 +752,13 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,\n \treturn finish_command(&proc);\n }\n \n+static int run_and_feed_hook(const char *hook_name, feed_fn feed,\n+\t\t\t struct receive_hook_feed_state *feed_state)\n+{\n+\tstruct receive_hook_data data = { feed, feed_state };\n+\treturn for_each_hook(hook_name, do_run_and_feed_hook, &data);\n+}\n+\n static int feed_receive_hook(void *state_, const char **bufp, size_t *sizep)\n {\n \tstruct receive_hook_feed_state *state = state_;\n@@ -790,16 +801,14 @@ static int run_receive_hook(struct command *commands,\n \treturn status;\n }\n \n-static int run_update_hook(struct command *cmd)\n+static int do_run_update_hook(const char *name, const char *path, void *data)\n {\n-\tconst char *argv[5];\n+\tstruct command *cmd = data;\n \tstruct child_process proc = CHILD_PROCESS_INIT;\n+\tconst char *argv[5];\n \tint code;\n \n-\targv[0] = find_hook(\"update\");\n-\tif (!argv[0])\n-\t\treturn 0;\n-\n+\targv[0] = path;\n \targv[1] = cmd->ref_name;\n \targv[2] = oid_to_hex(&cmd->old_oid);\n \targv[3] = oid_to_hex(&cmd->new_oid);\n@@ -819,6 +828,11 @@ static int run_update_hook(struct command *cmd)\n \treturn finish_command(&proc);\n }\n \n+static int run_update_hook(struct command *cmd)\n+{\n+\treturn for_each_hook(\"update\", do_run_update_hook, cmd);\n+}\n+\n static int is_ref_checked_out(const char *ref)\n {\n \tif (is_bare_repository())\n@@ -1011,7 +1025,7 @@ static const char *update_worktree(unsigned char *sha1)\n \n \targv_array_pushf(&env, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n \n-\tif (!find_hook(push_to_checkout_hook))\n+\tif (!find_hooks(push_to_checkout_hook, NULL))\n \t\tretval = push_to_deploy(sha1, &env, work_tree);\n \telse\n \t\tretval = push_to_checkout(sha1, &env, work_tree);\n@@ -1170,25 +1184,23 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t}\n }\n \n-static void run_update_post_hook(struct command *commands)\n+static int do_run_update_post_hook(const char *name, const char *path, void *data)\n {\n+\tstruct command *commands = data;\n \tstruct command *cmd;\n \tstruct child_process proc = CHILD_PROCESS_INIT;\n-\tconst char *hook;\n \n-\thook = find_hook(\"post-update\");\n-\tif (!hook)\n-\t\treturn;\n+\tchild_process_init(&proc);\n \n \tfor (cmd = commands; cmd; cmd = cmd->next) {\n \t\tif (cmd->error_string || cmd->did_not_exist)\n \t\t\tcontinue;\n \t\tif (!proc.args.argc)\n-\t\t\targv_array_push(&proc.args, hook);\n+\t\t\targv_array_push(&proc.args, path);\n \t\targv_array_push(&proc.args, cmd->ref_name);\n \t}\n \tif (!proc.args.argc)\n-\t\treturn;\n+\t\treturn 0;\n \n \tproc.no_stdin = 1;\n \tproc.stdout_to_stderr = 1;\n@@ -1198,8 +1210,14 @@ static void run_update_post_hook(struct command *commands)\n \tif (!start_command(&proc)) {\n \t\tif (use_sideband)\n \t\t\tcopy_to_sideband(proc.err, -1, NULL);\n-\t\tfinish_command(&proc);\n+\t\treturn finish_command(&proc);\n \t}\n+\treturn -1;\n+}\n+\n+static void run_update_post_hook(struct command *commands)\n+{\n+\tfor_each_hook(\"post-update\", do_run_update_post_hook, commands);\n }\n \n static void check_aliased_update_internal(struct command *cmd,\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex c81ca360ac..697c3ab074 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -15,6 +15,7 @@ This test checks the following functionality:\n '\n \n . ./test-lib.sh\n+. \"$TEST_DIRECTORY/lib-hooks.sh\"\n \n D=$(pwd)\n \n@@ -1712,4 +1713,33 @@ test_expect_success 'updateInstead with push-to-checkout hook' '\n \t)\n '\n \n+test_expect_success 'setup' '\n+\tmk_test_with_hooks hooktest heads/master\n+'\n+\n+cmd_receive () {\n+\tgit reset --hard &&\n+\techo \"$1\" >>../file &&\n+\tgit -C .. add file &&\n+\tgit -C .. commit -m \"$1\" &&\n+\tgit -C .. push hooktest refs/heads/master:refs/heads/master\n+}\n+\n+cd hooktest\n+test_multiple_hooks pre-receive cmd_receive\n+test_multiple_hooks --ignore-exit-status post-receive cmd_receive\n+test_multiple_hooks update cmd_receive\n+test_multiple_hooks --ignore-exit-status post-update cmd_receive\n+cd ..\n+\n+test_expect_success 'setup' '\n+\trm -fr hooktest &&\n+\tgit init hooktest &&\n+\tgit -C hooktest config receive.denyCurrentBranch updateInstead\n+'\n+\n+cd hooktest\n+test_multiple_hooks push-to-checkout cmd_receive\n+cd ..\n+\n test_done\n"},{"id":"375447","messageId":"20190514002332.121089-4-sandals@crustytoothpaste.net","threadId":"51084","inReplyTo":"20190514002332.121089-1-sandals@crustytoothpaste.net","subject":"[PATCH v2 3/7] rebase: add support for multiple hooks","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-05-14T00:23:27Z","receivedAt":"2019-05-14T00:23:57Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"Add support for multiple post-rewrite hooks, both for \"git commit\n--amend\" and \"git rebase\". Unify the hook handling between the am-based\nand sequencer-based code paths.\n\nAdditionally add support for multiple prepare-commit-msg hooks.\n\nNote that the prepare-commit-msg hook is not passed a set of hooks\ndirectly because these are discovered later when the code calls\nrun_hooks_le.\n\nSigned-off-by: brian m. carlson \n---\n builtin/am.c | 20 +---------\n sequencer.c | 59 ++++++++++++++++++------------\n sequencer.h | 2 +\n t/t5407-post-rewrite-hook.sh | 15 ++++++++\n t/t7505-prepare-commit-msg-hook.sh | 9 +++++\n 5 files changed, 63 insertions(+), 42 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 912d9821b1..340eacbd44 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -441,24 +441,8 @@ static int run_applypatch_msg_hook(struct am_state *state)\n */\n static int run_post_rewrite_hook(const struct am_state *state)\n {\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tconst char *hook = find_hook(\"post-rewrite\");\n-\tint ret;\n-\n-\tif (!hook)\n-\t\treturn 0;\n-\n-\targv_array_push(&cp.args, hook);\n-\targv_array_push(&cp.args, \"rebase\");\n-\n-\tcp.in = xopen(am_path(state, \"rewritten\"), O_RDONLY);\n-\tcp.stdout_to_stderr = 1;\n-\tcp.trace2_hook_name = \"post-rewrite\";\n-\n-\tret = run_command(&cp);\n-\n-\tclose(cp.in);\n-\treturn ret;\n+\treturn for_each_hook(\"post-rewrite\", post_rewrite_rebase_hook,\n+\t\t\t (void *)am_path(state, \"rewritten\"));\n }\n \n /**\ndiff --git a/sequencer.c b/sequencer.c\nindex 546f281898..b899209f76 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1084,30 +1084,32 @@ int update_head_with_reflog(const struct commit *old_head,\n \treturn ret;\n }\n \n-static int run_rewrite_hook(const struct object_id *oldoid,\n-\t\t\t const struct object_id *newoid)\n+struct rewrite_hook_data {\n+\tconst struct object_id *oldoid;\n+\tconst struct object_id *newoid;\n+};\n+\n+static int do_run_rewrite_hook(const char *name, const char *path, void *p)\n {\n+\tstruct rewrite_hook_data *data = p;\n \tstruct child_process proc = CHILD_PROCESS_INIT;\n+\tstruct strbuf sb = STRBUF_INIT;\n \tconst char *argv[3];\n \tint code;\n-\tstruct strbuf sb = STRBUF_INIT;\n-\n-\targv[0] = find_hook(\"post-rewrite\");\n-\tif (!argv[0])\n-\t\treturn 0;\n \n+\targv[0] = path;\n \targv[1] = \"amend\";\n \targv[2] = NULL;\n \n \tproc.argv = argv;\n \tproc.in = -1;\n \tproc.stdout_to_stderr = 1;\n-\tproc.trace2_hook_name = \"post-rewrite\";\n+\tproc.trace2_hook_name = name;\n \n \tcode = start_command(&proc);\n \tif (code)\n \t\treturn code;\n-\tstrbuf_addf(&sb, \"%s %s\\n\", oid_to_hex(oldoid), oid_to_hex(newoid));\n+\tstrbuf_addf(&sb, \"%s %s\\n\", oid_to_hex(data->oldoid), oid_to_hex(data->newoid));\n \tsigchain_push(SIGPIPE, SIG_IGN);\n \twrite_in_full(proc.in, sb.buf, sb.len);\n \tclose(proc.in);\n@@ -1116,6 +1118,25 @@ static int run_rewrite_hook(const struct object_id *oldoid,\n \treturn finish_command(&proc);\n }\n \n+static int run_rewrite_hook(const struct object_id *oldoid,\n+\t\t\t const struct object_id *newoid)\n+{\n+\tstruct rewrite_hook_data data = { oldoid, newoid };\n+\treturn for_each_hook(\"post-rewrite\", do_run_rewrite_hook, &data);\n+}\n+\n+int post_rewrite_rebase_hook(const char *name, const char *path, void *input)\n+{\n+\tstruct child_process child = CHILD_PROCESS_INIT;\n+\n+\tchild.in = open(input, O_RDONLY);\n+\tchild.stdout_to_stderr = 1;\n+\tchild.trace2_hook_name = \"post-rewrite\";\n+\targv_array_push(&child.args, path);\n+\targv_array_push(&child.args, \"rebase\");\n+\treturn run_command(&child);\n+}\n+\n void commit_post_rewrite(struct repository *r,\n \t\t\t const struct commit *old_head,\n \t\t\t const struct object_id *new_head)\n@@ -1368,7 +1389,7 @@ static int try_to_commit(struct repository *r,\n \t\tgoto out;\n \t}\n \n-\tif (find_hook(\"prepare-commit-msg\")) {\n+\tif (find_hooks(\"prepare-commit-msg\", NULL)) {\n \t\tres = run_prepare_commit_msg_hook(r, msg, hook_commit);\n \t\tif (res)\n \t\t\tgoto out;\n@@ -3763,8 +3784,6 @@ static int pick_commits(struct repository *r,\n \t\tif (!stat(rebase_path_rewritten_list(), &st) &&\n \t\t\t\tst.st_size > 0) {\n \t\t\tstruct child_process child = CHILD_PROCESS_INIT;\n-\t\t\tconst char *post_rewrite_hook =\n-\t\t\t\tfind_hook(\"post-rewrite\");\n \n \t\t\tchild.in = open(rebase_path_rewritten_list(), O_RDONLY);\n \t\t\tchild.git_cmd = 1;\n@@ -3774,18 +3793,10 @@ static int pick_commits(struct repository *r,\n \t\t\t/* we don't care if this copying failed */\n \t\t\trun_command(&child);\n \n-\t\t\tif (post_rewrite_hook) {\n-\t\t\t\tstruct child_process hook = CHILD_PROCESS_INIT;\n-\n-\t\t\t\thook.in = open(rebase_path_rewritten_list(),\n-\t\t\t\t\tO_RDONLY);\n-\t\t\t\thook.stdout_to_stderr = 1;\n-\t\t\t\thook.trace2_hook_name = \"post-rewrite\";\n-\t\t\t\targv_array_push(&hook.args, post_rewrite_hook);\n-\t\t\t\targv_array_push(&hook.args, \"rebase\");\n-\t\t\t\t/* we don't care if this hook failed */\n-\t\t\t\trun_command(&hook);\n-\t\t\t}\n+\t\t\t/* we don't care if this hook failed */\n+\t\t\tfor_each_hook(\"post-rewrite\",\n+\t\t\t\t post_rewrite_rebase_hook,\n+\t\t\t\t (void *)rebase_path_rewritten_list());\n \t\t}\n \t\tapply_autostash(opts);\n \ndiff --git a/sequencer.h b/sequencer.h\nindex a515ee4457..710ef5c18c 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -154,6 +154,8 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \t\t unsigned autosquash, struct todo_list *todo_list);\n int todo_list_rearrange_squash(struct todo_list *todo_list);\n \n+int post_rewrite_rebase_hook(const char *name, const char *path, void *input);\n+\n /*\n * Append a signoff to the commit message in \"msgbuf\". The ignore_footer\n * parameter specifies the number of bytes at the end of msgbuf that should\ndiff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh\nindex 7344253bfb..f8ce32fe3b 100755\n--- a/t/t5407-post-rewrite-hook.sh\n+++ b/t/t5407-post-rewrite-hook.sh\n@@ -5,6 +5,7 @@\n \n test_description='Test the post-rewrite hook.'\n . ./test-lib.sh\n+. \"$TEST_DIRECTORY/lib-hooks.sh\"\n \n test_expect_success 'setup' '\n \ttest_commit A foo A &&\n@@ -263,4 +264,18 @@ test_expect_success 'git rebase -i (exec)' '\n \tverify_hook_input\n '\n \n+cmd_rebase () {\n+\tgit reset --hard D &&\n+\tFAKE_LINES=\"1 fixup 2\" git rebase -i B\n+}\n+\n+cmd_amend () {\n+\tgit reset --hard D &&\n+\techo \"D new message\" > newmsg &&\n+\tgit commit -Fnewmsg --amend\n+}\n+\n+test_multiple_hooks --ignore-exit-status post-rewrite cmd_rebase\n+test_multiple_hooks --ignore-exit-status post-rewrite cmd_amend\n+\n test_done\ndiff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\nindex ba8bd1b514..287e13e29d 100755\n--- a/t/t7505-prepare-commit-msg-hook.sh\n+++ b/t/t7505-prepare-commit-msg-hook.sh\n@@ -3,6 +3,7 @@\n test_description='prepare-commit-msg hook'\n \n . ./test-lib.sh\n+. \"$TEST_DIRECTORY/lib-hooks.sh\"\n \n test_expect_success 'set up commits for rebasing' '\n \ttest_commit root &&\n@@ -317,4 +318,12 @@ test_expect_success C_LOCALE_OUTPUT 'with failing hook (cherry-pick)' '\n \ttest $(grep -c prepare-commit-msg actual) = 1\n '\n \n+cherry_pick_command () {\n+\tgit checkout -f master &&\n+\tgit checkout -B other b &&\n+\tgit cherry-pick rebase-1\n+}\n+\n+test_multiple_hooks prepare-commit-msg cherry_pick_command\n+\n test_done\n"},{"id":"375448","messageId":"20190514002332.121089-5-sandals@crustytoothpaste.net","threadId":"51084","inReplyTo":"20190514002332.121089-1-sandals@crustytoothpaste.net","subject":"[PATCH v2 3/7] sequencer: add support for multiple hooks","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-05-14T00:23:28Z","receivedAt":"2019-05-14T00:24:09Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"Add support for multiple post-rewrite hooks, both for \"git commit\n--amend\" and \"git rebase\".\n\nAdditionally add support for multiple prepare-commit-msg hooks.\n\nNote that the prepare-commit-msg hook is not passed a set of hooks\ndirectly because these are discovered later when the code calls\nrun_hooks_le.\n\nSigned-off-by: brian m. carlson \n---\n builtin/am.c | 20 +++++-----\n sequencer.c | 59 ++++++++++++++++++------------\n t/t5407-post-rewrite-hook.sh | 15 ++++++++\n t/t7505-prepare-commit-msg-hook.sh | 9 +++++\n 4 files changed, 70 insertions(+), 33 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 912d9821b1..55a5b74da3 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -436,19 +436,13 @@ static int run_applypatch_msg_hook(struct am_state *state)\n \treturn ret;\n }\n \n-/**\n- * Runs post-rewrite hook. Returns it exit code.\n- */\n-static int run_post_rewrite_hook(const struct am_state *state)\n+static int do_run_post_rewrite_hook(const char *name, const char *path, void *p)\n {\n+\tconst struct am_state *state = p;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tconst char *hook = find_hook(\"post-rewrite\");\n \tint ret;\n \n-\tif (!hook)\n-\t\treturn 0;\n-\n-\targv_array_push(&cp.args, hook);\n+\targv_array_push(&cp.args, path);\n \targv_array_push(&cp.args, \"rebase\");\n \n \tcp.in = xopen(am_path(state, \"rewritten\"), O_RDONLY);\n@@ -461,6 +455,14 @@ static int run_post_rewrite_hook(const struct am_state *state)\n \treturn ret;\n }\n \n+/**\n+ * Runs post-rewrite hook. Returns it exit code.\n+ */\n+static int run_post_rewrite_hook(const struct am_state *state)\n+{\n+\treturn for_each_hook(\"post-rewrite\", do_run_post_rewrite_hook, (void *)state);\n+}\n+\n /**\n * Reads the state directory's \"rewritten\" file, and copies notes from the old\n * commits listed in the file to their rewritten commits.\ndiff --git a/sequencer.c b/sequencer.c\nindex 546f281898..f7703bc7a5 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1084,30 +1084,32 @@ int update_head_with_reflog(const struct commit *old_head,\n \treturn ret;\n }\n \n-static int run_rewrite_hook(const struct object_id *oldoid,\n-\t\t\t const struct object_id *newoid)\n+struct rewrite_hook_data {\n+\tconst struct object_id *oldoid;\n+\tconst struct object_id *newoid;\n+};\n+\n+static int do_run_rewrite_hook(const char *name, const char *path, void *p)\n {\n+\tstruct rewrite_hook_data *data = p;\n \tstruct child_process proc = CHILD_PROCESS_INIT;\n+\tstruct strbuf sb = STRBUF_INIT;\n \tconst char *argv[3];\n \tint code;\n-\tstruct strbuf sb = STRBUF_INIT;\n-\n-\targv[0] = find_hook(\"post-rewrite\");\n-\tif (!argv[0])\n-\t\treturn 0;\n \n+\targv[0] = path;\n \targv[1] = \"amend\";\n \targv[2] = NULL;\n \n \tproc.argv = argv;\n \tproc.in = -1;\n \tproc.stdout_to_stderr = 1;\n-\tproc.trace2_hook_name = \"post-rewrite\";\n+\tproc.trace2_hook_name = name;\n \n \tcode = start_command(&proc);\n \tif (code)\n \t\treturn code;\n-\tstrbuf_addf(&sb, \"%s %s\\n\", oid_to_hex(oldoid), oid_to_hex(newoid));\n+\tstrbuf_addf(&sb, \"%s %s\\n\", oid_to_hex(data->oldoid), oid_to_hex(data->newoid));\n \tsigchain_push(SIGPIPE, SIG_IGN);\n \twrite_in_full(proc.in, sb.buf, sb.len);\n \tclose(proc.in);\n@@ -1116,6 +1118,26 @@ static int run_rewrite_hook(const struct object_id *oldoid,\n \treturn finish_command(&proc);\n }\n \n+static int run_rewrite_hook(const struct object_id *oldoid,\n+\t\t\t const struct object_id *newoid)\n+{\n+\tstruct rewrite_hook_data data = { oldoid, newoid };\n+\treturn for_each_hook(\"post-rewrite\", do_run_rewrite_hook, &data);\n+}\n+\n+static int interactive_rewrite_hook_handler(const char *name, const char *path, void *data)\n+{\n+\tstruct child_process child = CHILD_PROCESS_INIT;\n+\n+\tchild.in = open(rebase_path_rewritten_list(),\n+\t\tO_RDONLY);\n+\tchild.stdout_to_stderr = 1;\n+\tchild.trace2_hook_name = \"post-rewrite\";\n+\targv_array_push(&child.args, path);\n+\targv_array_push(&child.args, \"rebase\");\n+\treturn run_command(&child);\n+}\n+\n void commit_post_rewrite(struct repository *r,\n \t\t\t const struct commit *old_head,\n \t\t\t const struct object_id *new_head)\n@@ -1368,7 +1390,7 @@ static int try_to_commit(struct repository *r,\n \t\tgoto out;\n \t}\n \n-\tif (find_hook(\"prepare-commit-msg\")) {\n+\tif (find_hooks(\"prepare-commit-msg\", NULL)) {\n \t\tres = run_prepare_commit_msg_hook(r, msg, hook_commit);\n \t\tif (res)\n \t\t\tgoto out;\n@@ -3763,8 +3785,6 @@ static int pick_commits(struct repository *r,\n \t\tif (!stat(rebase_path_rewritten_list(), &st) &&\n \t\t\t\tst.st_size > 0) {\n \t\t\tstruct child_process child = CHILD_PROCESS_INIT;\n-\t\t\tconst char *post_rewrite_hook =\n-\t\t\t\tfind_hook(\"post-rewrite\");\n \n \t\t\tchild.in = open(rebase_path_rewritten_list(), O_RDONLY);\n \t\t\tchild.git_cmd = 1;\n@@ -3774,18 +3794,9 @@ static int pick_commits(struct repository *r,\n \t\t\t/* we don't care if this copying failed */\n \t\t\trun_command(&child);\n \n-\t\t\tif (post_rewrite_hook) {\n-\t\t\t\tstruct child_process hook = CHILD_PROCESS_INIT;\n-\n-\t\t\t\thook.in = open(rebase_path_rewritten_list(),\n-\t\t\t\t\tO_RDONLY);\n-\t\t\t\thook.stdout_to_stderr = 1;\n-\t\t\t\thook.trace2_hook_name = \"post-rewrite\";\n-\t\t\t\targv_array_push(&hook.args, post_rewrite_hook);\n-\t\t\t\targv_array_push(&hook.args, \"rebase\");\n-\t\t\t\t/* we don't care if this hook failed */\n-\t\t\t\trun_command(&hook);\n-\t\t\t}\n+\t\t\t/* we don't care if this hook failed */\n+\t\t\tfor_each_hook(\"post-rewrite\",\n+\t\t\t\t interactive_rewrite_hook_handler, NULL);\n \t\t}\n \t\tapply_autostash(opts);\n \ndiff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh\nindex 7344253bfb..f8ce32fe3b 100755\n--- a/t/t5407-post-rewrite-hook.sh\n+++ b/t/t5407-post-rewrite-hook.sh\n@@ -5,6 +5,7 @@\n \n test_description='Test the post-rewrite hook.'\n . ./test-lib.sh\n+. \"$TEST_DIRECTORY/lib-hooks.sh\"\n \n test_expect_success 'setup' '\n \ttest_commit A foo A &&\n@@ -263,4 +264,18 @@ test_expect_success 'git rebase -i (exec)' '\n \tverify_hook_input\n '\n \n+cmd_rebase () {\n+\tgit reset --hard D &&\n+\tFAKE_LINES=\"1 fixup 2\" git rebase -i B\n+}\n+\n+cmd_amend () {\n+\tgit reset --hard D &&\n+\techo \"D new message\" > newmsg &&\n+\tgit commit -Fnewmsg --amend\n+}\n+\n+test_multiple_hooks --ignore-exit-status post-rewrite cmd_rebase\n+test_multiple_hooks --ignore-exit-status post-rewrite cmd_amend\n+\n test_done\ndiff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\nindex ba8bd1b514..287e13e29d 100755\n--- a/t/t7505-prepare-commit-msg-hook.sh\n+++ b/t/t7505-prepare-commit-msg-hook.sh\n@@ -3,6 +3,7 @@\n test_description='prepare-commit-msg hook'\n \n . ./test-lib.sh\n+. \"$TEST_DIRECTORY/lib-hooks.sh\"\n \n test_expect_success 'set up commits for rebasing' '\n \ttest_commit root &&\n@@ -317,4 +318,12 @@ test_expect_success C_LOCALE_OUTPUT 'with failing hook (cherry-pick)' '\n \ttest $(grep -c prepare-commit-msg actual) = 1\n '\n \n+cherry_pick_command () {\n+\tgit checkout -f master &&\n+\tgit checkout -B other b &&\n+\tgit cherry-pick rebase-1\n+}\n+\n+test_multiple_hooks prepare-commit-msg cherry_pick_command\n+\n test_done\n"},{"id":"375449","messageId":"20190514002332.121089-6-sandals@crustytoothpaste.net","threadId":"51084","inReplyTo":"20190514002332.121089-1-sandals@crustytoothpaste.net","subject":"[PATCH v2 4/7] builtin/worktree: add support for multiple post-checkout hooks","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-05-14T00:23:29Z","receivedAt":"2019-05-14T00:24:11Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"Add support for multiple post-checkout hooks. We test only one possible\npath in the multiple hook case because the same code path is used for\nall checkouts and we know the hooks work from earlier assertions.\n\nSigned-off-by: brian m. carlson \n---\n builtin/worktree.c | 44 ++++++++++++++++++++++-------------\n t/t5403-post-checkout-hook.sh | 8 +++++++\n 2 files changed, 36 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex d2a7e2f3f1..1edcde8c84 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -262,6 +262,32 @@ static void validate_worktree_add(const char *path, const struct add_opts *opts)\n \tfree_worktrees(worktrees);\n }\n \n+struct hook_data {\n+\tstruct commit *commit;\n+\tconst char *dir;\n+};\n+\n+static int run_post_checkout_hook(const char *name, const char *path, void *p)\n+{\n+\tstruct hook_data *data = p;\n+\tstruct commit *commit = data->commit;\n+\tconst char *env[] = { \"GIT_DIR\", \"GIT_WORK_TREE\", NULL };\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\n+\tcp.git_cmd = 0;\n+\tcp.no_stdin = 1;\n+\tcp.stdout_to_stderr = 1;\n+\tcp.dir = data->dir;\n+\tcp.env = env;\n+\tcp.argv = NULL;\n+\tcp.trace2_hook_name = \"post-checkout\";\n+\targv_array_pushl(&cp.args, absolute_path(path),\n+\t\t\t oid_to_hex(&null_oid),\n+\t\t\t oid_to_hex(&commit->object.oid),\n+\t\t\t \"1\", NULL);\n+\treturn run_command(&cp);\n+}\n+\n static int add_worktree(const char *path, const char *refname,\n \t\t\tconst struct add_opts *opts)\n {\n@@ -395,22 +421,8 @@ static int add_worktree(const char *path, const char *refname,\n \t * is_junk is cleared, but do return appropriate code when hook fails.\n \t */\n \tif (!ret && opts->checkout) {\n-\t\tconst char *hook = find_hook(\"post-checkout\");\n-\t\tif (hook) {\n-\t\t\tconst char *env[] = { \"GIT_DIR\", \"GIT_WORK_TREE\", NULL };\n-\t\t\tcp.git_cmd = 0;\n-\t\t\tcp.no_stdin = 1;\n-\t\t\tcp.stdout_to_stderr = 1;\n-\t\t\tcp.dir = path;\n-\t\t\tcp.env = env;\n-\t\t\tcp.argv = NULL;\n-\t\t\tcp.trace2_hook_name = \"post-checkout\";\n-\t\t\targv_array_pushl(&cp.args, absolute_path(hook),\n-\t\t\t\t\t oid_to_hex(&null_oid),\n-\t\t\t\t\t oid_to_hex(&commit->object.oid),\n-\t\t\t\t\t \"1\", NULL);\n-\t\t\tret = run_command(&cp);\n-\t\t}\n+\t\tstruct hook_data data = { commit, path };\n+\t\tret = for_each_hook(\"post-checkout\", run_post_checkout_hook, &data);\n \t}\n \n \targv_array_clear(&child_env);\ndiff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh\nindex a39b3b5c78..aa265ce610 100755\n--- a/t/t5403-post-checkout-hook.sh\n+++ b/t/t5403-post-checkout-hook.sh\n@@ -5,6 +5,7 @@\n \n test_description='Test the post-checkout hook.'\n . ./test-lib.sh\n+. \"$TEST_DIRECTORY/lib-hooks.sh\"\n \n test_expect_success setup '\n \tmkdir -p .git/hooks &&\n@@ -73,4 +74,11 @@ test_expect_success 'post-checkout hook is triggered by clone' '\n \ttest -f clone3/.git/post-checkout.args\n '\n \n+cmd_rebase () {\n+\tgit checkout -B hook-test rebase-on-me^ &&\n+\tgit rebase rebase-on-me\n+}\n+\n+test_multiple_hooks post-checkout cmd_rebase\n+\n test_done\n"},{"id":"375450","messageId":"20190514002332.121089-7-sandals@crustytoothpaste.net","threadId":"51084","inReplyTo":"20190514002332.121089-1-sandals@crustytoothpaste.net","subject":"[PATCH v2 5/7] transport: add support for multiple pre-push hooks","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-05-14T00:23:30Z","receivedAt":"2019-05-14T00:24:12Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"Add support for multiple pre-push hooks.\n\nRemove find_hook since there are no longer any callers of it.\n\nSigned-off-by: brian m. carlson \n---\n run-command.c | 12 ------------\n run-command.h | 6 ------\n t/t5571-pre-push-hook.sh | 19 +++++++++++++++++++\n transport.c | 29 +++++++++++++++++++----------\n 4 files changed, 38 insertions(+), 28 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex eb075ac86b..191d6f6f7e 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1346,18 +1346,6 @@ static int has_hook(struct strbuf *path, int strip, int check)\n \treturn 1;\n }\n \n-const char *find_hook(const char *name)\n-{\n-\tstatic struct strbuf path = STRBUF_INIT;\n-\n-\tstrbuf_reset(&path);\n-\tstrbuf_git_path(&path, \"hooks/%s\", name);\n-\tif (has_hook(&path, 1, X_OK)) {\n-\t\treturn path.buf;\n-\t}\n-\treturn NULL;\n-}\n-\n int find_hooks(const char *name, struct string_list *list)\n {\n \tstruct strbuf path = STRBUF_INIT;\ndiff --git a/run-command.h b/run-command.h\nindex 1b3677fcac..15974e26d4 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -63,12 +63,6 @@ int finish_command(struct child_process *);\n int finish_command_in_signal(struct child_process *);\n int run_command(struct child_process *);\n \n-/*\n- * Returns the path to the hook file, or NULL if the hook is missing\n- * or disabled. Note that this points to static storage that will be\n- * overwritten by further calls to find_hook and run_hook_*.\n- */\n-extern const char *find_hook(const char *name);\n /*\n * Returns the paths to all hook files, or NULL if all hooks are missing or\n * disabled.\ndiff --git a/t/t5571-pre-push-hook.sh b/t/t5571-pre-push-hook.sh\nindex ac53d63869..754ad8eb93 100755\n--- a/t/t5571-pre-push-hook.sh\n+++ b/t/t5571-pre-push-hook.sh\n@@ -2,6 +2,7 @@\n \n test_description='check pre-push hooks'\n . ./test-lib.sh\n+. \"$TEST_DIRECTORY/lib-hooks.sh\"\n \n # Setup hook that always succeeds\n HOOKDIR=\"$(git rev-parse --git-dir)/hooks\"\n@@ -125,4 +126,22 @@ test_expect_success 'sigpipe does not cause pre-push hook failure' '\n \tgit push parent1 \"refs/heads/b/*:refs/heads/b/*\"\n '\n \n+push_command () {\n+\ttest_commit \"$1\" &&\n+\tgit push hooks refs/heads/master:refs/heads/master\n+}\n+\n+push_no_verify_command () {\n+\ttest_commit \"$1\" &&\n+\tgit push --no-verify hooks refs/heads/master:refs/heads/master\n+}\n+\n+test_expect_success 'setup' '\n+\tgit checkout master &&\n+\tgit init --bare hooktest &&\n+\tgit remote add hooks hooktest\n+'\n+\n+test_multiple_hooks pre-push push_command push_no_verify_command\n+\n test_done\ndiff --git a/transport.c b/transport.c\nindex 365ea574c7..7672f4fb57 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1042,20 +1042,23 @@ static void die_with_unpushed_submodules(struct string_list *needs_pushing)\n \tdie(_(\"Aborting.\"));\n }\n \n-static int run_pre_push_hook(struct transport *transport,\n-\t\t\t struct ref *remote_refs)\n+struct pre_push_hook_data {\n+\tstruct transport *transport;\n+\tstruct ref *remote_refs;\n+};\n+\n+static int do_run_pre_push_hook(const char *name, const char *path, void *p)\n {\n+\tstruct pre_push_hook_data *data = p;\n+\tstruct child_process proc = CHILD_PROCESS_INIT;\n \tint ret = 0, x;\n \tstruct ref *r;\n-\tstruct child_process proc = CHILD_PROCESS_INIT;\n \tstruct strbuf buf;\n \tconst char *argv[4];\n \n-\tif (!(argv[0] = find_hook(\"pre-push\")))\n-\t\treturn 0;\n-\n-\targv[1] = transport->remote->name;\n-\targv[2] = transport->url;\n+\targv[0] = path;\n+\targv[1] = data->transport->remote->name;\n+\targv[2] = data->transport->url;\n \targv[3] = NULL;\n \n \tproc.argv = argv;\n@@ -1071,7 +1074,7 @@ static int run_pre_push_hook(struct transport *transport,\n \n \tstrbuf_init(&buf, 256);\n \n-\tfor (r = remote_refs; r; r = r->next) {\n+\tfor (r = data->remote_refs; r; r = r->next) {\n \t\tif (!r->peer_ref) continue;\n \t\tif (r->status == REF_STATUS_REJECT_NONFASTFORWARD) continue;\n \t\tif (r->status == REF_STATUS_REJECT_STALE) continue;\n@@ -1101,10 +1104,16 @@ static int run_pre_push_hook(struct transport *transport,\n \tx = finish_command(&proc);\n \tif (!ret)\n \t\tret = x;\n-\n \treturn ret;\n }\n \n+static int run_pre_push_hook(struct transport *transport,\n+\t\t\t struct ref *remote_refs)\n+{\n+\tstruct pre_push_hook_data data = { transport, remote_refs };\n+\treturn for_each_hook(\"pre-push\", do_run_pre_push_hook, &data);\n+}\n+\n int transport_push(struct repository *r,\n \t\t struct transport *transport,\n \t\t struct refspec *rs, int flags,\n"},{"id":"375451","messageId":"20190514002332.121089-8-sandals@crustytoothpaste.net","threadId":"51084","inReplyTo":"20190514002332.121089-1-sandals@crustytoothpaste.net","subject":"[PATCH v2 6/7] config: allow configuration of multiple hook error behavior","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-05-14T00:23:31Z","receivedAt":"2019-05-14T00:24:13Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"There are a variety of situations in which a user may want an error\nbehavior for multiple hooks other than the default. Add a config option,\nhook..errorBehavior to allow users to customize this behavior on a\nper-hook basis. Provide options for the default behavior (exiting\nearly), executing all hooks and succeeding if all hooks succeed, or\nexecuting all hooks and succeeding if any hook succeeds.\n\nSigned-off-by: brian m. carlson \n---\n config.c | 27 +++++++++++++\n run-command.c | 42 +++++++++++++++++---\n run-command.h | 5 +++\n t/lib-hooks.sh | 106 ++++++++++++++++++++++++++++++++++++++++++++++++-\n 4 files changed, 173 insertions(+), 7 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex c2846df3f1..9cba4061a9 100644\n--- a/config.c\n+++ b/config.c\n@@ -19,6 +19,7 @@\n #include \"utf8.h\"\n #include \"dir.h\"\n #include \"color.h\"\n+#include \"run-command.h\"\n \n struct config_source {\n \tstruct config_source *prev;\n@@ -1093,6 +1094,29 @@ int git_config_color(char *dest, const char *var, const char *value)\n \treturn 0;\n }\n \n+static int git_default_hook_config(const char *key, const char *value)\n+{\n+\tconst char *hook;\n+\tsize_t key_len;\n+\tuintptr_t behavior;\n+\n+\tkey += strlen(\"hook.\");\n+\tif (strip_suffix(key, \".errorbehavior\", &key_len)) {\n+\t\thook = xmemdupz(key, key_len);\n+\t\tif (!strcmp(value, \"stop-on-first\"))\n+\t\t\tbehavior = HOOK_ERROR_STOP_ON_FIRST;\n+\t\telse if (!strcmp(value, \"report-any-error\"))\n+\t\t\tbehavior = HOOK_ERROR_REPORT_ANY_ERROR;\n+\t\telse if (!strcmp(value, \"report-any-success\"))\n+\t\t\tbehavior = HOOK_ERROR_REPORT_ANY_SUCCESS;\n+\t\telse\n+\t\t\tdie(_(\"invalid mode for hook %s error behavior: %s\"), hook, value);\n+\t\tstring_list_insert(&hook_error_behavior, hook)->util = (void *)behavior;\n+\t\treturn 0;\n+\t}\n+\treturn 0;\n+}\n+\n static int git_default_core_config(const char *var, const char *value, void *cb)\n {\n \t/* This needs a better name */\n@@ -1450,6 +1474,9 @@ int git_default_config(const char *var, const char *value, void *cb)\n \t starts_with(var, \"committer.\"))\n \t\treturn git_ident_config(var, value, cb);\n \n+\tif (starts_with(var, \"hook.\"))\n+\t\treturn git_default_hook_config(var, value);\n+\n \tif (starts_with(var, \"i18n.\"))\n \t\treturn git_default_i18n_config(var, value);\n \ndiff --git a/run-command.c b/run-command.c\nindex 191d6f6f7e..70fb19a55b 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1308,6 +1308,8 @@ int async_with_fork(void)\n #endif\n }\n \n+struct string_list hook_error_behavior = STRING_LIST_INIT_NODUP;\n+\n /*\n * Return 1 if a hook exists at path (which may be modified) using access(2)\n * with check (which should be F_OK or X_OK), 0 otherwise. If strip is true,\n@@ -1401,18 +1403,48 @@ int for_each_hook(const char *name,\n \t\t void *data)\n {\n \tstruct string_list paths = STRING_LIST_INIT_DUP;\n-\tint i, ret = 0;\n+\tint i, hret = 0;\n+\tuintptr_t behavior = HOOK_ERROR_STOP_ON_FIRST;\n+\tstruct string_list_item *item;\n+\t/* Use -2 as sentinel because failure to exec is -1. */\n+\tint ret = -2;\n+\n+\titem = string_list_lookup(&hook_error_behavior, name);\n+\tif (item)\n+\t\tbehavior = (uintptr_t)item->util;\n \n \tfind_hooks(name, &paths);\n \tfor (i = 0; i < paths.nr; i++) {\n \t\tconst char *p = paths.items[i].string;\n \n-\t\tret = handler(name, p, data);\n-\t\tif (ret)\n-\t\t\tbreak;\n+\t\thret = handler(name, p, data);\n+\t\tswitch (behavior) {\n+\t\t\tcase HOOK_ERROR_STOP_ON_FIRST:\n+\t\t\t\tif (hret) {\n+\t\t\t\t\tret = hret;\n+\t\t\t\t\tgoto out;\n+\t\t\t\t}\n+\t\t\t\tbreak;\n+\t\t\tcase HOOK_ERROR_REPORT_ANY_SUCCESS:\n+\t\t\t\tif (ret == -2)\n+\t\t\t\t\tret = 1;\n+\t\t\t\tif (!hret)\n+\t\t\t\t\tret = 0;\n+\t\t\t\tbreak;\n+\t\t\tcase HOOK_ERROR_REPORT_ANY_ERROR:\n+\t\t\t\tif (ret == -2)\n+\t\t\t\t\tret = 0;\n+\t\t\t\tif (hret)\n+\t\t\t\t\tret = hret;\n+\t\t\t\tbreak;\n+\t\t\tdefault:\n+\t\t\t\tBUG(\"unknown hook error behavior\");\n+\t\t}\n \t}\n-\n+out:\n \tstring_list_clear(&paths, 0);\n+\tif (ret == -2)\n+\t\treturn 0;\n \treturn ret;\n }\n \ndiff --git a/run-command.h b/run-command.h\nindex 15974e26d4..879ebb768f 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -63,6 +63,11 @@ int finish_command(struct child_process *);\n int finish_command_in_signal(struct child_process *);\n int run_command(struct child_process *);\n \n+#define HOOK_ERROR_STOP_ON_FIRST 1\n+#define HOOK_ERROR_REPORT_ANY_ERROR 2\n+#define HOOK_ERROR_REPORT_ANY_SUCCESS 3\n+extern struct string_list hook_error_behavior;\n+\n /*\n * Returns the paths to all hook files, or NULL if all hooks are missing or\n * disabled.\ndiff --git a/t/lib-hooks.sh b/t/lib-hooks.sh\nindex 721250aea0..c1d7688313 100644\n--- a/t/lib-hooks.sh\n+++ b/t/lib-hooks.sh\n@@ -121,7 +121,7 @@ test_multiple_hooks () {\n \t\t! test -f \"$OUTPUTDIR/c\"\n \t'\n \n-\ttest_expect_success \"$hook: multiple hooks, all successful\" '\n+\ttest_expect_success \"$hook: multiple hooks, all successful by default\" '\n \t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n \t\trm -f \"$HOOK\" &&\n \t\tcreate_multihooks 0 0 0 &&\n@@ -131,7 +131,40 @@ test_multiple_hooks () {\n \t\ttest -f \"$OUTPUTDIR/c\"\n \t'\n \n-\ttest_expect_success \"$hook: hooks after first failure not executed\" '\n+\ttest_expect_success \"$hook: multiple hooks, all successful with stop-on-first\" '\n+\t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n+\t\ttest_config \"hook.$hook.errorbehavior\" stop-on-first &&\n+\t\trm -f \"$HOOK\" &&\n+\t\tcreate_multihooks 0 0 0 &&\n+\t\t$cmd content-stop-on-first &&\n+\t\ttest -f \"$OUTPUTDIR/a\" &&\n+\t\ttest -f \"$OUTPUTDIR/b\" &&\n+\t\ttest -f \"$OUTPUTDIR/c\"\n+\t'\n+\n+\ttest_expect_success \"$hook: multiple hooks, all successful with report-any-error\" '\n+\t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n+\t\ttest_config \"hook.$hook.errorbehavior\" report-any-error &&\n+\t\trm -f \"$HOOK\" &&\n+\t\tcreate_multihooks 0 0 0 &&\n+\t\t$cmd content-report-any-error &&\n+\t\ttest -f \"$OUTPUTDIR/a\" &&\n+\t\ttest -f \"$OUTPUTDIR/b\" &&\n+\t\ttest -f \"$OUTPUTDIR/c\"\n+\t'\n+\n+\ttest_expect_success \"$hook: multiple hooks, all successful with report-any-success\" '\n+\t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n+\t\ttest_config \"hook.$hook.errorbehavior\" report-any-success &&\n+\t\trm -f \"$HOOK\" &&\n+\t\tcreate_multihooks 0 0 0 &&\n+\t\t$cmd content-report-any-success &&\n+\t\ttest -f \"$OUTPUTDIR/a\" &&\n+\t\ttest -f \"$OUTPUTDIR/b\" &&\n+\t\ttest -f \"$OUTPUTDIR/c\"\n+\t'\n+\n+\ttest_expect_success \"$hook: hooks after first failure not executed by default\" '\n \t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n \t\tcreate_multihooks 0 1 0 &&\n \t\t$must_fail $cmd more-content &&\n@@ -140,6 +173,75 @@ test_multiple_hooks () {\n \t\t! test -f \"$OUTPUTDIR/c\"\n \t'\n \n+\ttest_expect_success \"$hook: hooks after first failure not executed with stop-on-first\" '\n+\t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n+\t\ttest_config \"hook.$hook.errorbehavior\" stop-on-first &&\n+\t\tcreate_multihooks 0 1 0 &&\n+\t\t$must_fail $cmd more-content-stop-on-first &&\n+\t\ttest -f \"$OUTPUTDIR/a\" &&\n+\t\ttest -f \"$OUTPUTDIR/b\" &&\n+\t\t! test -f \"$OUTPUTDIR/c\"\n+\t'\n+\n+\ttest_expect_success \"$hook: hooks after first failure executed with report-any-error\" '\n+\t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n+\t\ttest_config \"hook.$hook.errorbehavior\" report-any-error &&\n+\t\tcreate_multihooks 0 1 0 &&\n+\t\t$must_fail $cmd more-content-report-any-error &&\n+\t\ttest -f \"$OUTPUTDIR/a\" &&\n+\t\ttest -f \"$OUTPUTDIR/b\" &&\n+\t\ttest -f \"$OUTPUTDIR/c\"\n+\t'\n+\n+\ttest_expect_success \"$hook: hooks after first failure executed with report-any-success\" '\n+\t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n+\t\ttest_config \"hook.$hook.errorbehavior\" report-any-success &&\n+\t\tcreate_multihooks 0 1 0 &&\n+\t\t$cmd more-content-report-any-success &&\n+\t\ttest -f \"$OUTPUTDIR/a\" &&\n+\t\ttest -f \"$OUTPUTDIR/b\" &&\n+\t\ttest -f \"$OUTPUTDIR/c\"\n+\t'\n+\n+\ttest_expect_success \"$hook: failing hooks by default\" '\n+\t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n+\t\tcreate_multihooks 1 1 1 &&\n+\t\t$must_fail $cmd most-content &&\n+\t\ttest -f \"$OUTPUTDIR/a\" &&\n+\t\t! test -f \"$OUTPUTDIR/b\" &&\n+\t\t! test -f \"$OUTPUTDIR/c\"\n+\t'\n+\n+\ttest_expect_success \"$hook: failing hooks with stop-on-first\" '\n+\t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n+\t\ttest_config \"hook.$hook.errorbehavior\" stop-on-first &&\n+\t\tcreate_multihooks 1 1 1 &&\n+\t\t$must_fail $cmd most-content-stop-on-first &&\n+\t\ttest -f \"$OUTPUTDIR/a\" &&\n+\t\t! test -f \"$OUTPUTDIR/b\" &&\n+\t\t! test -f \"$OUTPUTDIR/c\"\n+\t'\n+\n+\ttest_expect_success \"$hook: failing hooks with report-any-error\" '\n+\t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n+\t\ttest_config \"hook.$hook.errorbehavior\" report-any-error &&\n+\t\tcreate_multihooks 1 1 1 &&\n+\t\t$must_fail $cmd most-content-report-any-error &&\n+\t\ttest -f \"$OUTPUTDIR/a\" &&\n+\t\ttest -f \"$OUTPUTDIR/b\" &&\n+\t\ttest -f \"$OUTPUTDIR/c\"\n+\t'\n+\n+\ttest_expect_success \"$hook: failing hooks with report-any-success\" '\n+\t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n+\t\ttest_config \"hook.$hook.errorbehavior\" report-any-success &&\n+\t\tcreate_multihooks 1 1 1 &&\n+\t\t$must_fail $cmd most-content-report-any-success &&\n+\t\ttest -f \"$OUTPUTDIR/a\" &&\n+\t\ttest -f \"$OUTPUTDIR/b\" &&\n+\t\ttest -f \"$OUTPUTDIR/c\"\n+\t'\n+\n \ttest_expect_success POSIXPERM \"$hook: non-executable hook not executed\" '\n \t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n \t\tcreate_multihooks 0 1 0 &&\n"},{"id":"375452","messageId":"20190514002332.121089-9-sandals@crustytoothpaste.net","threadId":"51084","inReplyTo":"20190514002332.121089-1-sandals@crustytoothpaste.net","subject":"[PATCH v2 7/7] docs: document multiple hooks","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-05-14T00:23:32Z","receivedAt":"2019-05-14T00:24:15Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"Document the semantics and behavior of multiple hooks, including the\nconfiguration options and requirements for them to be run.\n\nSigned-off-by: brian m. carlson \n---\n Documentation/config.txt | 2 ++\n Documentation/config/hook.txt | 19 +++++++++++++++++++\n Documentation/githooks.txt | 9 +++++++++\n 3 files changed, 30 insertions(+)\n create mode 100644 Documentation/config/hook.txt\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex d87846faa6..f62b8ce494 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -350,6 +350,8 @@ include::config/guitool.txt[]\n \n include::config/help.txt[]\n \n+include::config/hook.txt[]\n+\n include::config/http.txt[]\n \n include::config/i18n.txt[]\ndiff --git a/Documentation/config/hook.txt b/Documentation/config/hook.txt\nnew file mode 100644\nindex 0000000000..4585cc7f55\n--- /dev/null\n+++ b/Documentation/config/hook.txt\n@@ -0,0 +1,19 @@\n+hook..errorBehavior::\n+ Control the error behavior when using multiple hooks.\n++\n+--\n+* `stop-on-first` - If a hook fails, do not execute further hooks, even if the\n+ command normally ignores whether hooks succeed or fail, and return its exit\n+ code as the exit code of the hook set. If all hooks succeed, the exit code is 0.\n+ This is the default.\n+* `report-any-error` - Always execute all hooks, but return the exit code of the\n+ first failing hook as the exit code of the hook set. If all hooks succeed, the\n+ exit code of the hook set is 0.\n+* `report-any-success` - Always execute all hooks, and if any hook succeeds,\n+ return 0 as the exit code of the hook set. If all hooks fail, the exit code of\n+ the hook set is 1.\n+--\n++\n+If the exit code of the hook set is zero, then the hooks are considered to have\n+succeeded; otherwise, they are considered to have failed. Note that the success\n+or failure of some hooks is ignored (see linkgit:githooks[5] for more).\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 786e778ab8..c680e575b3 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -31,6 +31,15 @@ Hooks can get their arguments via the environment, command-line\n arguments, and stdin. See the documentation for each hook below for\n details.\n \n+It is possible to provide multiple hooks for a single function. If the\n+main hook file is absent, hooks are additionally looked for in a\n+directory with the name of the main hook file with a `.d` appended.\n+(That is, if `post-receive` is missing, `post-receive.d` is inspected\n+for any hooks that might be present.) Each of these hooks is executed in order,\n+sorted by file name. By default, if a hook fails, additional hooks are not\n+executed, but this can be controlled with the `hook.*.errorBehavior` setting\n+(see linkgit:git-config[1]).\n+\n `git init` may copy hooks to the new repository, depending on its\n configuration. See the \"TEMPLATE DIRECTORY\" section in\n linkgit:git-init[1] for details. When the rest of this document refers\n"},{"id":"375455","messageId":"20190514004920.GB136746@google.com","threadId":"51084","inReplyTo":"20190514002332.121089-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 0/7] Multiple hook support","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-05-14T00:51:01Z","receivedAt":"2019-05-14T00:51:07Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nbrian m. carlson wrote:\n\n> I've thought a lot about the discussion over whether this series should\n> use the configuration as the source for multiple hooks. Ultimately, I've\n> come to the decision that it's not a good idea. Even adopting the empty\n> entry as a reset marker, the fact that inheritance in the configuration\n> is in-order and can't be easily modified means that it's not likely to\n> be very useful, but it is likely to be quite surprising for the average\n> user.\n\nCan we discuss this some more? What would it take to make it likely\nto be useful in your view?\n\n> I think a solution that sticks with the existing model and builds\n> off a design used by other systems people are familiar with, like cron\n> and run-parts, is going to be a better choice. Moreover, this is the\n> design that people have already built with outside tooling, which is a\n> further argument in favor of it.\n\nTo be clear, the main advantage I see for config versus the .git/hooks\nmodel is that with the config model, a user doesn't have to search\nthroughout the filesystem for .git/hooks directories to update when a\nhook is broken.\n\nThere are other models that similarly fix that --- e.g. putting hooks\nin /etc. But as long as they're in .git/hooks, I think we're digging\nourselves deeper into the same hole.\n\nThanks,\nJonathan\n"},{"id":"375457","messageId":"20190514015928.GG7458@genre.crustytoothpaste.net","threadId":"51084","inReplyTo":"20190514004920.GB136746@google.com","subject":"Re: [PATCH v2 0/7] Multiple hook support","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-05-14T01:59:28Z","receivedAt":"2019-05-14T01:59:39Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Mon, May 13, 2019 at 05:51:01PM -0700, Jonathan Nieder wrote:\n> Hi,\n> \n> brian m. carlson wrote:\n> \n> > I've thought a lot about the discussion over whether this series should\n> > use the configuration as the source for multiple hooks. Ultimately, I've\n> > come to the decision that it's not a good idea. Even adopting the empty\n> > entry as a reset marker, the fact that inheritance in the configuration\n> > is in-order and can't be easily modified means that it's not likely to\n> > be very useful, but it is likely to be quite surprising for the average\n> > user.\n> \n> Can we discuss this some more? What would it take to make it likely\n> to be useful in your view?\n\nThere are two aspects here which I think are worth discussing. Let's\ndiscuss the inheritance issue first.\n\nOrder with multiple hooks matters. The best hook as an example for this\nis prepare-commit-msg. If I have a hook which I want to run on every\nrepository, such as a hook that inserts some sort of ID (bug tracker,\nGerrit, etc.), that hook, due to inheritance, *has* to be first, before\nany other prepare-commit-msg hooks. If I want a hook that runs before\nit, perhaps because a particular repository needs a different\nconfiguration, I have to wipe the list and insert both hooks. I'm now\nmaintaining two separate locations for the command lines instead of just\ninserting a symlink to the global hook and dropping a new hook before\nit.\n\nI don't think there's a good way to make it easier unless we radically\ncustomize the way configuration is done. I don't doubt that there are a\nsmall number of configurations where the inheritance behavior is fine—I\nbelieve GitHub's backend is one of them—but overall I think it's hard to\nreason about and customize.\n\nThe second issue here is that it's surprising. Users don't know how to\nreset a configuration option because we don't have a consistent way to\ndo that. Users will not expect for there to be multiple ways to set\nhooks. Users will also not expect that their hooks in their\nconfiguration aren't run if there are hooks in .git/hooks. Tooling that\nhas so far used .git/hooks will compete with users' global configuration\noptions, which I guarantee you will be a surprise for users using older\nversions of tools.\n\nThe new behavior, which puts everything in the same directory\n(.git/hooks) is much easier to reason about. I think we're obligated to\nconsider the experience for the average end user, who may not be\nintimately familiar with how Git works but still needs to use it to get\nwork done.\n\nIt also provides a convenient place for hooks to live, which a\nconfig-based option doesn't. We'll need to invoke things using /bin/sh,\nso will they all have to live in PATH? What about one-offs that don't\nreally belong in PATH?\n\n> > I think a solution that sticks with the existing model and builds\n> > off a design used by other systems people are familiar with, like cron\n> > and run-parts, is going to be a better choice. Moreover, this is the\n> > design that people have already built with outside tooling, which is a\n> > further argument in favor of it.\n> \n> To be clear, the main advantage I see for config versus the .git/hooks\n> model is that with the config model, a user doesn't have to search\n> throughout the filesystem for .git/hooks directories to update when a\n> hook is broken.\n\nI agree this is an advantage if they don't hit the ordering issue. I\nthink a lot of the common use cases where this approach has benefits can\nbe handled well with core.hooksPath and hooks that can turn themselves\non or off depending on the repository config.\n\nWhat might be an interesting approach that would address these concerns\nis a core.globalHooks[0] option that points to a set (or sets,\ndepending) of multiple-hook directories. We then enumerate hooks in sort\norder, considering both the global and the local directories as one\nunit, perhaps with some way of disabling hooks. I'm not planning on\nworking on this myself, but I wouldn't be opposed to seeing someone else\nwork on it.\n\n[0] Better name suggestions are, of course, welcome.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"375461","messageId":"20190514022653.GA34155@google.com","threadId":"51084","inReplyTo":"20190514015928.GG7458@genre.crustytoothpaste.net","subject":"Re: [PATCH v2 0/7] Multiple hook support","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-05-14T02:26:53Z","receivedAt":"2019-05-14T02:26:59Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nbrian m. carlson wrote:\n> On Mon, May 13, 2019 at 05:51:01PM -0700, Jonathan Nieder wrote:\n>> brian m. carlson wrote:\n\n>>> the fact that inheritance in the configuration\n>>> is in-order and can't be easily modified means that it's not likely to\n>>> be very useful, but it is likely to be quite surprising for the average\n>>> user.\n>>\n>> Can we discuss this some more? What would it take to make it likely\n>> to be useful in your view?\n>\n> There are two aspects here which I think are worth discussing. Let's\n> discuss the inheritance issue first.\n>\n> Order with multiple hooks matters. The best hook as an example for this\n> is prepare-commit-msg. If I have a hook which I want to run on every\n> repository, such as a hook that inserts some sort of ID (bug tracker,\n> Gerrit, etc.), that hook, due to inheritance, *has* to be first, before\n> any other prepare-commit-msg hooks. If I want a hook that runs before\n> it, perhaps because a particular repository needs a different\n> configuration, I have to wipe the list and insert both hooks. I'm now\n> maintaining two separate locations for the command lines instead of just\n> inserting a symlink to the global hook and dropping a new hook before\n> it.\n>\n> I don't think there's a good way to make it easier unless we radically\n> customize the way configuration is done.\n\nWouldn't a separate config item e.g. to reverse order (or to perform\nwhatever other customization seems appropriate) cover this?\n\nIn other words, use the standard config convention for the set of\nhooks, and treat the order in which they are invoked as a separate\nquestion. You could even use the hooks.d style alphabetical order\nconvention.\n\n[...]\n> The second issue here is that it's surprising. Users don't know how to\n> reset a configuration option because we don't have a consistent way to\n> do that.\n\nI agree that it's underdocumented and underimplemented. But I'm not\naware of any other method that Git provides to reset a configuration\nitem. What is it inconsistent with?\n\n> Users will not expect for there to be multiple ways to set\n> hooks. Users will also not expect that their hooks in their\n> configuration aren't run if there are hooks in .git/hooks. Tooling that\n> has so far used .git/hooks will compete with users' global configuration\n> options, which I guarantee you will be a surprise for users using older\n> versions of tools.\n\nIndeed, in the long term I think we should remove the .git/hooks/\nmechanism entirely.\n\nIn the shorter term, I think the kind of inconsistency you're referring\nto applies to hooks.d as well.\n\n> The new behavior, which puts everything in the same directory\n> (.git/hooks) is much easier to reason about.\n\nThat's a good point: a .git/hooks/README sounds like it would be\nhelpful here.\n\n[...]\n> It also provides a convenient place for hooks to live, which a\n> config-based option doesn't. We'll need to invoke things using /bin/sh,\n> so will they all have to live in PATH? What about one-offs that don't\n> really belong in PATH?\n\nThis hasn't been a problem for remote helpers, merge drivers, etc in\nthe past. Why are hooks different?\n\nTo be clear, I think it's a reasonable problem to solve, and I've\nactually been surprised that it hasn't been a problem for people.\n\n[...]\n> I agree this is an advantage if they don't hit the ordering issue.\n\nWonderful. Sounds like if I do some work on the ordering issue, then\nwe have a path forward.\n\n> I\n> think a lot of the common use cases where this approach has benefits can\n> be handled well with core.hooksPath and hooks that can turn themselves\n> on or off depending on the repository config.\n\nI think core.hooksPath attempted to solve this problem, but it has\nseveral deficiencies:\n\n1. It assumes a single, centrally managed hooks directory, and there's\n no standard for where that directory lives. This means that it\n can't be counted on by tools like \"git secrets\" --- instead, each\n particular installation has to set up a custom hooks directory for\n themselves.\n\n2. Since it assumes a single, centrally managed hooks directory,\n customizations in a single repository (e.g. to enable or disable a\n single hook) require duplicating the entire directory.\n\n3. It had no migration path defined to becoming the default, so it\n doesn't end up being discoverable. core.hooksPath is designed as\n a special case, making it hard to debug, instead of being a\n mainstream setting that can be recommended as a future default.\n\n> What might be an interesting approach that would address these concerns\n> is a core.globalHooks[0] option that points to a set (or sets,\n> depending) of multiple-hook directories. We then enumerate hooks in sort\n> order, considering both the global and the local directories as one\n> unit, perhaps with some way of disabling hooks. I'm not planning on\n> working on this myself, but I wouldn't be opposed to seeing someone else\n> work on it.\n\nThis sounds overflexible to me. Because of that, I don't think it\nwould end up as a default, so we wouldn't have a path to improving our\nsecurity stature.\n\nIf I implement a config based multiple hooks feature with name based\nordering, would that be useful to you?\n\nThanks,\nJonathan\n\n> [0] Better name suggestions are, of course, welcome.\n"},{"id":"375528","messageId":"CACsJy8Ba98baQ2wZnMZyEva6gxO1ROZ4qJFTOdrCUXDMwrHnXA@mail.gmail.com","threadId":"51084","inReplyTo":"20190514002332.121089-2-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 1/7] run-command: add preliminary support for multiple hooks","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-14T12:46:17Z","receivedAt":"2019-05-14T12:46:45Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, May 14, 2019 at 7:24 AM brian m. carlson\n wrote:\n\n> -int run_hook_ve(const char *const *env, const char *name, va_list args)\n> +int find_hooks(const char *name, struct string_list *list)\n> {\n> - struct child_process hook = CHILD_PROCESS_INIT;\n> - const char *p;\n> + struct strbuf path = STRBUF_INIT;\n> + DIR *d;\n> + struct dirent *de;\n>\n> - p = find_hook(name);\n> - if (!p)\n> + /*\n> + * We look for a single hook. If present, return it, and skip the\n> + * individual directories.\n> + */\n> + strbuf_git_path(&path, \"hooks/%s\", name);\n> + if (has_hook(&path, 1, X_OK)) {\n> + if (list)\n> + string_list_append(list, path.buf);\n> + return 1;\n> + }\n> +\n> + if (has_hook(&path, 1, F_OK))\n> return 0;\n>\n> - argv_array_push(&hook.args, p);\n> - while ((p = va_arg(args, const char *)))\n> - argv_array_push(&hook.args, p);\n> - hook.env = env;\n> + strbuf_reset(&path);\n> + strbuf_git_path(&path, \"hooks/%s.d\", name);\n> + d = opendir(path.buf);\n> + if (!d) {\n> + if (list)\n> + string_list_clear(list, 0);\n> + return 0;\n> + }\n> + while ((de = readdir(d))) {\n> + if (!strcmp(de->d_name, \".\") || !strcmp(de->d_name, \"..\"))\n> + continue;\n> + strbuf_reset(&path);\n> + strbuf_git_path(&path, \"hooks/%s.d/%s\", name, de->d_name);\n> + if (has_hook(&path, 0, X_OK)) {\n\nDo we want to support hooks in subdirectories as well (if so, using\ndir-iterator.h might be more appropriate)\n\nIf not, what happens when \"path\" is a directory. X_OK could be set\n(and often are) on them too.\n\n> + if (list)\n> + string_list_append(list, path.buf);\n> + else\n> + return 1;\n> + }\n> + }\n> + closedir(d);\n> + strbuf_reset(&path);\n> + if (!list->nr) {\n> + return 0;\n> + }\n> +\n> + string_list_sort(list);\n\nThis is going to be interesting on case-insensitive filesystems\nbecause we do strcmp by default, not the friendlier fspathcmp. And the\n\".exe\" suffix might affect sort order too.\n\nBut I suppose we just need to be clear here (in documentation). They\ncan always prefix with a number to keep hook files in expected order.\n\n> + return 1;\n> +}\n> +\n\n> diff --git a/run-command.h b/run-command.h\n> index a6950691c0..1b3677fcac 100644\n> --- a/run-command.h\n> +++ b/run-command.h\n> @@ -4,6 +4,7 @@\n> #include \"thread-utils.h\"\n>\n> #include \"argv-array.h\"\n> +#include \"string-list.h\"\n\n'struct string_list;' should be enough (and a bit lighter) although I\ndon't suppose it really matters.\n\n>\n> struct child_process {\n> const char **argv;\n-- \nDuy\n"},{"id":"375529","messageId":"CACsJy8CnCoMK7ahjRotKfiC2ic-U0hdhKuQAeF-TtVt8N+bU2Q@mail.gmail.com","threadId":"51084","inReplyTo":"20190514002332.121089-4-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 3/7] rebase: add support for multiple hooks","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-14T12:56:49Z","receivedAt":"2019-05-14T12:57:17Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, May 14, 2019 at 7:24 AM brian m. carlson\n wrote:\n> diff --git a/builtin/am.c b/builtin/am.c\n> index 912d9821b1..340eacbd44 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -441,24 +441,8 @@ static int run_applypatch_msg_hook(struct am_state *state)\n> */\n> static int run_post_rewrite_hook(const struct am_state *state)\n> {\n> - struct child_process cp = CHILD_PROCESS_INIT;\n> - const char *hook = find_hook(\"post-rewrite\");\n> - int ret;\n> -\n> - if (!hook)\n> - return 0;\n> -\n> - argv_array_push(&cp.args, hook);\n> - argv_array_push(&cp.args, \"rebase\");\n> -\n> - cp.in = xopen(am_path(state, \"rewritten\"), O_RDONLY);\n> - cp.stdout_to_stderr = 1;\n> - cp.trace2_hook_name = \"post-rewrite\";\n> -\n> - ret = run_command(&cp);\n> -\n> - close(cp.in);\n\nIn the old code, we close cp.in...\n\n> +int post_rewrite_rebase_hook(const char *name, const char *path, void *input)\n> +{\n> + struct child_process child = CHILD_PROCESS_INIT;\n> +\n> + child.in = open(input, O_RDONLY);\n> + child.stdout_to_stderr = 1;\n> + child.trace2_hook_name = \"post-rewrite\";\n\nmaybe use \"name\" and avoid hard coding \"post-rewrite\".\n\n> + argv_array_push(&child.args, path);\n> + argv_array_push(&child.args, \"rebase\");\n> + return run_command(&child);\n\n... but in the new one we don't. Smells fd leaking to me.\n-- \nDuy\n"},{"id":"375531","messageId":"CACsJy8BN32Jm62MQPLsRwKST8DDNZ7NxnJbsqBO3PJ0K2qU-Ng@mail.gmail.com","threadId":"51084","inReplyTo":"20190514002332.121089-8-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 6/7] config: allow configuration of multiple hook error behavior","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-14T13:20:10Z","receivedAt":"2019-05-14T13:20:39Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, May 14, 2019 at 7:24 AM brian m. carlson\n wrote:\n>\n> There are a variety of situations in which a user may want an error\n> behavior for multiple hooks other than the default. Add a config option,\n> hook..errorBehavior to allow users to customize this behavior on a\n\nAn alternative name is onError, probably more often used for event\ncallbacks. But I don't know, maybe errorBehavior is actually better.\n\n> per-hook basis. Provide options for the default behavior (exiting\n\nshould we fall back to hook.errorBehavior? That allows people to set\nglobal policy, then customize just a small set of weird hooks.\n\n> early), executing all hooks and succeeding if all hooks succeed, or\n> executing all hooks and succeeding if any hook succeeds.\n> Signed-off-by: brian m. carlson \n> ---\n> config.c | 27 +++++++++++++\n> run-command.c | 42 +++++++++++++++++---\n> run-command.h | 5 +++\n> t/lib-hooks.sh | 106 ++++++++++++++++++++++++++++++++++++++++++++++++-\n> 4 files changed, 173 insertions(+), 7 deletions(-)\n>\n> diff --git a/config.c b/config.c\n> index c2846df3f1..9cba4061a9 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -19,6 +19,7 @@\n> #include \"utf8.h\"\n> #include \"dir.h\"\n> #include \"color.h\"\n> +#include \"run-command.h\"\n>\n> struct config_source {\n> struct config_source *prev;\n> @@ -1093,6 +1094,29 @@ int git_config_color(char *dest, const char *var, const char *value)\n> return 0;\n> }\n>\n> +static int git_default_hook_config(const char *key, const char *value)\n> +{\n> + const char *hook;\n> + size_t key_len;\n> + uintptr_t behavior;\n> +\n> + key += strlen(\"hook.\");\n> + if (strip_suffix(key, \".errorbehavior\", &key_len)) {\n> + hook = xmemdupz(key, key_len);\n> + if (!strcmp(value, \"stop-on-first\"))\n\nmaybe stop-on-first-error (or if you go with the \"onError\" name, I\nthink \"stop\" is enough). I know \"stop on/after first hook\" does not\nreally make any sense when you think about it. Maybe stop-on-first is\nsufficient.\n\nI was going to suggest strcasecmp. But core.whitespace (also has\nmultiple-word-values) already sets a precedent on strcmp. I think\nwe're good. Or mostly good, I don't know, we still accept False, false\nand FALSE.\n\n> + behavior = HOOK_ERROR_STOP_ON_FIRST;\n\nThis is basically the logical \"and\" behavior in a C expression. Which\nmakes me think if anybody's crazy enough to need the \"or\" counterpart\n(i.e. run hooks, expect failure, keep going until the first success).\n\nI guess it's a crazy mode. We should not care about until a real use\ncase shows up.\n\n> + else if (!strcmp(value, \"report-any-error\"))\n\nI couldn't guess based on this name alone, whether we continue or stop\nafter the reporting part. The 7/7 document makes it clear though. So\nall good.\n\n> diff --git a/run-command.c b/run-command.c\n> index 191d6f6f7e..70fb19a55b 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -1308,6 +1308,8 @@ int async_with_fork(void)\n> #endif\n> }\n>\n> +struct string_list hook_error_behavior = STRING_LIST_INIT_NODUP;\n\nMaybe stick this in 'struct repository'. I know most config variables\nare still global. But I think we have to move/reorganize them at some\npoint. Most may end up in 'struct repository'.\n\n> @@ -1401,18 +1403,48 @@ int for_each_hook(const char *name,\n> void *data)\n> {\n> struct string_list paths = STRING_LIST_INIT_DUP;\n> - int i, ret = 0;\n> + int i, hret = 0;\n> + uintptr_t behavior = HOOK_ERROR_STOP_ON_FIRST;\n> + struct string_list_item *item;\n> + /* Use -2 as sentinel because failure to exec is -1. */\n> + int ret = -2;\n> +\n> + item = string_list_lookup(&hook_error_behavior, name);\n> + if (item)\n> + behavior = (uintptr_t)item->util;\n>\n> find_hooks(name, &paths);\n> for (i = 0; i < paths.nr; i++) {\n> const char *p = paths.items[i].string;\n>\n> - ret = handler(name, p, data);\n> - if (ret)\n> - break;\n> + hret = handler(name, p, data);\n> + switch (behavior) {\n> + case HOOK_ERROR_STOP_ON_FIRST:\n> + if (hret) {\n> + ret = hret;\n> + goto out;\n> + }\n> + break;\n> + case HOOK_ERROR_REPORT_ANY_SUCCESS:\n> + if (ret == -2)\n> + ret = 1;\n> + if (!hret)\n> + ret = 0;\n> + break;\n> + case HOOK_ERROR_REPORT_ANY_ERROR:\n> + if (ret == -2)\n> + ret = 0;\n> + if (hret)\n> + ret = hret;\n> + break;\n> + default:\n> + BUG(\"unknown hook error behavior\");\n\nmaybe BUG(\".. behavior %d\", behavior);\n\n> + }\n> }\n> -\n> +out:\n> string_list_clear(&paths, 0);\n> + if (ret == -2)\n> + return 0;\n> return ret;\n> }\n>\n-- \nDuy\n"},{"id":"375532","messageId":"CACsJy8DH5MzqM+8sn7Wi04KxUTxJjnWhEBMk2RVrXUDMwB9sTg@mail.gmail.com","threadId":"51084","inReplyTo":"20190514002332.121089-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 0/7] Multiple hook support","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-14T13:30:55Z","receivedAt":"2019-05-14T13:31:23Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, May 14, 2019 at 7:23 AM brian m. carlson\n wrote:\n>\n> This series introduces multiple hook support.\n>\n> I've thought a lot about the discussion over whether this series should\n> use the configuration as the source for multiple hooks. Ultimately, I've\n> come to the decision that it's not a good idea. Even adopting the empty\n> entry as a reset marker, the fact that inheritance in the configuration\n> is in-order and can't be easily modified means that it's not likely to\n> be very useful, but it is likely to be quite surprising for the average\n> user.\n\nCan we just do like we do with hooks-in-directory? Ignore the config\nvariable order. Sort them all alphabetically. The user then has to\nprefix a number or something to control order. Easier transition from\nhooks-in-dir to hooks-in-config too.\n\n> I think a solution that sticks with the existing model and builds\n> off a design used by other systems people are familiar with, like cron\n> and run-parts, is going to be a better choice. Moreover, this is the\n> design that people have already built with outside tooling, which is a\n> further argument in favor of it.\n-- \nDuy\n"},{"id":"375533","messageId":"CACsJy8B-qn9Vqrm5xF4Eg0j+dfw=WqtnJK5-Jz4kDKvJTNv_tw@mail.gmail.com","threadId":"51084","inReplyTo":"20190514002332.121089-9-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 7/7] docs: document multiple hooks","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-14T13:38:06Z","receivedAt":"2019-05-14T13:38:35Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, May 14, 2019 at 7:24 AM brian m. carlson\n wrote:\n> +It is possible to provide multiple hooks for a single function. If the\n> +main hook file is absent,\n\nIf I remember 1/7 correctly, if the hook \"file\" is a directory, you\nignore it and check for hook.d too.\n\nWhich makes me think, can we just check if hooks/ is a directory\nand use it instead of hooks/.d? [1]\n\nThe only advantage of .d that I can see is if we support some\nsort of combination of and .d.\n\n[1] Of course if you're really strict on backward compatibility then\nthis is out of question.\n\n> hooks are additionally looked for in a\n> +directory with the name of the main hook file with a `.d` appended.\n> +(That is, if `post-receive` is missing, `post-receive.d` is inspected\n> +for any hooks that might be present.) Each of these hooks is executed in order,\n> +sorted by file name. By default, if a hook fails, additional hooks are not\n> +executed, but this can be controlled with the `hook.*.errorBehavior` setting\n> +(see linkgit:git-config[1]).\n> +\n> `git init` may copy hooks to the new repository, depending on its\n> configuration. See the \"TEMPLATE DIRECTORY\" section in\n> linkgit:git-init[1] for details. When the rest of this document refers\n\n\n\n-- \nDuy\n"},{"id":"375543","messageId":"nycvar.QRO.7.76.6.1905141653130.44@tvgsbejvaqbjf.bet","threadId":"51084","inReplyTo":"20190514002332.121089-2-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 1/7] run-command: add preliminary support for multiple hooks","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-05-14T15:12:39Z","receivedAt":"2019-05-14T15:13:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi brian,\n\nOn Tue, 14 May 2019, brian m. carlson wrote:\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 833ecb316a..29bf80e0d1 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -943,7 +943,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> \t\treturn 0;\n> \t}\n>\n> -\tif (!no_verify && find_hook(\"pre-commit\")) {\n> +\tif (!no_verify && find_hooks(\"pre-commit\", NULL)) {\n\nHmm. This makes me concerned, as `find_hook()` essentially boiled down to\na semi-fast `stat()` check, but `find_hooks()` needs to really look,\nright?\n\nIt might make sense to somehow keep the list of discovered and executed\nhooks, as we already have a call to `run_commit_hook()` to execute the\n`pre-commit` hook at the beginning of this function.\n\nSpeaking of which... Shouldn't that `run_commit_hook()` call be adjusted\nat the same time, or else we have an inconsistent situation?\n\n> \t\t/*\n> \t\t * Re-read the index as pre-commit hook could have updated it,\n> \t\t * and write it out as a tree. We must do this before we invoke\n> diff --git a/run-command.c b/run-command.c\n> index 3449db319b..eb075ac86b 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -1308,53 +1308,143 @@ int async_with_fork(void)\n> #endif\n> }\n>\n> +/*\n> + * Return 1 if a hook exists at path (which may be modified) using access(2)\n> + * with check (which should be F_OK or X_OK), 0 otherwise. If strip is true,\n> + * additionally consider the same filename but with STRIP_EXTENSION added.\n> + * If check is X_OK, warn if the hook exists but is not executable.\n> + */\n> +static int has_hook(struct strbuf *path, int strip, int check)\n> +{\n> +\tif (access(path->buf, check) < 0) {\n> +\t\tint err = errno;\n> +\n> +\t\tif (strip) {\n> +#ifdef STRIP_EXTENSION\n> +\t\t\tstrbuf_addstr(path, STRIP_EXTENSION);\n> +\t\t\tif (access(path->buf, check) >= 0)\n> +\t\t\t\treturn 1;\n> +\t\t\tif (errno == EACCES)\n> +\t\t\t\terr = errno;\n> +#endif\n> +\t\t}\n\nHow about simply guarding the entire `if()`? It is a bit unusual to guard\n*only* the inside block ;-)\n\n> +\n> +\t\tif (err == EACCES && advice_ignored_hook) {\n\nDidn't you want to test for `X_OK` here, too? The code comment above the\nfunction seems to suggest that.\n\n> +\t\t\tstatic struct string_list advise_given = STRING_LIST_INIT_DUP;\n> +\n> +\t\t\tif (!string_list_lookup(&advise_given, path->buf)) {\n> +\t\t\t\tstring_list_insert(&advise_given, path->buf);\n> +\t\t\t\tadvise(_(\"The '%s' hook was ignored because \"\n> +\t\t\t\t\t \"it's not set as executable.\\n\"\n> +\t\t\t\t\t \"You can disable this warning with \"\n> +\t\t\t\t\t \"`git config advice.ignoredHook false`.\"),\n> +\t\t\t\t path->buf);\n> +\t\t\t}\n> +\t\t}\n> +\t\treturn 0;\n> +\t}\n> +\treturn 1;\n\nWouldn't it make sense to do this very early? Something like\n\n\tif (!access(path->buf, check))\n\t\treturn 1;\n\nfirst thing in the function, that that part is already out of the way and\nwe don't have to indent so much.\n\n> +}\n> +\n> const char *find_hook(const char *name)\n> {\n> \tstatic struct strbuf path = STRBUF_INIT;\n>\n> \tstrbuf_reset(&path);\n> \tstrbuf_git_path(&path, \"hooks/%s\", name);\n> -\tif (access(path.buf, X_OK) < 0) {\n> -\t\tint err = errno;\n> -\n> -#ifdef STRIP_EXTENSION\n> -\t\tstrbuf_addstr(&path, STRIP_EXTENSION);\n> -\t\tif (access(path.buf, X_OK) >= 0)\n> -\t\t\treturn path.buf;\n> -\t\tif (errno == EACCES)\n> -\t\t\terr = errno;\n> -#endif\n> -\n> -\t\tif (err == EACCES && advice_ignored_hook) {\n> -\t\t\tstatic struct string_list advise_given = STRING_LIST_INIT_DUP;\n> -\n> -\t\t\tif (!string_list_lookup(&advise_given, name)) {\n> -\t\t\t\tstring_list_insert(&advise_given, name);\n> -\t\t\t\tadvise(_(\"The '%s' hook was ignored because \"\n> -\t\t\t\t\t \"it's not set as executable.\\n\"\n> -\t\t\t\t\t \"You can disable this warning with \"\n> -\t\t\t\t\t \"`git config advice.ignoredHook false`.\"),\n> -\t\t\t\t path.buf);\n> -\t\t\t}\n> -\t\t}\n> -\t\treturn NULL;\n\nSo that's where this comes from ;-)\n\n> +\tif (has_hook(&path, 1, X_OK)) {\n> +\t\treturn path.buf;\n> \t}\n> -\treturn path.buf;\n> +\treturn NULL;\n> }\n>\n> -int run_hook_ve(const char *const *env, const char *name, va_list args)\n> +int find_hooks(const char *name, struct string_list *list)\n> {\n> -\tstruct child_process hook = CHILD_PROCESS_INIT;\n> -\tconst char *p;\n> +\tstruct strbuf path = STRBUF_INIT;\n> +\tDIR *d;\n> +\tstruct dirent *de;\n>\n> -\tp = find_hook(name);\n> -\tif (!p)\n> +\t/*\n> +\t * We look for a single hook. If present, return it, and skip the\n> +\t * individual directories.\n> +\t */\n> +\tstrbuf_git_path(&path, \"hooks/%s\", name);\n> +\tif (has_hook(&path, 1, X_OK)) {\n> +\t\tif (list)\n> +\t\t\tstring_list_append(list, path.buf);\n> +\t\treturn 1;\n> +\t}\n> +\n> +\tif (has_hook(&path, 1, F_OK))\n> \t\treturn 0;\n>\n> -\targv_array_push(&hook.args, p);\n> -\twhile ((p = va_arg(args, const char *)))\n> -\t\targv_array_push(&hook.args, p);\n> -\thook.env = env;\n> +\tstrbuf_reset(&path);\n> +\tstrbuf_git_path(&path, \"hooks/%s.d\", name);\n> +\td = opendir(path.buf);\n> +\tif (!d) {\n> +\t\tif (list)\n> +\t\t\tstring_list_clear(list, 0);\n> +\t\treturn 0;\n> +\t}\n> +\twhile ((de = readdir(d))) {\n> +\t\tif (!strcmp(de->d_name, \".\") || !strcmp(de->d_name, \"..\"))\n> +\t\t\tcontinue;\n> +\t\tstrbuf_reset(&path);\n> +\t\tstrbuf_git_path(&path, \"hooks/%s.d/%s\", name, de->d_name);\n> +\t\tif (has_hook(&path, 0, X_OK)) {\n> +\t\t\tif (list)\n> +\t\t\t\tstring_list_append(list, path.buf);\n> +\t\t\telse\n> +\t\t\t\treturn 1;\n> +\t\t}\n> +\t}\n> +\tclosedir(d);\n> +\tstrbuf_reset(&path);\n> +\tif (!list->nr) {\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tstring_list_sort(list);\n> +\treturn 1;\n> +}\n> +\n> +int for_each_hook(const char *name,\n> +\t\t int (*handler)(const char *name, const char *path, void *),\n> +\t\t void *data)\n> +{\n> +\tstruct string_list paths = STRING_LIST_INIT_DUP;\n> +\tint i, ret = 0;\n> +\n> +\tfind_hooks(name, &paths);\n> +\tfor (i = 0; i < paths.nr; i++) {\n> +\t\tconst char *p = paths.items[i].string;\n> +\n> +\t\tret = handler(name, p, data);\n> +\t\tif (ret)\n> +\t\t\tbreak;\n> +\t}\n> +\n> +\tstring_list_clear(&paths, 0);\n> +\treturn ret;\n> +}\n> +\n> +struct hook_data {\n> +\tconst char *const *env;\n> +\tstruct string_list *args;\n> +};\n> +\n> +static int do_run_hook_ve(const char *name, const char *path, void *cb)\n> +{\n> +\tstruct hook_data *data = cb;\n> +\tstruct child_process hook = CHILD_PROCESS_INIT;\n> +\tstruct string_list_item *arg;\n> +\n> +\targv_array_push(&hook.args, path);\n> +\tfor_each_string_list_item(arg, data->args) {\n> +\t\targv_array_push(&hook.args, arg->string);\n> +\t}\n> +\n> +\thook.env = data->env;\n> \thook.no_stdin = 1;\n> \thook.stdout_to_stderr = 1;\n> \thook.trace2_hook_name = name;\n> @@ -1362,6 +1452,21 @@ int run_hook_ve(const char *const *env, const char *name, va_list args)\n> \treturn run_command(&hook);\n> }\n>\n> +int run_hook_ve(const char *const *env, const char *name, va_list args)\n> +{\n> +\tstruct string_list arglist = STRING_LIST_INIT_DUP;\n> +\tstruct hook_data data = {env, &arglist};\n> +\tconst char *p;\n> +\tint ret = 0;\n> +\n> +\twhile ((p = va_arg(args, const char *)))\n> +\t\tstring_list_append(&arglist, p);\n> +\n> +\tret = for_each_hook(name, do_run_hook_ve, &data);\n> +\tstring_list_clear(&arglist, 0);\n> +\treturn ret;\n> +}\n> +\n> int run_hook_le(const char *const *env, const char *name, ...)\n> {\n> \tva_list args;\n> diff --git a/run-command.h b/run-command.h\n> index a6950691c0..1b3677fcac 100644\n> --- a/run-command.h\n> +++ b/run-command.h\n> @@ -4,6 +4,7 @@\n> #include \"thread-utils.h\"\n>\n> #include \"argv-array.h\"\n> +#include \"string-list.h\"\n>\n> struct child_process {\n> \tconst char **argv;\n> @@ -68,6 +69,20 @@ int run_command(struct child_process *);\n> * overwritten by further calls to find_hook and run_hook_*.\n> */\n> extern const char *find_hook(const char *name);\n> +/*\n> + * Returns the paths to all hook files, or NULL if all hooks are missing or\n> + * disabled.\n\nLeft-over comment?\n\n> + * Returns 1 if there are hooks; 0 otherwise. If hooks is not NULL, stores the\n> + * names of the hooks into them in the order they should be executed.\n> + */\n> +int find_hooks(const char *name, struct string_list *hooks);\n> +/*\n> + * Invokes the handler function once for each hook. Returns 0 if all hooks were\n> + * successful, or the exit status of the first failing hook.\n> + */\n> +int for_each_hook(const char *name,\n> +\t\t int (*handler)(const char *name, const char *path, void *),\n> +\t\t void *data);\n\nMy gut says that it would make sense for `struct repository *` to sprout a\nhashmap for hook lookup that would be populated by demand, and allowed\nthings like\n\n\thash_hook(r, \"pre-commit\")\n\nI can't follow up with code, though, as I'm off for the day!\n\nCiao,\nDscho\n\n> LAST_ARG_MUST_BE_NULL\n> extern int run_hook_le(const char *const *env, const char *name, ...);\n> extern int run_hook_ve(const char *const *env, const char *name, va_list args);\n> diff --git a/t/lib-hooks.sh b/t/lib-hooks.sh\n> new file mode 100644\n> index 0000000000..721250aea0\n> --- /dev/null\n> +++ b/t/lib-hooks.sh\n> @@ -0,0 +1,172 @@\n> +create_multihooks () {\n> +\tmkdir -p \"$MULTIHOOK_DIR\"\n> +\tfor i in \"a $1\" \"b $2\" \"c $3\"\n> +\tdo\n> +\t\techo \"$i\" | (while read script ex\n> +\t\tdo\n> +\t\t\tmkdir -p \"$MULTIHOOK_DIR\"\n> +\t\t\twrite_script \"$MULTIHOOK_DIR/$script\" <<-EOF\n> +\t\t\tmkdir -p \"$OUTPUTDIR\"\n> +\t\t\ttouch \"$OUTPUTDIR/$script\"\n> +\t\t\texit $ex\n> +\t\t\tEOF\n> +\t\tdone)\n> +\tdone\n> +}\n> +\n> +# Run the multiple hook tests.\n> +# Usage: test_multiple_hooks [--ignore-exit-status] HOOK COMMAND [SKIP-COMMAND]\n> +# HOOK: the name of the hook to test\n> +# COMMAND: command to test the hook for; takes a single argument indicating test\n> +# name.\n> +# SKIP-COMMAND: like $1, except the hook should be skipped.\n> +# --ignore-exit-status: the command does not fail if the exit status from the\n> +# hook is nonzero.\n> +test_multiple_hooks () {\n> +\tlocal must_fail cmd skip_cmd hook\n> +\tif test \"$1\" = \"--ignore-exit-status\"\n> +\tthen\n> +\t\tshift\n> +\telse\n> +\t\tmust_fail=\"test_must_fail\"\n> +\tfi\n> +\thook=\"$1\"\n> +\tcmd=\"$2\"\n> +\tskip_cmd=\"$3\"\n> +\n> +\tHOOKDIR=\"$(git rev-parse --absolute-git-dir)/hooks\"\n> +\tOUTPUTDIR=\"$(git rev-parse --absolute-git-dir)/../hook-output\"\n> +\tHOOK=\"$HOOKDIR/$hook\"\n> +\tMULTIHOOK_DIR=\"$HOOKDIR/$hook.d\"\n> +\trm -f \"$HOOK\" \"$MULTIHOOK_DIR\" \"$OUTPUTDIR\"\n> +\n> +\ttest_expect_success \"$hook: with no hook\" '\n> +\t\t$cmd foo\n> +\t'\n> +\n> +\tif test -n \"$skip_cmd\"\n> +\tthen\n> +\t\ttest_expect_success \"$hook: skipped hook with no hook\" '\n> +\t\t\t$skip_cmd bar\n> +\t\t'\n> +\tfi\n> +\n> +\ttest_expect_success 'setup' '\n> +\t\tmkdir -p \"$HOOKDIR\" &&\n> +\t\twrite_script \"$HOOK\" <<-EOF\n> +\t\tmkdir -p \"$OUTPUTDIR\"\n> +\t\ttouch \"$OUTPUTDIR/simple\"\n> +\t\texit 0\n> +\t\tEOF\n> +\t'\n> +\n> +\ttest_expect_success \"$hook: with succeeding hook\" '\n> +\t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n> +\t\t$cmd more &&\n> +\t\ttest -f \"$OUTPUTDIR/simple\"\n> +\t'\n> +\n> +\tif test -n \"$skip_cmd\"\n> +\tthen\n> +\t\ttest_expect_success \"$hook: skipped but succeeding hook\" '\n> +\t\t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n> +\t\t\t$skip_cmd even-more &&\n> +\t\t\t! test -f \"$OUTPUTDIR/simple\"\n> +\t\t'\n> +\tfi\n> +\n> +\ttest_expect_success \"$hook: with both simple and multiple hooks, simple success\" '\n> +\t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n> +\t\tcreate_multihooks 0 1 0 &&\n> +\t\t$cmd yet-more &&\n> +\t\ttest -f \"$OUTPUTDIR/simple\" &&\n> +\t\t! test -f \"$OUTPUTDIR/a\" &&\n> +\t\t! test -f \"$OUTPUTDIR/b\" &&\n> +\t\t! test -f \"$OUTPUTDIR/c\"\n> +\t'\n> +\n> +\ttest_expect_success 'setup' '\n> +\t\trm -fr \"$MULTIHOOK_DIR\" &&\n> +\n> +\t\t# now a hook that fails\n> +\t\twrite_script \"$HOOK\" <<-EOF\n> +\t\tmkdir -p \"$OUTPUTDIR\"\n> +\t\ttouch \"$OUTPUTDIR/simple\"\n> +\t\texit 1\n> +\t\tEOF\n> +\t'\n> +\n> +\ttest_expect_success \"$hook: with failing hook\" '\n> +\t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n> +\t\t$must_fail $cmd another &&\n> +\t\ttest -f \"$OUTPUTDIR/simple\"\n> +\t'\n> +\n> +\tif test -n \"$skip_cmd\"\n> +\tthen\n> +\t\ttest_expect_success \"$hook: skipped but failing hook\" '\n> +\t\t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n> +\t\t\t$skip_cmd stuff &&\n> +\t\t\t! test -f \"$OUTPUTDIR/simple\"\n> +\t\t'\n> +\tfi\n> +\n> +\ttest_expect_success \"$hook: with both simple and multiple hooks, simple failure\" '\n> +\t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n> +\t\tcreate_multihooks 0 1 0 &&\n> +\t\t$must_fail $cmd more-stuff &&\n> +\t\ttest -f \"$OUTPUTDIR/simple\" &&\n> +\t\t! test -f \"$OUTPUTDIR/a\" &&\n> +\t\t! test -f \"$OUTPUTDIR/b\" &&\n> +\t\t! test -f \"$OUTPUTDIR/c\"\n> +\t'\n> +\n> +\ttest_expect_success \"$hook: multiple hooks, all successful\" '\n> +\t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n> +\t\trm -f \"$HOOK\" &&\n> +\t\tcreate_multihooks 0 0 0 &&\n> +\t\t$cmd content &&\n> +\t\ttest -f \"$OUTPUTDIR/a\" &&\n> +\t\ttest -f \"$OUTPUTDIR/b\" &&\n> +\t\ttest -f \"$OUTPUTDIR/c\"\n> +\t'\n> +\n> +\ttest_expect_success \"$hook: hooks after first failure not executed\" '\n> +\t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n> +\t\tcreate_multihooks 0 1 0 &&\n> +\t\t$must_fail $cmd more-content &&\n> +\t\ttest -f \"$OUTPUTDIR/a\" &&\n> +\t\ttest -f \"$OUTPUTDIR/b\" &&\n> +\t\t! test -f \"$OUTPUTDIR/c\"\n> +\t'\n> +\n> +\ttest_expect_success POSIXPERM \"$hook: non-executable hook not executed\" '\n> +\t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\"\" &&\n> +\t\tcreate_multihooks 0 1 0 &&\n> +\t\tchmod -x \"$MULTIHOOK_DIR/b\" &&\n> +\t\t$cmd things &&\n> +\t\ttest -f \"$OUTPUTDIR/a\" &&\n> +\t\t! test -f \"$OUTPUTDIR/b\" &&\n> +\t\ttest -f \"$OUTPUTDIR/c\"\n> +\t'\n> +\n> +\ttest_expect_success POSIXPERM \"$hook: multiple hooks not executed if simple hook present\" '\n> +\t\ttest_when_finished \"rm -fr \\\"$OUTPUTDIR\\\" && rm -f \\\"$HOOK\\\"\" &&\n> +\t\twrite_script \"$HOOK\" <<-EOF &&\n> +\t\tmkdir -p \"$OUTPUTDIR\"\n> +\t\ttouch \"$OUTPUTDIR/simple\"\n> +\t\texit 0\n> +\t\tEOF\n> +\t\tcreate_multihooks 0 1 0 &&\n> +\t\tchmod -x \"$HOOK\" &&\n> +\t\t$cmd other-things &&\n> +\t\t! test -f \"$OUTPUTDIR/simple\" &&\n> +\t\t! test -f \"$OUTPUTDIR/a\" &&\n> +\t\t! test -f \"$OUTPUTDIR/b\" &&\n> +\t\t! test -f \"$OUTPUTDIR/c\"\n> +\t'\n> +\n> +\ttest_expect_success 'cleanup' '\n> +\t\trm -fr \"$MULTIHOOK_DIR\"\n> +\t'\n> +}\n> diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh\n> index 984889b39d..d63d059e04 100755\n> --- a/t/t7503-pre-commit-hook.sh\n> +++ b/t/t7503-pre-commit-hook.sh\n> @@ -3,6 +3,7 @@\n> test_description='pre-commit hook'\n>\n> . ./test-lib.sh\n> +. \"$TEST_DIRECTORY/lib-hooks.sh\"\n>\n> test_expect_success 'with no hook' '\n>\n> @@ -136,4 +137,18 @@ test_expect_success 'check the author in hook' '\n> \tgit show -s\n> '\n>\n> +commit_command () {\n> +\techo \"$1\" >>file &&\n> +\tgit add file &&\n> +\tgit commit -m \"$1\"\n> +}\n> +\n> +commit_no_verify_command () {\n> +\techo \"$1\" >>file &&\n> +\tgit add file &&\n> +\tgit commit --no-verify -m \"$1\"\n> +}\n> +\n> +test_multiple_hooks pre-commit commit_command commit_no_verify_command\n> +\n> test_done\n>\n"},{"id":"375546","messageId":"68ebbf52-3925-901d-b4c1-c6144c7f60b7@kdbg.org","threadId":"51084","inReplyTo":"CACsJy8CnCoMK7ahjRotKfiC2ic-U0hdhKuQAeF-TtVt8N+bU2Q@mail.gmail.com","subject":"Re: [PATCH v2 3/7] rebase: add support for multiple hooks","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-05-14T17:58:54Z","receivedAt":"2019-05-14T17:59:01Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 14.05.19 um 14:56 schrieb Duy Nguyen:\n> On Tue, May 14, 2019 at 7:24 AM brian m. carlson\n> wrote:\n>> diff --git a/builtin/am.c b/builtin/am.c\n>> index 912d9821b1..340eacbd44 100644\n>> --- a/builtin/am.c\n>> +++ b/builtin/am.c\n>> @@ -441,24 +441,8 @@ static int run_applypatch_msg_hook(struct am_state *state)\n>> */\n>> static int run_post_rewrite_hook(const struct am_state *state)\n>> {\n>> - struct child_process cp = CHILD_PROCESS_INIT;\n>> - const char *hook = find_hook(\"post-rewrite\");\n>> - int ret;\n>> -\n>> - if (!hook)\n>> - return 0;\n>> -\n>> - argv_array_push(&cp.args, hook);\n>> - argv_array_push(&cp.args, \"rebase\");\n>> -\n>> - cp.in = xopen(am_path(state, \"rewritten\"), O_RDONLY);\n>> - cp.stdout_to_stderr = 1;\n>> - cp.trace2_hook_name = \"post-rewrite\";\n>> -\n>> - ret = run_command(&cp);\n>> -\n>> - close(cp.in);\n> \n> In the old code, we close cp.in...\n> \n>> +int post_rewrite_rebase_hook(const char *name, const char *path, void *input)\n>> +{\n>> + struct child_process child = CHILD_PROCESS_INIT;\n>> +\n>> + child.in = open(input, O_RDONLY);\n>> + child.stdout_to_stderr = 1;\n>> + child.trace2_hook_name = \"post-rewrite\";\n> \n>> + argv_array_push(&child.args, path);\n>> + argv_array_push(&child.args, \"rebase\");\n>> + return run_command(&child);\n> \n> ... but in the new one we don't. Smells fd leaking to me.\n\nIIRC, run_command always closes the fds that it receives (even on error\npaths). Therefore, the old code is incorrect and should not call\nclose(cp.in).\n\n-- Hannes\n"},{"id":"375659","messageId":"20190515222756.GJ7458@genre.crustytoothpaste.net","threadId":"51084","inReplyTo":"CACsJy8Ba98baQ2wZnMZyEva6gxO1ROZ4qJFTOdrCUXDMwrHnXA@mail.gmail.com","subject":"Re: [PATCH v2 1/7] run-command: add preliminary support for multiple hooks","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-05-15T22:27:56Z","receivedAt":"2019-05-15T22:28:08Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, May 14, 2019 at 07:46:17PM +0700, Duy Nguyen wrote:\n> On Tue, May 14, 2019 at 7:24 AM brian m. carlson\n> wrote:\n> > - argv_array_push(&hook.args, p);\n> > - while ((p = va_arg(args, const char *)))\n> > - argv_array_push(&hook.args, p);\n> > - hook.env = env;\n> > + strbuf_reset(&path);\n> > + strbuf_git_path(&path, \"hooks/%s.d\", name);\n> > + d = opendir(path.buf);\n> > + if (!d) {\n> > + if (list)\n> > + string_list_clear(list, 0);\n> > + return 0;\n> > + }\n> > + while ((de = readdir(d))) {\n> > + if (!strcmp(de->d_name, \".\") || !strcmp(de->d_name, \"..\"))\n> > + continue;\n> > + strbuf_reset(&path);\n> > + strbuf_git_path(&path, \"hooks/%s.d/%s\", name, de->d_name);\n> > + if (has_hook(&path, 0, X_OK)) {\n> \n> Do we want to support hooks in subdirectories as well (if so, using\n> dir-iterator.h might be more appropriate)\n\nNo, I don't think so. That's not what most other software does, and I\ndon't really think recursive behavior provides a lot of value.\n\n> If not, what happens when \"path\" is a directory. X_OK could be set\n> (and often are) on them too.\n\nWe get an exec failure when trying to run it, just like if you create a\ndirectory in place of the existing hook now. I think that's a fine\nbehavior.\n\n> > + if (list)\n> > + string_list_append(list, path.buf);\n> > + else\n> > + return 1;\n> > + }\n> > + }\n> > + closedir(d);\n> > + strbuf_reset(&path);\n> > + if (!list->nr) {\n> > + return 0;\n> > + }\n> > +\n> > + string_list_sort(list);\n> \n> This is going to be interesting on case-insensitive filesystems\n> because we do strcmp by default, not the friendlier fspathcmp. And the\n> \".exe\" suffix might affect sort order too.\n> \n> But I suppose we just need to be clear here (in documentation). They\n> can always prefix with a number to keep hook files in expected order.\n\nNumbering is definitely the way I'd go on a case-insensitive file\nsystem. Also, I don't think the \".exe\" will be a problem in practice,\nsince generally you'd have to have a mix of binary and non-binary hooks,\nwhich is unlikely to be super common.\n\n> > diff --git a/run-command.h b/run-command.h\n> > index a6950691c0..1b3677fcac 100644\n> > --- a/run-command.h\n> > +++ b/run-command.h\n> > @@ -4,6 +4,7 @@\n> > #include \"thread-utils.h\"\n> >\n> > #include \"argv-array.h\"\n> > +#include \"string-list.h\"\n> \n> 'struct string_list;' should be enough (and a bit lighter) although I\n> don't suppose it really matters.\n\nI can make that change.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"375660","messageId":"20190515224425.GK7458@genre.crustytoothpaste.net","threadId":"51084","inReplyTo":"nycvar.QRO.7.76.6.1905141653130.44@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 1/7] run-command: add preliminary support for multiple hooks","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-05-15T22:44:25Z","receivedAt":"2019-05-15T22:45:00Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, May 14, 2019 at 05:12:39PM +0200, Johannes Schindelin wrote:\n> Hi brian,\n> \n> On Tue, 14 May 2019, brian m. carlson wrote:\n> \n> > diff --git a/builtin/commit.c b/builtin/commit.c\n> > index 833ecb316a..29bf80e0d1 100644\n> > --- a/builtin/commit.c\n> > +++ b/builtin/commit.c\n> > @@ -943,7 +943,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> > \t\treturn 0;\n> > \t}\n> >\n> > -\tif (!no_verify && find_hook(\"pre-commit\")) {\n> > +\tif (!no_verify && find_hooks(\"pre-commit\", NULL)) {\n> \n> Hmm. This makes me concerned, as `find_hook()` essentially boiled down to\n> a semi-fast `stat()` check, but `find_hooks()` needs to really look,\n> right?\n> \n> It might make sense to somehow keep the list of discovered and executed\n> hooks, as we already have a call to `run_commit_hook()` to execute the\n> `pre-commit` hook at the beginning of this function.\n\nWith NULL as an argument, we return 1 as soon as we know there's a\nsingle hook, so this is fairly optimized. I've tried to make it as cheap\nas possible to check.\n\n> Speaking of which... Shouldn't that `run_commit_hook()` call be adjusted\n> at the same time, or else we have an inconsistent situation?\n\nNope, it calls run_hook_ve, which is updated.\n\n> > diff --git a/run-command.c b/run-command.c\n> > index 3449db319b..eb075ac86b 100644\n> > --- a/run-command.c\n> > +++ b/run-command.c\n> > @@ -1308,53 +1308,143 @@ int async_with_fork(void)\n> > #endif\n> > }\n> >\n> > +/*\n> > + * Return 1 if a hook exists at path (which may be modified) using access(2)\n> > + * with check (which should be F_OK or X_OK), 0 otherwise. If strip is true,\n> > + * additionally consider the same filename but with STRIP_EXTENSION added.\n> > + * If check is X_OK, warn if the hook exists but is not executable.\n> > + */\n> > +static int has_hook(struct strbuf *path, int strip, int check)\n> > +{\n> > +\tif (access(path->buf, check) < 0) {\n> > +\t\tint err = errno;\n> > +\n> > +\t\tif (strip) {\n> > +#ifdef STRIP_EXTENSION\n> > +\t\t\tstrbuf_addstr(path, STRIP_EXTENSION);\n> > +\t\t\tif (access(path->buf, check) >= 0)\n> > +\t\t\t\treturn 1;\n> > +\t\t\tif (errno == EACCES)\n> > +\t\t\t\terr = errno;\n> > +#endif\n> > +\t\t}\n> \n> How about simply guarding the entire `if()`? It is a bit unusual to guard\n> *only* the inside block ;-)\n\nI can make that change.\n\n> > +\n> > +\t\tif (err == EACCES && advice_ignored_hook) {\n> \n> Didn't you want to test for `X_OK` here, too? The code comment above the\n> function seems to suggest that.\n\nYeah, that makes sense. I'll do that.\n\n> > +\t\t\tstatic struct string_list advise_given = STRING_LIST_INIT_DUP;\n> > +\n> > +\t\t\tif (!string_list_lookup(&advise_given, path->buf)) {\n> > +\t\t\t\tstring_list_insert(&advise_given, path->buf);\n> > +\t\t\t\tadvise(_(\"The '%s' hook was ignored because \"\n> > +\t\t\t\t\t \"it's not set as executable.\\n\"\n> > +\t\t\t\t\t \"You can disable this warning with \"\n> > +\t\t\t\t\t \"`git config advice.ignoredHook false`.\"),\n> > +\t\t\t\t path->buf);\n> > +\t\t\t}\n> > +\t\t}\n> > +\t\treturn 0;\n> > +\t}\n> > +\treturn 1;\n> \n> Wouldn't it make sense to do this very early? Something like\n> \n> \tif (!access(path->buf, check))\n> \t\treturn 1;\n> \n> first thing in the function, that that part is already out of the way and\n> we don't have to indent so much.\n\nSure. That's a nice improvement.\n\n> > const char *find_hook(const char *name)\n> > {\n> > \tstatic struct strbuf path = STRBUF_INIT;\n> >\n> > \tstrbuf_reset(&path);\n> > \tstrbuf_git_path(&path, \"hooks/%s\", name);\n> > -\tif (access(path.buf, X_OK) < 0) {\n> > -\t\tint err = errno;\n> > -\n> > -#ifdef STRIP_EXTENSION\n> > -\t\tstrbuf_addstr(&path, STRIP_EXTENSION);\n> > -\t\tif (access(path.buf, X_OK) >= 0)\n> > -\t\t\treturn path.buf;\n> > -\t\tif (errno == EACCES)\n> > -\t\t\terr = errno;\n> > -#endif\n> > -\n> > -\t\tif (err == EACCES && advice_ignored_hook) {\n> > -\t\t\tstatic struct string_list advise_given = STRING_LIST_INIT_DUP;\n> > -\n> > -\t\t\tif (!string_list_lookup(&advise_given, name)) {\n> > -\t\t\t\tstring_list_insert(&advise_given, name);\n> > -\t\t\t\tadvise(_(\"The '%s' hook was ignored because \"\n> > -\t\t\t\t\t \"it's not set as executable.\\n\"\n> > -\t\t\t\t\t \"You can disable this warning with \"\n> > -\t\t\t\t\t \"`git config advice.ignoredHook false`.\"),\n> > -\t\t\t\t path.buf);\n> > -\t\t\t}\n> > -\t\t}\n> > -\t\treturn NULL;\n> \n> So that's where this comes from ;-)\n\nExactly. I didn't make a lot of changes.\n\n> > +/*\n> > + * Returns the paths to all hook files, or NULL if all hooks are missing or\n> > + * disabled.\n> \n> Left-over comment?\n\nYup, thanks for pointing it out.\n\n> > + * Returns 1 if there are hooks; 0 otherwise. If hooks is not NULL, stores the\n> > + * names of the hooks into them in the order they should be executed.\n> > + */\n> > +int find_hooks(const char *name, struct string_list *hooks);\n> > +/*\n> > + * Invokes the handler function once for each hook. Returns 0 if all hooks were\n> > + * successful, or the exit status of the first failing hook.\n> > + */\n> > +int for_each_hook(const char *name,\n> > +\t\t int (*handler)(const char *name, const char *path, void *),\n> > +\t\t void *data);\n> \n> My gut says that it would make sense for `struct repository *` to sprout a\n> hashmap for hook lookup that would be populated by demand, and allowed\n> things like\n> \n> \thash_hook(r, \"pre-commit\")\n\nKnowing that we have an optimized check, do you still think we should do\nthis, or are you okay leaving it as it is?\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"375664","messageId":"20190515225537.GL7458@genre.crustytoothpaste.net","threadId":"51084","inReplyTo":"CACsJy8CnCoMK7ahjRotKfiC2ic-U0hdhKuQAeF-TtVt8N+bU2Q@mail.gmail.com","subject":"Re: [PATCH v2 3/7] rebase: add support for multiple hooks","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-05-15T22:55:37Z","receivedAt":"2019-05-15T22:55:46Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, May 14, 2019 at 07:56:49PM +0700, Duy Nguyen wrote:\n> On Tue, May 14, 2019 at 7:24 AM brian m. carlson\n> wrote:\n> > - close(cp.in);\n> \n> In the old code, we close cp.in...\n> \n> > +int post_rewrite_rebase_hook(const char *name, const char *path, void *input)\n> > +{\n> > + struct child_process child = CHILD_PROCESS_INIT;\n> > +\n> > + child.in = open(input, O_RDONLY);\n> > + child.stdout_to_stderr = 1;\n> > + child.trace2_hook_name = \"post-rewrite\";\n> \n> maybe use \"name\" and avoid hard coding \"post-rewrite\".\n> \n> > + argv_array_push(&child.args, path);\n> > + argv_array_push(&child.args, \"rebase\");\n> > + return run_command(&child);\n> \n> ... but in the new one we don't. Smells fd leaking to me.\n\nAh, good point. Will fix.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"375665","messageId":"20190515231017.GM7458@genre.crustytoothpaste.net","threadId":"51084","inReplyTo":"CACsJy8BN32Jm62MQPLsRwKST8DDNZ7NxnJbsqBO3PJ0K2qU-Ng@mail.gmail.com","subject":"Re: [PATCH v2 6/7] config: allow configuration of multiple hook error behavior","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-05-15T23:10:17Z","receivedAt":"2019-05-15T23:10:25Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, May 14, 2019 at 08:20:10PM +0700, Duy Nguyen wrote:\n> On Tue, May 14, 2019 at 7:24 AM brian m. carlson\n> wrote:\n> >\n> > There are a variety of situations in which a user may want an error\n> > behavior for multiple hooks other than the default. Add a config option,\n> > hook..errorBehavior to allow users to customize this behavior on a\n> \n> An alternative name is onError, probably more often used for event\n> callbacks. But I don't know, maybe errorBehavior is actually better.\n\nI'm going to use \"errorStrategy\", since we already have\nsubmodule.alternateErrorStrategy.\n\n> > per-hook basis. Provide options for the default behavior (exiting\n> \n> should we fall back to hook.errorBehavior? That allows people to set\n> global policy, then customize just a small set of weird hooks.\n\nSure, that sounds good.\n\n> maybe stop-on-first-error (or if you go with the \"onError\" name, I\n> think \"stop\" is enough). I know \"stop on/after first hook\" does not\n> really make any sense when you think about it. Maybe stop-on-first is\n> sufficient.\n> \n> I was going to suggest strcasecmp. But core.whitespace (also has\n> multiple-word-values) already sets a precedent on strcmp. I think\n> we're good. Or mostly good, I don't know, we still accept False, false\n> and FALSE.\n\nI think with errorStrategy, \"stop\" is fine. Simpler is better.\n\nI literally picked what Peff had suggested in his email (mostly because\nI'm terrible at naming things), and I don't get the impression he spent\na great deal of time analyzing the ins and outs of the names before\nsending. I could be wrong, though.\n\n> > + behavior = HOOK_ERROR_STOP_ON_FIRST;\n> \n> This is basically the logical \"and\" behavior in a C expression. Which\n> makes me think if anybody's crazy enough to need the \"or\" counterpart\n> (i.e. run hooks, expect failure, keep going until the first success).\n> \n> I guess it's a crazy mode. We should not care about until a real use\n> case shows up.\n\nYeah, I think that's unlikely to be the case. Nothing prevents us from\nadding it later.\n\n> > + else if (!strcmp(value, \"report-any-error\"))\n> \n> I couldn't guess based on this name alone, whether we continue or stop\n> after the reporting part. The 7/7 document makes it clear though. So\n> all good.\n\nI'm open to hearing better suggestions if anyone has any.\n\n> > diff --git a/run-command.c b/run-command.c\n> > index 191d6f6f7e..70fb19a55b 100644\n> > --- a/run-command.c\n> > +++ b/run-command.c\n> > @@ -1308,6 +1308,8 @@ int async_with_fork(void)\n> > #endif\n> > }\n> >\n> > +struct string_list hook_error_behavior = STRING_LIST_INIT_NODUP;\n> \n> Maybe stick this in 'struct repository'. I know most config variables\n> are still global. But I think we have to move/reorganize them at some\n> point. Most may end up in 'struct repository'.\n\nOkay, sounds fine.\n\n> > + default:\n> > + BUG(\"unknown hook error behavior\");\n> \n> maybe BUG(\".. behavior %d\", behavior);\n\nSure.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"375670","messageId":"20190516004233.GO7458@genre.crustytoothpaste.net","threadId":"51084","inReplyTo":"20190514022653.GA34155@google.com","subject":"Re: [PATCH v2 0/7] Multiple hook support","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-05-16T00:42:33Z","receivedAt":"2019-05-16T01:49:49Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Mon, May 13, 2019 at 07:26:53PM -0700, Jonathan Nieder wrote:\n> brian m. carlson wrote:\n> In other words, use the standard config convention for the set of\n> hooks, and treat the order in which they are invoked as a separate\n> question. You could even use the hooks.d style alphabetical order\n> convention.\n\nIf we're going to have named items in PATH, we do not want to have those\nitems numbered.\n\n> [...]\n> > The second issue here is that it's surprising. Users don't know how to\n> > reset a configuration option because we don't have a consistent way to\n> > do that.\n> \n> I agree that it's underdocumented and underimplemented. But I'm not\n> aware of any other method that Git provides to reset a configuration\n> item. What is it inconsistent with?\n\nSome options support it, and some don't.\n\n> > Users will not expect for there to be multiple ways to set\n> > hooks. Users will also not expect that their hooks in their\n> > configuration aren't run if there are hooks in .git/hooks. Tooling that\n> > has so far used .git/hooks will compete with users' global configuration\n> > options, which I guarantee you will be a surprise for users using older\n> > versions of tools.\n> \n> Indeed, in the long term I think we should remove the .git/hooks/\n> mechanism entirely.\n\nSee, I don't. I like the current mechanism and don't want to get rid of\nit.\n\n> In the shorter term, I think the kind of inconsistency you're referring\n> to applies to hooks.d as well.\n\nI think it's a lot easier to reason about adding a directory to the\nexisting mechanism than adopting a dramatically new one. Users can\neasily understand that if there's a file, that the directory is ignored.\n\nAlso, this is the way that most other programs on Unix do this behavior,\nand I think that is a compelling argument for this design in and of\nitself. I think Unix has generally made the best decisions in operating\nsystem design, and I aim to emulate it as much as possible.\n\n> > It also provides a convenient place for hooks to live, which a\n> > config-based option doesn't. We'll need to invoke things using /bin/sh,\n> > so will they all have to live in PATH? What about one-offs that don't\n> > really belong in PATH?\n> \n> This hasn't been a problem for remote helpers, merge drivers, etc in\n> the past. Why are hooks different?\n\nBecause they're often customized or installed via the repository itself.\nSome projects provide pre-commit hooks to run tests or such in a\nrepository-specific way. Some people have one-off scripts that are\nappropriate for a repo but they don't want in tab-completion. Also, they\nmay wrap a system command with helpful information, but that doesn't\nbelong in PATH.\n\nGit LFS, for example, installs hooks that warn the user if the command\nisn't installed, since it's a common misconfiguration and not pushing\nthe LFS objects (via the pre-push hook) along with the Git objects is a\ncommon source of data loss. Uninstalling the tool (or not installing it\nif it's a shared repository) doesn't mean the hook still shouldn't be\nrun.\n\n> > What might be an interesting approach that would address these concerns\n> > is a core.globalHooks[0] option that points to a set (or sets,\n> > depending) of multiple-hook directories. We then enumerate hooks in sort\n> > order, considering both the global and the local directories as one\n> > unit, perhaps with some way of disabling hooks. I'm not planning on\n> > working on this myself, but I wouldn't be opposed to seeing someone else\n> > work on it.\n> \n> This sounds overflexible to me. Because of that, I don't think it\n> would end up as a default, so we wouldn't have a path to improving our\n> security stature.\n> \n> If I implement a config based multiple hooks feature with name based\n> ordering, would that be useful to you?\n\nOne of my primary motivations for writing this series is because it's a\nrequested feature in conjunction with Git LFS. We often get requests for\nfeatures that are properly directed to Git itself, but because the issue\ncomes up in conjunction with the use of Git LFS, folks report the issues\nto us. (Also, people don't like reporting issues to mailing lists.) I\nthink it's a useful feature for others, though, judging by the multiple\ntimes it's come up.\n\nI see the series that I'm proposing as easy to implement and work with\nusing the existing tools and with other tools that use the existing\nhooks. It's a simple change of path. I anticipate there being more work\nto implement a config-based multiple hooks solution in various tools,\nand I expect it will be less intuitive and less discoverable. I can't\nprovide definitive evidence for this, but my experience answering a lot\nof user questions on a daily basis leads me to believe that it's not the\nright way forward.\n\nI'm not opposed to extending the config system to implement multiple\nhooks directories or add support for inheriting hooks, because that's a\ncommon thing that people want. I just don't think our config system is\nthe right tool for specifying what commands to run, for the reasons I've\nspecified.\n\nI can't prevent you from writing a series that implements a config-based\noption, and if it's the solution the list wants, I'll go along with it,\nbut it's not the solution I want to see personally or as a tooling\nimplementer.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"375671","messageId":"20190516005144.GC34155@google.com","threadId":"51084","inReplyTo":"20190516004233.GO7458@genre.crustytoothpaste.net","subject":"Re: [PATCH v2 0/7] Multiple hook support","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-05-16T00:51:44Z","receivedAt":"2019-05-16T01:49:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"brian m. carlson wrote:\n\n> Also, this is the way that most other programs on Unix do this behavior,\n> and I think that is a compelling argument for this design in and of\n> itself. I think Unix has generally made the best decisions in operating\n> system design, and I aim to emulate it as much as possible.\n\nDo a lot of other programs run commands from a specially named\nsubdirectory of the current directory?\n\nIf you were talking about a hooks dir in /etc, I would completely\nagree. But we are talking about .git/hooks/, which has been a\nconstant source of real compromises.\n\nAnyway, I think we've gone back and forth enough times to discover\nwe're not going to agree on this.\n\n[...]\n>> This hasn't been a problem for remote helpers, merge drivers, etc in\n>> the past. Why are hooks different?\n[...]\n> Git LFS, for example, installs hooks that warn the user if the command\n> isn't installed, since it's a common misconfiguration and not pushing\n> the LFS objects (via the pre-push hook) along with the Git objects is a\n> common source of data loss. Uninstalling the tool (or not installing it\n> if it's a shared repository) doesn't mean the hook still shouldn't be\n> run.\n\nI don't understand this example. If the repository is configured to\nuse the Git LFS hooks, why wouldn't it print a friendly message?\n\n[...]\n> I'm not opposed to extending the config system to implement multiple\n> hooks directories or add support for inheriting hooks, because that's a\n> common thing that people want. I just don't think our config system is\n> the right tool for specifying what commands to run, for the reasons I've\n> specified.\n>\n> I can't prevent you from writing a series that implements a config-based\n> option, and if it's the solution the list wants, I'll go along with it,\n> but it's not the solution I want to see personally or as a tooling\n> implementer.\n\nI think you're answering a different question than I asked.\n\nOne thing I've been talking about is having a path to eventually\ngetting rid of support for .git/hooks/, on a user controlled timeline,\nwith a smooth migration. I proposed one possible way to do that, and\nI was asking whether it would work okay for your use case, or whether\nthere are problems with it (which would give me something to work with\non iterating toward something that would work for you).\n\nYour answer is \"I can't prevent you\", which means you don't like the\nproposal, but doesn't tell me what about it would not work.\n\nThanks,\nJonathan\n"},{"id":"375699","messageId":"20190516045140.GA7241@sigill.intra.peff.net","threadId":"51084","inReplyTo":"20190514015928.GG7458@genre.crustytoothpaste.net","subject":"Re: [PATCH v2 0/7] Multiple hook support","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-16T04:51:40Z","receivedAt":"2019-05-16T04:51:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 14, 2019 at 01:59:28AM +0000, brian m. carlson wrote:\n\n> There are two aspects here which I think are worth discussing. Let's\n> discuss the inheritance issue first.\n> \n> Order with multiple hooks matters. The best hook as an example for this\n> is prepare-commit-msg. If I have a hook which I want to run on every\n> repository, such as a hook that inserts some sort of ID (bug tracker,\n> Gerrit, etc.), that hook, due to inheritance, *has* to be first, before\n> any other prepare-commit-msg hooks. If I want a hook that runs before\n> it, perhaps because a particular repository needs a different\n> configuration, I have to wipe the list and insert both hooks. I'm now\n> maintaining two separate locations for the command lines instead of just\n> inserting a symlink to the global hook and dropping a new hook before\n> it.\n\nThis part confuses me. In the config based scheme you describe, you have\nto mention the original hook again. But isn't that exactly what creating\na symlink in your hooks directory is doing?\n\nAnd in fact, I think a config based scheme can be a lot more flexible in\nthe end, simply because the parser _does_ see all of the hooks (whereas\nin the hook directory scheme, we only ever look in one directory). So we\ncan have an option to sort them before running: alphabetically,\nsystem-to-repo order, repo-to-system order, etc.\n\n> The second issue here is that it's surprising. Users don't know how to\n> reset a configuration option because we don't have a consistent way to\n> do that. Users will not expect for there to be multiple ways to set\n> hooks. Users will also not expect that their hooks in their\n> configuration aren't run if there are hooks in .git/hooks. Tooling that\n> has so far used .git/hooks will compete with users' global configuration\n> options, which I guarantee you will be a surprise for users using older\n> versions of tools.\n\nI don't agree here. If the rule is \"config takes precedence\", and \"if\nconfig is absent \"behave as if .git/hooks/whatever was in the config\",\nthen the transition is easy to explain. And nobody sees any change at\nall until they decide to set the config. It would probably also be\nprudent to issue a warning if there's config _and_ an on-disk hook file\n(or even run them both, though then you open up the question of\nordering).\n\nWhich isn't to say it's impossible for a person to get confused (new\ntool sets the config, disabling their old hook?). But I think any\ntransition to multi-hook support (including your directory scheme) is\ngoing to have some possibility that tools automatically setting things\nup behind the scenes is going to confuse a user.\n\n> It also provides a convenient place for hooks to live, which a\n> config-based option doesn't. We'll need to invoke things using /bin/sh,\n> so will they all have to live in PATH? What about one-offs that don't\n> really belong in PATH?\n\nActually, my biggest beef with the current hooks mechanism is that it's\nan _inconvenient_ place for them to live.\n\n - it's not version controlled, and putting a repository inside another\n repository is awkward (though it at least works for the bare case)\n\n - touching the filesystem is awkward and expensive if you have a large\n number of repositories whose hooks you want to update\n\n - it requires a manual step to modify the filesystem after a fresh\n clone (as opposed to putting things in ~/.gitconfig). Technically\n one can solve this by modifying .../share/git/templates, but that\n has its own issues.\n\nIn my world-view config-based hooks would just be run with SHELL_PATH,\njust like our other config options. If you don't want one-offs in your\nPATH, then use absolute paths (just like you would have to for\nsymlinks). If you really don't know where to put one-off hooks, perhaps\nwe could document a base directory from which the hook script is run,\nand you could put it in a directory in .git and use a relative path? You\ncould even call that directory \"hooks\", but I suspect that would be too\nconfusing. :)\n\n-Peff\n"},{"id":"375700","messageId":"20190516050200.GB7241@sigill.intra.peff.net","threadId":"51084","inReplyTo":"20190514002332.121089-8-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 6/7] config: allow configuration of multiple hook error behavior","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-16T05:02:00Z","receivedAt":"2019-05-16T05:02:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 14, 2019 at 12:23:31AM +0000, brian m. carlson wrote:\n\n> There are a variety of situations in which a user may want an error\n> behavior for multiple hooks other than the default. Add a config option,\n> hook..errorBehavior to allow users to customize this behavior on a\n> per-hook basis. Provide options for the default behavior (exiting\n> early), executing all hooks and succeeding if all hooks succeed, or\n> executing all hooks and succeeding if any hook succeeds.\n\nThanks for using that naming scheme. I think if we do move to allowing\nconfig-based hooks, the config schemes will mesh together well.\n\n> +static int git_default_hook_config(const char *key, const char *value)\n> +{\n> +\tconst char *hook;\n> +\tsize_t key_len;\n> +\tuintptr_t behavior;\n> +\n> +\tkey += strlen(\"hook.\");\n> +\tif (strip_suffix(key, \".errorbehavior\", &key_len)) {\n\nThere's an undocumented assumption that the caller has confirmed that\nthe key starts with \"hook.\" here. Can we be a little more defensive and\ndo:\n\n if (skip_prefix(key, \"hook.\", &key))\n\treturn 0;\n\nhere (we could even drop the check in git_default_config).\n\nOr we could use parse_key(), which is designed for this:\n\n if (parse_key(key, \"hook\", &subsection, &subsection_len, &key) < 0 ||\n !subsection)\n\treturn 0;\n if (!strcmp(key, \"errorbehavior\"))\n\t...\n\n> +\t/* Use -2 as sentinel because failure to exec is -1. */\n> +\tint ret = -2;\n\nMaybe this would be simpler to follow by using an enum for the handler\nreturn value?\n\nAside from these nits, the code looked sensible to me.\n\n-Peff\n"},{"id":"375701","messageId":"20190516050835.GA7723@sigill.intra.peff.net","threadId":"51084","inReplyTo":"20190515231017.GM7458@genre.crustytoothpaste.net","subject":"Re: [PATCH v2 6/7] config: allow configuration of multiple hook error behavior","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-16T05:08:35Z","receivedAt":"2019-05-16T05:08:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 15, 2019 at 11:10:17PM +0000, brian m. carlson wrote:\n\n> > An alternative name is onError, probably more often used for event\n> > callbacks. But I don't know, maybe errorBehavior is actually better.\n> \n> I'm going to use \"errorStrategy\", since we already have\n> submodule.alternateErrorStrategy.\n\nThat sounds good (and I don't care too much about the name as long as it\nit is in the per-hook subsection like this).\n\n> > should we fall back to hook.errorBehavior? That allows people to set\n> > global policy, then customize just a small set of weird hooks.\n> \n> Sure, that sounds good.\n\nI like this, too.\n\n> > maybe stop-on-first-error (or if you go with the \"onError\" name, I\n> > think \"stop\" is enough). I know \"stop on/after first hook\" does not\n> > really make any sense when you think about it. Maybe stop-on-first is\n> > sufficient.\n> > \n> > I was going to suggest strcasecmp. But core.whitespace (also has\n> > multiple-word-values) already sets a precedent on strcmp. I think\n> > we're good. Or mostly good, I don't know, we still accept False, false\n> > and FALSE.\n> \n> I think with errorStrategy, \"stop\" is fine. Simpler is better.\n> \n> I literally picked what Peff had suggested in his email (mostly because\n> I'm terrible at naming things), and I don't get the impression he spent\n> a great deal of time analyzing the ins and outs of the names before\n> sending. I could be wrong, though.\n\nNo, I didn't. :) I think \"stop\" is good. If the others are\nreport-any-error and report-any-success, then the matching name for this\ncould be report-first-error.\n\n> > > + else if (!strcmp(value, \"report-any-error\"))\n> > \n> > I couldn't guess based on this name alone, whether we continue or stop\n> > after the reporting part. The 7/7 document makes it clear though. So\n> > all good.\n> \n> I'm open to hearing better suggestions if anyone has any.\n\nMaybe report-all-errors would indicate that it was going to run all of\nthe hooks. I dunno. I think the documentation you wrote is plenty clear\nwith the current name.\n\n-Peff\n"},{"id":"375713","messageId":"CACsJy8B-PY9eBqGBL4CSVGS184DrgZSQqTfwY9gVctX_WfdDhA@mail.gmail.com","threadId":"51084","inReplyTo":"20190515225537.GL7458@genre.crustytoothpaste.net","subject":"Re: [PATCH v2 3/7] rebase: add support for multiple hooks","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-16T10:29:40Z","receivedAt":"2019-05-16T10:30:09Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, May 16, 2019 at 5:55 AM brian m. carlson\n wrote:\n>\n> On Tue, May 14, 2019 at 07:56:49PM +0700, Duy Nguyen wrote:\n> > On Tue, May 14, 2019 at 7:24 AM brian m. carlson\n> > wrote:\n> > > - close(cp.in);\n> >\n> > In the old code, we close cp.in...\n> >\n> > > +int post_rewrite_rebase_hook(const char *name, const char *path, void *input)\n> > > +{\n> > > + struct child_process child = CHILD_PROCESS_INIT;\n> > > +\n> > > + child.in = open(input, O_RDONLY);\n> > > + child.stdout_to_stderr = 1;\n> > > + child.trace2_hook_name = \"post-rewrite\";\n> >\n> > maybe use \"name\" and avoid hard coding \"post-rewrite\".\n> >\n> > > + argv_array_push(&child.args, path);\n> > > + argv_array_push(&child.args, \"rebase\");\n> > > + return run_command(&child);\n> >\n> > ... but in the new one we don't. Smells fd leaking to me.\n>\n> Ah, good point. Will fix.\n\nActually I think Johannes is right (in the other mail, same thread)\nthat the old code is buggy.\n\nThe only good thing is, I don't think the old code could actually\nresult in double close() problem. It would just return EBADF, which we\ndon't care. So no hurry fixing the old code until you replace it with\na better one.\n\n-- \nDuy\n"},{"id":"375728","messageId":"20190516171953.GA265608@genre.crustytoothpaste.net","threadId":"51084","inReplyTo":"20190516050200.GB7241@sigill.intra.peff.net","subject":"Re: [PATCH v2 6/7] config: allow configuration of multiple hook error behavior","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-05-16T17:19:53Z","receivedAt":"2019-05-16T17:20:03Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2019-05-16 at 05:02:00, Jeff King wrote:\n> > +static int git_default_hook_config(const char *key, const char *value)\n> > +{\n> > +\tconst char *hook;\n> > +\tsize_t key_len;\n> > +\tuintptr_t behavior;\n> > +\n> > +\tkey += strlen(\"hook.\");\n> > +\tif (strip_suffix(key, \".errorbehavior\", &key_len)) {\n> \n> There's an undocumented assumption that the caller has confirmed that\n> the key starts with \"hook.\" here. Can we be a little more defensive and\n> do:\n> \n> if (skip_prefix(key, \"hook.\", &key))\n> \treturn 0;\n\nYeah, the caller checks that, but I think being a little more defensive\nis fine.\n\n> here (we could even drop the check in git_default_config).\n> \n> Or we could use parse_key(), which is designed for this:\n> \n> if (parse_key(key, \"hook\", &subsection, &subsection_len, &key) < 0 ||\n> !subsection)\n> \treturn 0;\n\nOh, good, I didn't know we had that. That's exactly what I want.\n\n> if (!strcmp(key, \"errorbehavior\"))\n> \t...\n> \n> > +\t/* Use -2 as sentinel because failure to exec is -1. */\n> > +\tint ret = -2;\n> \n> Maybe this would be simpler to follow by using an enum for the handler\n> return value?\n\nWe can't make this variable an enum because we'd have to define 256\nentries (well, we can, but it would be a hassle), but I can create an\nenum and assign it to the int variable, sure.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"375737","messageId":"91532c2d-7536-6e15-728b-bd18256b2ef3@kdbg.org","threadId":"51084","inReplyTo":"20190515224425.GK7458@genre.crustytoothpaste.net","subject":"Re: [PATCH v2 1/7] run-command: add preliminary support for multiple hooks","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-05-16T19:11:10Z","receivedAt":"2019-05-16T19:11:16Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 16.05.19 um 00:44 schrieb brian m. carlson:\n> On Tue, May 14, 2019 at 05:12:39PM +0200, Johannes Schindelin wrote:\n>> On Tue, 14 May 2019, brian m. carlson wrote:\n>>> +/*\n>>> + * Return 1 if a hook exists at path (which may be modified) using access(2)\n>>> + * with check (which should be F_OK or X_OK), 0 otherwise. If strip is true,\n>>> + * additionally consider the same filename but with STRIP_EXTENSION added.\n>>> + * If check is X_OK, warn if the hook exists but is not executable.\n>>> + */\n>>> +static int has_hook(struct strbuf *path, int strip, int check)\n>>> +{\n>>> +\tif (access(path->buf, check) < 0) {\n>>> +\t\tint err = errno;\n>>> +\n>>> +\t\tif (strip) {\n>>> +#ifdef STRIP_EXTENSION\n>>> +\t\t\tstrbuf_addstr(path, STRIP_EXTENSION);\n>>> +\t\t\tif (access(path->buf, check) >= 0)\n>>> +\t\t\t\treturn 1;\n>>> +\t\t\tif (errno == EACCES)\n>>> +\t\t\t\terr = errno;\n>>> +#endif\n>>> +\t\t}\n>>\n>> How about simply guarding the entire `if()`? It is a bit unusual to guard\n>> *only* the inside block ;-)\n> \n> I can make that change.\n\nBut then we'll have an unused argument in some build configurations.\n\n-- Hannes\n"},{"id":"375747","messageId":"20190516215201.GF10787@sigill.intra.peff.net","threadId":"51084","inReplyTo":"20190516171953.GA265608@genre.crustytoothpaste.net","subject":"Re: [PATCH v2 6/7] config: allow configuration of multiple hook error behavior","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-16T21:52:01Z","receivedAt":"2019-05-16T21:52:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 16, 2019 at 05:19:53PM +0000, brian m. carlson wrote:\n\n> > > +\t/* Use -2 as sentinel because failure to exec is -1. */\n> > > +\tint ret = -2;\n> > \n> > Maybe this would be simpler to follow by using an enum for the handler\n> > return value?\n> \n> We can't make this variable an enum because we'd have to define 256\n> entries (well, we can, but it would be a hassle), but I can create an\n> enum and assign it to the int variable, sure.\n\nI think you can do:\n\n enum HOOK_ERR {\n\tHOOK_ERR_NONE = -2,\n\tHOOK_ERR_EXEC = -1,\n\t/* otherwise it should be a system exit code */\n\tHOOK_ERR_MAX = 255\n };\n\nwhich ensures that the enum can hold any exit status.\n\n-Peff\n"},{"id":"375827","messageId":"nycvar.QRO.7.76.6.1905172227200.46@tvgsbejvaqbjf.bet","threadId":"51084","inReplyTo":"91532c2d-7536-6e15-728b-bd18256b2ef3@kdbg.org","subject":"Re: [PATCH v2 1/7] run-command: add preliminary support for multiple hooks","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-05-17T20:31:03Z","receivedAt":"2019-05-17T20:31:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Thu, 16 May 2019, Johannes Sixt wrote:\n\n> Am 16.05.19 um 00:44 schrieb brian m. carlson:\n> > On Tue, May 14, 2019 at 05:12:39PM +0200, Johannes Schindelin wrote:\n> >> On Tue, 14 May 2019, brian m. carlson wrote:\n> >>> +/*\n> >>> + * Return 1 if a hook exists at path (which may be modified) using access(2)\n> >>> + * with check (which should be F_OK or X_OK), 0 otherwise. If strip is true,\n> >>> + * additionally consider the same filename but with STRIP_EXTENSION added.\n> >>> + * If check is X_OK, warn if the hook exists but is not executable.\n> >>> + */\n> >>> +static int has_hook(struct strbuf *path, int strip, int check)\n> >>> +{\n> >>> +\tif (access(path->buf, check) < 0) {\n> >>> +\t\tint err = errno;\n> >>> +\n> >>> +\t\tif (strip) {\n> >>> +#ifdef STRIP_EXTENSION\n> >>> +\t\t\tstrbuf_addstr(path, STRIP_EXTENSION);\n> >>> +\t\t\tif (access(path->buf, check) >= 0)\n> >>> +\t\t\t\treturn 1;\n> >>> +\t\t\tif (errno == EACCES)\n> >>> +\t\t\t\terr = errno;\n> >>> +#endif\n> >>> +\t\t}\n> >>\n> >> How about simply guarding the entire `if()`? It is a bit unusual to guard\n> >> *only* the inside block ;-)\n> >\n> > I can make that change.\n>\n> But then we'll have an unused argument in some build configurations.\n\nThat's a valid point.\n\nThanks,\nDscho\n"},{"id":"376378","messageId":"20190529021816.GI8616@genre.crustytoothpaste.net","threadId":"51084","inReplyTo":"20190515222756.GJ7458@genre.crustytoothpaste.net","subject":"Re: [PATCH v2 1/7] run-command: add preliminary support for multiple hooks","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-05-29T02:18:16Z","receivedAt":"2019-05-29T02:18:29Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2019-05-15 at 22:27:56, brian m. carlson wrote:\n> On Tue, May 14, 2019 at 07:46:17PM +0700, Duy Nguyen wrote:\n> > 'struct string_list;' should be enough (and a bit lighter) although I\n> > don't suppose it really matters.\n> \n> I can make that change.\n\nOne thing I noticed when making this change is that we're going to need\nthe definition of the struct string_list in a later patch in the series\n(originally to define a variable, but now to define a struct). So\nknowing that, I think it makes sense for us to just include the header\nup front, since we're going to be using it a few patches later.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"}]}