{"thread":{"id":"50980","subject":"[PATCH 0/5] Multiple hook support","startedAt":"2019-04-24T00:54:26Z","lastAt":"2019-05-01T21:15:29Z","messageCount":51,"participants":["brian m. carlson","Junio C Hamano","Jonathan Nieder","Ævar Arnfjörð Bjarmason","Duy Nguyen","Phillip Wood","Bryan Turner","Johannes Sixt","Barret Rhoden","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"374349","messageId":"20190424004948.728326-1-sandals@crustytoothpaste.net","threadId":"50980","inReplyTo":null,"subject":"[PATCH 0/5] Multiple hook support","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-04-24T00:49:43Z","receivedAt":"2019-04-24T00:54:26Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"Oftentimes, people want to use multiple of the same kind of hook. This\nmay be because software or a project they use requires a given hook, but\nthey would also like to have a custom hook, or because they're using\nmultiple pieces of software that both require involvement with the same\nhook.\n\nThis series introduces support for multiple hooks by using a \".d\"\ndirectory. For example, instead of using a single\n\".git/hooks/post-checkout\" file, you'd use multiple files within\n\".git/hooks/post-checkout.d\". This is the standard Unix paradigm for\nmultiple files and should be familiar to most people. Hooks are executed\nin sorted order.\n\nTo preserve backwards compatibility, we don't run the hooks in the \".d\"\ndirectory if the single file is a valid hook (i.e. it exists and is\nexecutable). This is because some people already have multiple hook\nscripts configured, and if we ran them both, we'd run the hooks twice.\nThis would be bad for e.g. the prepare-commit-msg hook. This is also the\nleast surprising behavior.\n\nWe check each hook for its exit status, even if the hook normally\nignores exit status, and if it fails, we abort executing further hooks.\nThis provides an easy way to reason about what the exit status is when a\nhook fails; we need not consider how to handle multiple failing hooks.\nIt's also consistent among all hooks, whether they care about exit\nstatus or not.\n\nThis series uses a test library to verify that we run the hooks for each\ncommand instead of writing the same tests multiple times. If there are\nother cases you'd like to see, please let me know.\n\nI've talked with some people about this approach, and they've indicated\nthey would prefer a configuration-based approach. I've tried to make the\nseries such that it can be replaced with such an approach if that's the\ndecision we make. It should be easy enough to simply replace find_hooks\nwith an appropriate implementation and update the test framework.\n\nbrian m. carlson (5):\n  run-command: add preliminary support for multiple hooks\n  builtin/receive-pack: add support for multiple hooks\n  sequencer: 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\n builtin/am.c                       |  28 ++--\n builtin/commit.c                   |   5 +-\n builtin/receive-pack.c             | 212 +++++++++++++++++------------\n builtin/worktree.c                 |  40 ++++--\n run-command.c                      | 117 ++++++++++++----\n run-command.h                      |   9 +-\n sequencer.c                        |  96 ++++++++-----\n t/lib-hooks.sh                     | 156 +++++++++++++++++++++\n t/t5403-post-checkout-hook.sh      |   8 ++\n t/t5407-post-rewrite-hook.sh       |  15 ++\n t/t5516-fetch-push.sh              |  29 ++++\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                        |  98 +++++++------\n 15 files changed, 636 insertions(+), 220 deletions(-)\n create mode 100644 t/lib-hooks.sh\n\n"},{"id":"374350","messageId":"20190424004948.728326-2-sandals@crustytoothpaste.net","threadId":"50980","inReplyTo":"20190424004948.728326-1-sandals@crustytoothpaste.net","subject":"[PATCH 1/5] run-command: add preliminary support for multiple hooks","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-04-24T00:49:44Z","receivedAt":"2019-04-24T00:54:28Z","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: brian m. carlson <sandals@crustytoothpaste.net>\n---\n builtin/commit.c           |   5 +-\n run-command.c              | 141 +++++++++++++++++++++++++--------\n run-command.h              |   7 ++\n t/lib-hooks.sh             | 156 +++++++++++++++++++++++++++++++++++++\n t/t7503-pre-commit-hook.sh |  15 ++++\n 5 files changed, 292 insertions(+), 32 deletions(-)\n create mode 100644 t/lib-hooks.sh\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex f17537474a..e7cf6b16ba 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -666,6 +666,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \tstruct strbuf sb = STRBUF_INIT;\n \tconst char *hook_arg1 = NULL;\n \tconst char *hook_arg2 = NULL;\n+\tstruct string_list *hooks;\n \tint clean_message_contents = (cleanup_mode != COMMIT_MSG_CLEANUP_NONE);\n \tint old_display_comment_prefix;\n \n@@ -943,13 +944,15 @@ 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+\thooks = find_hooks(\"pre-commit\");\n+\tif (!no_verify && hooks) {\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 \t\t * the editor and after we invoke run_status above.\n \t\t */\n \t\tdiscard_cache();\n+\t\tfree_hooks(hooks);\n \t}\n \tread_cache_from(index_file);\n \ndiff --git a/run-command.c b/run-command.c\nindex 3449db319b..669af5ebc7 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1308,58 +1308,137 @@ int async_with_fork(void)\n #endif\n }\n \n+static int has_hook(struct strbuf *path, int strip)\n+{\n+\tif (access(path->buf, X_OK) < 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, X_OK) >= 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+\tif (has_hook(&path, 1)) {\n+\t\treturn path.buf;\n+\t}\n+\treturn NULL;\n+}\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+struct string_list *find_hooks(const char *name)\n+{\n+\tstruct string_list *list = xmalloc(sizeof(*list));\n+\tstruct strbuf path = STRBUF_INIT;\n+\tDIR *d;\n+\tstruct dirent *de;\n \n-\t\tif (err == EACCES && advice_ignored_hook) {\n-\t\t\tstatic struct string_list advise_given = STRING_LIST_INIT_DUP;\n+\tstring_list_init(list, 1);\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/*\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)) {\n+\t\tstring_list_append(list, path.buf);\n+\t\treturn list;\n+\t}\n+\n+\tstrbuf_reset(&path);\n+\tstrbuf_git_path(&path, \"hooks/%s.d\", name);\n+\td = opendir(path.buf);\n+\tif (!d) {\n+\t\tstring_list_clear(list, 0);\n \t\treturn NULL;\n \t}\n-\treturn path.buf;\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))\n+\t\t\tstring_list_append(list, path.buf);\n+\t}\n+\tclosedir(d);\n+\tstrbuf_reset(&path);\n+\tif (!list->nr) {\n+\t\tfree_hooks(list);\n+\t\treturn NULL;\n+\t}\n+\n+\tstring_list_sort(list);\n+\treturn list;\n+}\n+\n+void free_hooks(struct string_list *hooks)\n+{\n+\tstring_list_clear(hooks, 0);\n+\tfree(hooks);\n }\n \n int run_hook_ve(const char *const *env, const char *name, va_list args)\n {\n-\tstruct child_process hook = CHILD_PROCESS_INIT;\n+\tstruct string_list *hooks;\n+\tstruct string_list arglist = STRING_LIST_INIT_NODUP;\n \tconst char *p;\n+\tstruct string_list_item *q;\n+\tint ret = 0;\n \n-\tp = find_hook(name);\n-\tif (!p)\n+\thooks = find_hooks(name);\n+\tif (!hooks)\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-\thook.no_stdin = 1;\n-\thook.stdout_to_stderr = 1;\n-\thook.trace2_hook_name = name;\n+\t\tstring_list_append(&arglist, p);\n \n-\treturn run_command(&hook);\n+\tfor_each_string_list_item(q, hooks) {\n+\t\tstruct child_process hook = CHILD_PROCESS_INIT;\n+\t\tstruct string_list_item *arg;\n+\n+\t\targv_array_push(&hook.args, q->string);\n+\t\tfor_each_string_list_item(arg, &arglist) {\n+\t\t\targv_array_push(&hook.args, arg->string);\n+\t\t}\n+\n+\t\thook.env = env;\n+\t\thook.no_stdin = 1;\n+\t\thook.stdout_to_stderr = 1;\n+\t\thook.trace2_hook_name = name;\n+\n+\t\tret = run_command(&hook);\n+\t\tif (ret)\n+\t\t\tbreak;\n+\t}\n+\tstring_list_clear(&arglist, 0);\n+\tfree_hooks(hooks);\n+\treturn ret;\n }\n \n int run_hook_le(const char *const *env, const char *name, ...)\ndiff --git a/run-command.h b/run-command.h\nindex a6950691c0..7266dc2969 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -68,6 +68,13 @@ 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+ */\n+struct string_list *find_hooks(const char *name);\n+/* Frees the result of find_hooks. */\n+void free_hooks(struct string_list *hooks);\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..91b3acaba2\n--- /dev/null\n+++ b/t/lib-hooks.sh\n@@ -0,0 +1,156 @@\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 '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":"374351","messageId":"20190424004948.728326-3-sandals@crustytoothpaste.net","threadId":"50980","inReplyTo":"20190424004948.728326-1-sandals@crustytoothpaste.net","subject":"[PATCH 2/5] builtin/receive-pack: add support for multiple hooks","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-04-24T00:49:45Z","receivedAt":"2019-04-24T00:54:29Z","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 <sandals@crustytoothpaste.net>\n---\n builtin/receive-pack.c | 212 ++++++++++++++++++++++++-----------------\n t/t5516-fetch-push.sh  |  29 ++++++\n 2 files changed, 152 insertions(+), 89 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 29f165d8bd..7454834d2a 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -680,72 +680,82 @@ 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 {\n-\tstruct child_process proc = CHILD_PROCESS_INIT;\n+\tstruct child_process proc;\n+\tstruct string_list *hooks;\n+\tstruct string_list_item *p;\n \tstruct async muxer;\n \tconst char *argv[2];\n-\tint code;\n+\tint code = 0;\n \n-\targv[0] = find_hook(hook_name);\n-\tif (!argv[0])\n+\thooks = find_hooks(hook_name);\n+\tif (!hooks)\n \t\treturn 0;\n \n-\targv[1] = NULL;\n+\tfor_each_string_list_item(p, hooks) {\n+\t\targv[0] = p->string;\n+\t\targv[1] = NULL;\n \n-\tproc.argv = argv;\n-\tproc.in = -1;\n-\tproc.stdout_to_stderr = 1;\n-\tproc.trace2_hook_name = hook_name;\n+\t\tchild_process_init(&proc);\n+\t\tproc.argv = argv;\n+\t\tproc.in = -1;\n+\t\tproc.stdout_to_stderr = 1;\n+\t\tproc.trace2_hook_name = hook_name;\n \n-\tif (feed_state->push_options) {\n-\t\tint i;\n-\t\tfor (i = 0; i < feed_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\targv_array_pushf(&proc.env_array, \"GIT_PUSH_OPTION_COUNT=%d\",\n-\t\t\t\t feed_state->push_options->nr);\n-\t} else\n-\t\targv_array_pushf(&proc.env_array, \"GIT_PUSH_OPTION_COUNT\");\n+\t\tif (feed_state->push_options) {\n+\t\t\tint i;\n+\t\t\tfor (i = 0; i < feed_state->push_options->nr; i++)\n+\t\t\t\targv_array_pushf(&proc.env_array,\n+\t\t\t\t\t\"GIT_PUSH_OPTION_%d=%s\", i,\n+\t\t\t\t\tfeed_state->push_options->items[i].string);\n+\t\t\targv_array_pushf(&proc.env_array, \"GIT_PUSH_OPTION_COUNT=%d\",\n+\t\t\t\t\t feed_state->push_options->nr);\n+\t\t} else\n+\t\t\targv_array_pushf(&proc.env_array, \"GIT_PUSH_OPTION_COUNT\");\n \n-\tif (tmp_objdir)\n-\t\targv_array_pushv(&proc.env_array, tmp_objdir_env(tmp_objdir));\n+\t\tif (tmp_objdir)\n+\t\t\targv_array_pushv(&proc.env_array, tmp_objdir_env(tmp_objdir));\n \n-\tif (use_sideband) {\n-\t\tmemset(&muxer, 0, sizeof(muxer));\n-\t\tmuxer.proc = copy_to_sideband;\n-\t\tmuxer.in = -1;\n-\t\tcode = start_async(&muxer);\n-\t\tif (code)\n-\t\t\treturn code;\n-\t\tproc.err = muxer.in;\n-\t}\n+\t\tif (use_sideband) {\n+\t\t\tmemset(&muxer, 0, sizeof(muxer));\n+\t\t\tmuxer.proc = copy_to_sideband;\n+\t\t\tmuxer.in = -1;\n+\t\t\tcode = start_async(&muxer);\n+\t\t\tif (code)\n+\t\t\t\tbreak;\n+\t\t\tproc.err = muxer.in;\n+\t\t}\n \n-\tprepare_push_cert_sha1(&proc);\n+\t\tprepare_push_cert_sha1(&proc);\n \n-\tcode = start_command(&proc);\n-\tif (code) {\n+\t\tcode = start_command(&proc);\n+\t\tif (code) {\n+\t\t\tif (use_sideband)\n+\t\t\t\tfinish_async(&muxer);\n+\t\t\tbreak;\n+\t\t}\n+\n+\t\tsigchain_push(SIGPIPE, SIG_IGN);\n+\n+\t\twhile (1) {\n+\t\t\tconst char *buf;\n+\t\t\tsize_t n;\n+\t\t\tif (feed(feed_state, &buf, &n))\n+\t\t\t\tbreak;\n+\t\t\tif (write_in_full(proc.in, buf, n) < 0)\n+\t\t\t\tbreak;\n+\t\t}\n+\t\tclose(proc.in);\n \t\tif (use_sideband)\n \t\t\tfinish_async(&muxer);\n-\t\treturn code;\n-\t}\n \n-\tsigchain_push(SIGPIPE, SIG_IGN);\n+\t\tsigchain_pop(SIGPIPE);\n \n-\twhile (1) {\n-\t\tconst char *buf;\n-\t\tsize_t n;\n-\t\tif (feed(feed_state, &buf, &n))\n-\t\t\tbreak;\n-\t\tif (write_in_full(proc.in, buf, n) < 0)\n+\t\tcode = finish_command(&proc);\n+\t\tif (code)\n \t\t\tbreak;\n \t}\n-\tclose(proc.in);\n-\tif (use_sideband)\n-\t\tfinish_async(&muxer);\n-\n-\tsigchain_pop(SIGPIPE);\n-\n-\treturn finish_command(&proc);\n+        free_hooks(hooks);\n+\treturn code;\n }\n \n static int feed_receive_hook(void *state_, const char **bufp, size_t *sizep)\n@@ -793,30 +803,41 @@ static int run_receive_hook(struct command *commands,\n static int run_update_hook(struct command *cmd)\n {\n \tconst char *argv[5];\n-\tstruct child_process proc = CHILD_PROCESS_INIT;\n+\tstruct child_process proc;\n+\tstruct string_list *hooks;\n+\tstruct string_list_item *p;\n \tint code;\n \n-\targv[0] = find_hook(\"update\");\n-\tif (!argv[0])\n+\thooks = find_hooks(\"update\");\n+\tif (!hooks)\n \t\treturn 0;\n \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-\targv[4] = NULL;\n+\tfor_each_string_list_item(p, hooks) {\n+\t\tchild_process_init(&proc);\n \n-\tproc.no_stdin = 1;\n-\tproc.stdout_to_stderr = 1;\n-\tproc.err = use_sideband ? -1 : 0;\n-\tproc.argv = argv;\n-\tproc.trace2_hook_name = \"update\";\n+\t\targv[0] = p->string;\n+\t\targv[1] = cmd->ref_name;\n+\t\targv[2] = oid_to_hex(&cmd->old_oid);\n+\t\targv[3] = oid_to_hex(&cmd->new_oid);\n+\t\targv[4] = NULL;\n \n-\tcode = start_command(&proc);\n-\tif (code)\n-\t\treturn code;\n-\tif (use_sideband)\n-\t\tcopy_to_sideband(proc.err, -1, NULL);\n-\treturn finish_command(&proc);\n+\t\tproc.no_stdin = 1;\n+\t\tproc.stdout_to_stderr = 1;\n+\t\tproc.err = use_sideband ? -1 : 0;\n+\t\tproc.argv = argv;\n+\t\tproc.trace2_hook_name = \"update\";\n+\n+\t\tcode = start_command(&proc);\n+\t\tif (code)\n+\t\t\treturn code;\n+\t\tif (use_sideband)\n+\t\t\tcopy_to_sideband(proc.err, -1, NULL);\n+\t\tcode = finish_command(&proc);\n+\t\tif (code)\n+\t\t\tbreak;\n+\t}\n+        free_hooks(hooks);\n+\treturn code;\n }\n \n static int is_ref_checked_out(const char *ref)\n@@ -1005,16 +1026,20 @@ static const char *update_worktree(unsigned char *sha1)\n \tconst char *retval;\n \tconst char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : \"..\";\n \tstruct argv_array env = ARGV_ARRAY_INIT;\n+\tstruct string_list *hooks;\n \n \tif (is_bare_repository())\n \t\treturn \"denyCurrentBranch = updateInstead needs a worktree\";\n \n \targv_array_pushf(&env, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n \n-\tif (!find_hook(push_to_checkout_hook))\n+\thooks = find_hooks(push_to_checkout_hook);\n+\tif (!hooks)\n \t\tretval = push_to_deploy(sha1, &env, work_tree);\n-\telse\n+\telse {\n+\t\tfree_hooks(hooks);\n \t\tretval = push_to_checkout(sha1, &env, work_tree);\n+\t}\n \n \targv_array_clear(&env);\n \treturn retval;\n@@ -1173,33 +1198,42 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n static void run_update_post_hook(struct command *commands)\n {\n \tstruct command *cmd;\n-\tstruct child_process proc = CHILD_PROCESS_INIT;\n-\tconst char *hook;\n+\tstruct child_process proc;\n+\tstruct string_list *hooks;\n+\tstruct string_list_item *p;\n \n-\thook = find_hook(\"post-update\");\n-\tif (!hook)\n+\thooks = find_hooks(\"post-update\");\n+\tif (!hooks)\n \t\treturn;\n \n-\tfor (cmd = commands; cmd; cmd = cmd->next) {\n-\t\tif (cmd->error_string || cmd->did_not_exist)\n-\t\t\tcontinue;\n+\tfor_each_string_list_item(p, hooks) {\n+\t\tchild_process_init(&proc);\n+\n+\t\tfor (cmd = commands; cmd; cmd = cmd->next) {\n+\t\t\tif (cmd->error_string || cmd->did_not_exist)\n+\t\t\t\tcontinue;\n+\t\t\tif (!proc.args.argc)\n+\t\t\t\targv_array_push(&proc.args, p->string);\n+\t\t\targv_array_push(&proc.args, cmd->ref_name);\n+\t\t}\n \t\tif (!proc.args.argc)\n-\t\t\targv_array_push(&proc.args, hook);\n-\t\targv_array_push(&proc.args, cmd->ref_name);\n-\t}\n-\tif (!proc.args.argc)\n-\t\treturn;\n+\t\t\treturn;\n \n-\tproc.no_stdin = 1;\n-\tproc.stdout_to_stderr = 1;\n-\tproc.err = use_sideband ? -1 : 0;\n-\tproc.trace2_hook_name = \"post-update\";\n+\t\tproc.no_stdin = 1;\n+\t\tproc.stdout_to_stderr = 1;\n+\t\tproc.err = use_sideband ? -1 : 0;\n+\t\tproc.trace2_hook_name = \"post-update\";\n \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\tif (!start_command(&proc)) {\n+\t\t\tint ret;\n+\t\t\tif (use_sideband)\n+\t\t\t\tcopy_to_sideband(proc.err, -1, NULL);\n+\t\t\tret = finish_command(&proc);\n+\t\t\tif (ret)\n+\t\t\t\tbreak;\n+\t\t}\n \t}\n+        free_hooks(hooks);\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 49bf4280e8..3143422344 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@@ -1700,4 +1701,32 @@ 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+\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":"374352","messageId":"20190424004948.728326-4-sandals@crustytoothpaste.net","threadId":"50980","inReplyTo":"20190424004948.728326-1-sandals@crustytoothpaste.net","subject":"[PATCH 3/5] sequencer: add support for multiple hooks","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-04-24T00:49:46Z","receivedAt":"2019-04-24T00:54:33Z","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\nSigned-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n---\n builtin/am.c                       | 28 ++++++---\n sequencer.c                        | 96 +++++++++++++++++++-----------\n t/t5407-post-rewrite-hook.sh       | 15 +++++\n t/t7505-prepare-commit-msg-hook.sh |  9 +++\n 4 files changed, 105 insertions(+), 43 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 4fb107a9d1..303abc98c2 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -442,22 +442,32 @@ static int run_applypatch_msg_hook(struct am_state *state)\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+\tstruct child_process proc;\n+\tstruct string_list *hooks;\n+\tstruct string_list_item *p;\n \tint ret;\n \n-\tif (!hook)\n+\thooks = find_hooks(\"post-rewrite\");\n+\tif (!hooks)\n \t\treturn 0;\n \n-\targv_array_push(&cp.args, hook);\n-\targv_array_push(&cp.args, \"rebase\");\n+\tfor_each_string_list_item(p, hooks) {\n+\t\tchild_process_init(&proc);\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+\t\targv_array_push(&cp.args, p->string);\n+\t\targv_array_push(&cp.args, \"rebase\");\n \n-\tret = run_command(&cp);\n+\t\tcp.in = xopen(am_path(state, \"rewritten\"), O_RDONLY);\n+\t\tcp.stdout_to_stderr = 1;\n+\t\tcp.trace2_hook_name = \"post-rewrite\";\n \n-\tclose(cp.in);\n+\t\tret = run_command(&cp);\n+\n+\t\tclose(cp.in);\n+\t\tif (ret)\n+\t\t\tbreak;\n+\t}\n+\tfree_hooks(hooks);\n \treturn ret;\n }\n \ndiff --git a/sequencer.c b/sequencer.c\nindex 79a046d748..3a616ceba6 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1088,33 +1088,69 @@ int update_head_with_reflog(const struct commit *old_head,\n static int run_rewrite_hook(const struct object_id *oldoid,\n \t\t\t    const struct object_id *newoid)\n {\n-\tstruct child_process proc = CHILD_PROCESS_INIT;\n+\tstruct child_process proc;\n+\tstruct string_list *hooks;\n+\tstruct string_list_item *p;\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+\thooks = find_hooks(\"post-rewrite\");\n+\tif (!hooks)\n \t\treturn 0;\n \n-\targv[1] = \"amend\";\n-\targv[2] = NULL;\n+\tfor_each_string_list_item(p, hooks) {\n+\t\tchild_process_init(&proc);\n \n-\tproc.argv = argv;\n-\tproc.in = -1;\n-\tproc.stdout_to_stderr = 1;\n-\tproc.trace2_hook_name = \"post-rewrite\";\n+\t\targv[0] = p->string;\n+\t\targv[1] = \"amend\";\n+\t\targv[2] = NULL;\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-\tsigchain_push(SIGPIPE, SIG_IGN);\n-\twrite_in_full(proc.in, sb.buf, sb.len);\n-\tclose(proc.in);\n-\tstrbuf_release(&sb);\n-\tsigchain_pop(SIGPIPE);\n-\treturn finish_command(&proc);\n+\t\tproc.argv = argv;\n+\t\tproc.in = -1;\n+\t\tproc.stdout_to_stderr = 1;\n+\t\tproc.trace2_hook_name = \"post-rewrite\";\n+\n+\t\tcode = start_command(&proc);\n+\t\tif (code)\n+\t\t\treturn code;\n+\t\tstrbuf_addf(&sb, \"%s %s\\n\", oid_to_hex(oldoid), oid_to_hex(newoid));\n+\t\tsigchain_push(SIGPIPE, SIG_IGN);\n+\t\twrite_in_full(proc.in, sb.buf, sb.len);\n+\t\tclose(proc.in);\n+\t\tstrbuf_release(&sb);\n+\t\tsigchain_pop(SIGPIPE);\n+\t\tcode = finish_command(&proc);\n+\t\tif (code)\n+\t\t\tbreak;\n+\t}\n+\tfree_hooks(hooks);\n+\treturn code;\n+}\n+\n+static void run_interactive_rewrite_hook(void)\n+{\n+\tstruct string_list *hooks;\n+\tstruct string_list_item *p;\n+\tstruct child_process child;\n+\n+\thooks = find_hooks(\"post-rewrite\");\n+\tif (!hooks)\n+\t\treturn;\n+\n+\tfor_each_string_list_item(p, hooks) {\n+\t\tchild_process_init(&child);\n+\n+\t\tchild.in = open(rebase_path_rewritten_list(),\n+\t\t\tO_RDONLY);\n+\t\tchild.stdout_to_stderr = 1;\n+\t\tchild.trace2_hook_name = \"post-rewrite\";\n+\t\targv_array_push(&child.args, p->string);\n+\t\targv_array_push(&child.args, \"rebase\");\n+\t\tif (run_command(&child))\n+\t\t\tbreak;\n+\t}\n+\tfree_hooks(hooks);\n }\n \n void commit_post_rewrite(struct repository *r,\n@@ -1326,6 +1362,7 @@ static int try_to_commit(struct repository *r,\n \tchar *amend_author = NULL;\n \tconst char *hook_commit = NULL;\n \tenum commit_msg_cleanup_mode cleanup;\n+\tstruct string_list *hooks;\n \tint res = 0;\n \n \tif (parse_head(r, &current_head))\n@@ -1369,7 +1406,10 @@ static int try_to_commit(struct repository *r,\n \t\tgoto out;\n \t}\n \n-\tif (find_hook(\"prepare-commit-msg\")) {\n+\thooks = find_hooks(\"prepare-commit-msg\");\n+\tif (hooks) {\n+\t\tfree_hooks(hooks);\n+\n \t\tres = run_prepare_commit_msg_hook(r, msg, hook_commit);\n \t\tif (res)\n \t\t\tgoto out;\n@@ -3771,8 +3811,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@@ -3782,18 +3820,8 @@ 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\trun_interactive_rewrite_hook();\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..5b83f037b5 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+commit_command () {\n+\techo \"$1\" >>file &&\n+\tgit add file &&\n+\tgit commit -m \"$1\"\n+}\n+\n+test_multiple_hooks prepare-commit-msg commit_command\n+\n test_done\n"},{"id":"374353","messageId":"20190424004948.728326-6-sandals@crustytoothpaste.net","threadId":"50980","inReplyTo":"20190424004948.728326-1-sandals@crustytoothpaste.net","subject":"[PATCH 5/5] transport: add support for multiple pre-push hooks","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-04-24T00:49:48Z","receivedAt":"2019-04-24T00:54:36Z","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 <sandals@crustytoothpaste.net>\n---\n run-command.c            | 12 -----\n run-command.h            |  6 ---\n t/t5571-pre-push-hook.sh | 19 ++++++++\n transport.c              | 98 ++++++++++++++++++++++------------------\n 4 files changed, 74 insertions(+), 61 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 669af5ebc7..2c387a7aef 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1340,18 +1340,6 @@ static int has_hook(struct strbuf *path, int strip)\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)) {\n-\t\treturn path.buf;\n-\t}\n-\treturn NULL;\n-}\n-\n struct string_list *find_hooks(const char *name)\n {\n \tstruct string_list *list = xmalloc(sizeof(*list));\ndiff --git a/run-command.h b/run-command.h\nindex 7266dc2969..a657c09a6c 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -62,12 +62,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 d0608df5c9..a2a9437c5b 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1049,61 +1049,73 @@ static int run_pre_push_hook(struct transport *transport,\n {\n \tint ret = 0, x;\n \tstruct ref *r;\n-\tstruct child_process proc = CHILD_PROCESS_INIT;\n+\tstruct child_process proc;\n+\tstruct string_list *hooks;\n+\tstruct string_list_item *p;\n \tstruct strbuf buf;\n \tconst char *argv[4];\n \n-\tif (!(argv[0] = find_hook(\"pre-push\")))\n+\thooks = find_hooks(\"pre-push\");\n+\tif (!hooks)\n \t\treturn 0;\n \n-\targv[1] = transport->remote->name;\n-\targv[2] = transport->url;\n-\targv[3] = NULL;\n \n-\tproc.argv = argv;\n-\tproc.in = -1;\n-\tproc.trace2_hook_name = \"pre-push\";\n+\tfor_each_string_list_item(p, hooks) {\n+\t\tchild_process_init(&proc);\n \n-\tif (start_command(&proc)) {\n-\t\tfinish_command(&proc);\n-\t\treturn -1;\n-\t}\n+\t\targv[0] = p->string;\n+\t\targv[1] = transport->remote->name;\n+\t\targv[2] = transport->url;\n+\t\targv[3] = NULL;\n \n-\tsigchain_push(SIGPIPE, SIG_IGN);\n+\t\tproc.argv = argv;\n+\t\tproc.in = -1;\n+\t\tproc.trace2_hook_name = \"pre-push\";\n \n-\tstrbuf_init(&buf, 256);\n-\n-\tfor (r = 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-\t\tif (r->status == REF_STATUS_UPTODATE) continue;\n-\n-\t\tstrbuf_reset(&buf);\n-\t\tstrbuf_addf( &buf, \"%s %s %s %s\\n\",\n-\t\t\t r->peer_ref->name, oid_to_hex(&r->new_oid),\n-\t\t\t r->name, oid_to_hex(&r->old_oid));\n-\n-\t\tif (write_in_full(proc.in, buf.buf, buf.len) < 0) {\n-\t\t\t/* We do not mind if a hook does not read all refs. */\n-\t\t\tif (errno != EPIPE)\n-\t\t\t\tret = -1;\n+\t\tif (start_command(&proc)) {\n+\t\t\tfinish_command(&proc);\n+\t\t\tret = -1;\n \t\t\tbreak;\n \t\t}\n+\n+\t\tsigchain_push(SIGPIPE, SIG_IGN);\n+\n+\t\tstrbuf_init(&buf, 256);\n+\n+\t\tfor (r = remote_refs; r; r = r->next) {\n+\t\t\tif (!r->peer_ref) continue;\n+\t\t\tif (r->status == REF_STATUS_REJECT_NONFASTFORWARD) continue;\n+\t\t\tif (r->status == REF_STATUS_REJECT_STALE) continue;\n+\t\t\tif (r->status == REF_STATUS_UPTODATE) continue;\n+\n+\t\t\tstrbuf_reset(&buf);\n+\t\t\tstrbuf_addf( &buf, \"%s %s %s %s\\n\",\n+\t\t\t\t r->peer_ref->name, oid_to_hex(&r->new_oid),\n+\t\t\t\t r->name, oid_to_hex(&r->old_oid));\n+\n+\t\t\tif (write_in_full(proc.in, buf.buf, buf.len) < 0) {\n+\t\t\t\t/* We do not mind if a hook does not read all refs. */\n+\t\t\t\tif (errno != EPIPE)\n+\t\t\t\t\tret = -1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\n+\t\tstrbuf_release(&buf);\n+\n+\t\tx = close(proc.in);\n+\t\tif (!ret)\n+\t\t\tret = x;\n+\n+\t\tsigchain_pop(SIGPIPE);\n+\n+\t\tx = finish_command(&proc);\n+\t\tif (!ret)\n+\t\t\tret = x;\n+\t\tif (ret)\n+\t\t\tbreak;\n \t}\n-\n-\tstrbuf_release(&buf);\n-\n-\tx = close(proc.in);\n-\tif (!ret)\n-\t\tret = x;\n-\n-\tsigchain_pop(SIGPIPE);\n-\n-\tx = finish_command(&proc);\n-\tif (!ret)\n-\t\tret = x;\n-\n+\tfree_hooks(hooks);\n \treturn ret;\n }\n \n"},{"id":"374354","messageId":"20190424004948.728326-5-sandals@crustytoothpaste.net","threadId":"50980","inReplyTo":"20190424004948.728326-1-sandals@crustytoothpaste.net","subject":"[PATCH 4/5] builtin/worktree: add support for multiple post-checkout hooks","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-04-24T00:49:47Z","receivedAt":"2019-04-24T00:54:37Z","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 <sandals@crustytoothpaste.net>\n---\n builtin/worktree.c            | 40 ++++++++++++++++++++++-------------\n t/t5403-post-checkout-hook.sh |  8 +++++++\n 2 files changed, 33 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex d2a7e2f3f1..3126fa3a82 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -395,21 +395,31 @@ 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\tstruct string_list *hooks;\n+\t\thooks = find_hooks(\"post-checkout\");\n+\t\tif (hooks) {\n+\t\t\tstruct string_list_item *p;\n+\t\t\tstruct child_process cp;\n+\t\t\tfor_each_string_list_item(p, hooks) {\n+\t\t\t\tconst char *env[] = { \"GIT_DIR\", \"GIT_WORK_TREE\", NULL };\n+\t\t\t\tchild_process_init(&cp);\n+\n+\t\t\t\tcp.git_cmd = 0;\n+\t\t\t\tcp.no_stdin = 1;\n+\t\t\t\tcp.stdout_to_stderr = 1;\n+\t\t\t\tcp.dir = path;\n+\t\t\t\tcp.env = env;\n+\t\t\t\tcp.argv = NULL;\n+\t\t\t\tcp.trace2_hook_name = \"post-checkout\";\n+\t\t\t\targv_array_pushl(&cp.args, absolute_path(p->string),\n+\t\t\t\t\t\t oid_to_hex(&null_oid),\n+\t\t\t\t\t\t oid_to_hex(&commit->object.oid),\n+\t\t\t\t\t\t \"1\", NULL);\n+\t\t\t\tret = run_command(&cp);\n+\t\t\t\tif (ret)\n+\t\t\t\t\tbreak;\n+\t\t\t}\n+\t\t\tfree_hooks(hooks);\n \t\t}\n \t}\n \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":"374363","messageId":"xmqqtveo2lyx.fsf@gitster-ct.c.googlers.com","threadId":"50980","inReplyTo":"20190424004948.728326-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH 0/5] Multiple hook support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-24T02:09:10Z","receivedAt":"2019-04-24T02:09:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> To preserve backwards compatibility, we don't run the hooks in the \".d\"\n> directory if the single file is a valid hook (i.e. it exists and is\n> executable). This is because some people already have multiple hook\n> scripts configured, and if we ran them both, we'd run the hooks twice.\n> This would be bad for e.g. the prepare-commit-msg hook. This is also the\n> least surprising behavior.\n\nOK.  An obvious alternative may be to see if the expected hooks path\nis a directory and use the contents.  If \".git/hooks/pre-commit\" is\na single file, we know it is the single hook as before, and if it is\na directory, we know that is not a custom made (i.e. from the world\nbefore this series supported in the core-git) multi-hook setup.\n\n> We check each hook for its exit status, even if the hook normally\n> ignores exit status, and if it fails, we abort executing further hooks.\n\nThis part may become the most controversial in the whole topic, but\na design discussion is helped by having a concrete proposal that\nmakes its own design decision, and this is the simplest design of\nthe failure case that is the easiest to understand.\n\nThanks.  Let's see how the review discussion goes ;-)\n\n> brian m. carlson (5):\n>   run-command: add preliminary support for multiple hooks\n>   builtin/receive-pack: add support for multiple hooks\n>   sequencer: 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>\n>  builtin/am.c                       |  28 ++--\n>  builtin/commit.c                   |   5 +-\n>  builtin/receive-pack.c             | 212 +++++++++++++++++------------\n>  builtin/worktree.c                 |  40 ++++--\n>  run-command.c                      | 117 ++++++++++++----\n>  run-command.h                      |   9 +-\n>  sequencer.c                        |  96 ++++++++-----\n>  t/lib-hooks.sh                     | 156 +++++++++++++++++++++\n>  t/t5403-post-checkout-hook.sh      |   8 ++\n>  t/t5407-post-rewrite-hook.sh       |  15 ++\n>  t/t5516-fetch-push.sh              |  29 ++++\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                        |  98 +++++++------\n>  15 files changed, 636 insertions(+), 220 deletions(-)\n>  create mode 100644 t/lib-hooks.sh\n"},{"id":"374364","messageId":"20190424022240.GH6316@genre.crustytoothpaste.net","threadId":"50980","inReplyTo":"xmqqtveo2lyx.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/5] Multiple hook support","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-04-24T02:22:41Z","receivedAt":"2019-04-24T02:22:51Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Apr 24, 2019 at 11:09:10AM +0900, Junio C Hamano wrote:\n> \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n> \n> > To preserve backwards compatibility, we don't run the hooks in the \".d\"\n> > directory if the single file is a valid hook (i.e. it exists and is\n> > executable). This is because some people already have multiple hook\n> > scripts configured, and if we ran them both, we'd run the hooks twice.\n> > This would be bad for e.g. the prepare-commit-msg hook. This is also the\n> > least surprising behavior.\n> \n> OK.  An obvious alternative may be to see if the expected hooks path\n> is a directory and use the contents.  If \".git/hooks/pre-commit\" is\n> a single file, we know it is the single hook as before, and if it is\n> a directory, we know that is not a custom made (i.e. from the world\n> before this series supported in the core-git) multi-hook setup.\n\nThat's an idea I hadn't considered. I'm interested to hear other folks'\nideas on it, but that certainly avoids a lot of the problems in my\napproach.\n\n> > We check each hook for its exit status, even if the hook normally\n> > ignores exit status, and if it fails, we abort executing further hooks.\n> \n> This part may become the most controversial in the whole topic, but\n> a design discussion is helped by having a concrete proposal that\n> makes its own design decision, and this is the simplest design of\n> the failure case that is the easiest to understand.\n\nI expected it might be. I'm not strongly wedded to it, so if people make\na good argument for something else, I'm okay with changing it.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"374365","messageId":"xmqqo94w2l3k.fsf@gitster-ct.c.googlers.com","threadId":"50980","inReplyTo":"20190424004948.728326-2-sandals@crustytoothpaste.net","subject":"Re: [PATCH 1/5] run-command: add preliminary support for multiple hooks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-24T02:27:59Z","receivedAt":"2019-04-24T02:28:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index f17537474a..e7cf6b16ba 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -666,6 +666,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n>  \tstruct strbuf sb = STRBUF_INIT;\n>  \tconst char *hook_arg1 = NULL;\n>  \tconst char *hook_arg2 = NULL;\n> +\tstruct string_list *hooks;\n>  \tint clean_message_contents = (cleanup_mode != COMMIT_MSG_CLEANUP_NONE);\n>  \tint old_display_comment_prefix;\n>  \n> @@ -943,13 +944,15 @@ 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> +\thooks = find_hooks(\"pre-commit\");\n> +\tif (!no_verify && hooks) {\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>  \t\t * the editor and after we invoke run_status above.\n>  \t\t */\n>  \t\tdiscard_cache();\n> +\t\tfree_hooks(hooks);\n>  \t}\n>  \tread_cache_from(index_file);\n\nOK, so find_hook() that used to return a single hook now can return\na list of hook scripts.  Running the single one becomes a simple\nspecial case of \"run each of them in turn, and stop at the first\nfailure\".\n\n> diff --git a/run-command.c b/run-command.c\n> index 3449db319b..669af5ebc7 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -1308,58 +1308,137 @@ int async_with_fork(void)\n>  #endif\n>  }\n>  \n> +static int has_hook(struct strbuf *path, int strip)\n> +{\n> +\tif (access(path->buf, X_OK) < 0) {\n\nDoes \".git/post-commit\" that is not an executable exist?\n\nIt was perfectly fine for find_hook() to say \"there is no hook for\npost-commit\" in the old world in such a case, because the\nunexecutable file it found is not going to be run anyway.\n\nBut it is not clear if has_hook(), that affects \"there is no single\nhook file for post-commit, so let's look at post-commit.d\" decision\nmade by find_hooks(), should behave that way.  It somehow feels more\nintuitive if a post-commit file that is not executable, by merely\nexisting, stops post-commit.d directory from being scanned, at least\nto me.\n\n>  int run_hook_ve(const char *const *env, const char *name, va_list args)\n>  {\n> -\tstruct child_process hook = CHILD_PROCESS_INIT;\n> +\tstruct string_list *hooks;\n> +\tstruct string_list arglist = STRING_LIST_INIT_NODUP;\n>  \tconst char *p;\n> +\tstruct string_list_item *q;\n> +\tint ret = 0;\n>   ...\n> +\t\thook.env = env;\n> +\t\thook.no_stdin = 1;\n> +\t\thook.stdout_to_stderr = 1;\n> +\t\thook.trace2_hook_name = name;\n> +\n> +\t\tret = run_command(&hook);\n> +\t\tif (ret)\n> +\t\t\tbreak;\n> +\t}\n> +\tstring_list_clear(&arglist, 0);\n> +\tfree_hooks(hooks);\n> +\treturn ret;\n>  }\n\nThese \"run with command line arguments as its sole input, with the\nexit status as its sole output\" style hooks are easily handled and\nthe above looks like reasonable enhancement to the existing\nabstraction (e.g. run 'prepare-commit-msg' hook with these\narguments).\n\nI however wonder how the hooks in the other style should/can be\nhandled, that are fed data from their standard input stream, and\nreturns more than one bit via their standard output stream.  In\nany case, they are not in the scope of this step.\n\n"},{"id":"374366","messageId":"20190424023438.GE98980@google.com","threadId":"50980","inReplyTo":"20190424004948.728326-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH 0/5] Multiple hook support","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-04-24T02:34:38Z","receivedAt":"2019-04-24T02:34:44Z","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 talked with some people about this approach, and they've indicated\n> they would prefer a configuration-based approach.\n\nI would, too, mostly because that reduces the problem of securing\nhooks to securing configuration.  See\nhttps://public-inbox.org/git/20171002234517.GV19555@aiede.mtv.corp.google.com/\nfor more on this subject.\n\nMore precisely, a few problems with the current hooks system:\n\n 1. There's not a standard way to have multiple hooks for a single event.\n    That's what this series is about.\n\n    (The recommended workaround has been to use a trampoline script as\n    your hook, and to make that trampoline script implement whatever\n    policy for the order of invocation and accumulation of results is\n    appropriate, but that's a bit of a cop-out.)\n\n 2. Because they are stored in the Git repository, they do not have a\n    way to be automatically updated.\n\n    (The recommended workaround is to use a trampoline script as your\n    hook and put the actual hook somewhere standard like $PATH where\n    it can be upgraded system-wide.  But that's a bit of a cop-out.)\n\n 3. Because they are part of the Git repository, it is very easy to\n    compromise a user's account by tricking them into running an\n    attacker-authored hook.  Attacks include \"hey admin, can you tell\n    me why 'git commit' is failing in this repo?\" and \"here's a zip file\n    containing a Git repository with our fancy software.  Feel free\n    to look around, run 'git pull', etc\".\n\n    Similar attacks, probably even worse, apply to shell prompt scripts\n    using commands from attacker-controlled .git/config.\n\n    (The recommended workaround is to inspect .git/config and\n    .git/hooks whenever you're looking at an untrusted repository, and\n    to write your shell prompt script defensively.)\n\nSolving (1) without (2) feels like a bit of a missed opportunity to\nme.  Ideally, what I would like is\n\n   i. A central registry of trustworthy Git hooks that can be upgraded\n      using the system package manager to address (2).  Perhaps just\n      git-hook-* commands on the $PATH.\n\n  ii. Instead of putting hooks in .git/hooks, put a list of hooks to\n      run for each event in .git/config.\n\n iii. For backward compatibility, perform a multi-stage migration.\n      In the stage I am most interested in:\n\n      When encountering a hook in .git/hooks, don't run it, but print\n      a message about how to migrate it to the modern scheme.\n\n      To make migration to the modern scheme painless, stick a\n      standard trampoline script in .git/hooks in all converted and\n      all newly \"git init\"ed repositories to allow old versions of Git\n      to respect the configuration from (i) and (ii).\n\nThat doesn't handle core.pager et al, but those we can handle\nseparately (for example by, at least optionally, not respecting values\nfor them in per-repo config at all).\n\nThanks for tackling this.  What do you think?\n\nThanks,\nJonathan\n"},{"id":"374367","messageId":"xmqqk1fk2kgq.fsf@gitster-ct.c.googlers.com","threadId":"50980","inReplyTo":"20190424022240.GH6316@genre.crustytoothpaste.net","subject":"Re: [PATCH 0/5] Multiple hook support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-24T02:41:41Z","receivedAt":"2019-04-24T02:41:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> On Wed, Apr 24, 2019 at 11:09:10AM +0900, Junio C Hamano wrote:\n>> \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n>> \n>> > To preserve backwards compatibility, we don't run the hooks in the \".d\"\n>> > directory if the single file is a valid hook (i.e. it exists and is\n>> > executable). This is because some people already have multiple hook\n>> > scripts configured, and if we ran them both, we'd run the hooks twice.\n>> > This would be bad for e.g. the prepare-commit-msg hook. This is also the\n>> > least surprising behavior.\n>> \n>> OK.  An obvious alternative may be to see if the expected hooks path\n>> is a directory and use the contents.  If \".git/hooks/pre-commit\" is\n>> a single file, we know it is the single hook as before, and if it is\n>> a directory, we know that is not a custom made (i.e. from the world\n>> before this series supported in the core-git) multi-hook setup.\n>\n> That's an idea I hadn't considered. I'm interested to hear other folks'\n> ideas on it, but that certainly avoids a lot of the problems in my\n> approach.\n\nOne downside is that the transition from the old to the new world\norder becomes a bigger deal.  You could have been using a precursor\nof this series implemented entirely as a shell script\n\n\t$ cat >.git/pre-commit <<\\EOF\n\t#!/bin/sh\n\tfor x in .git/pre-commit.d/*\n\tdo\n\t\t\"$x\" \"$@\" || exit 1\n\tdone\n\texit 0\n\tEOF\n\t$ chmod +x .git/pre-commit\n\nand with your approach, the user can keep using that set-up and then\nsimply remove that single file once all the Git executables the user\nuses are natively aware of the multi-hook setup.  With \"either a\nfile or directory\" approach, the transition would be removing the\nfile *and* renaming pre-commit.d directory to pre-commit.\n"},{"id":"374372","messageId":"87wojjsv9p.fsf@evledraar.gmail.com","threadId":"50980","inReplyTo":"20190424023438.GE98980@google.com","subject":"Re: [PATCH 0/5] Multiple hook support","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-04-24T07:43:46Z","receivedAt":"2019-04-24T07:43:52Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Apr 24 2019, Jonathan Nieder wrote:\n\n> brian m. carlson wrote:\n\nbrian: I'm very interested in this. I barked up this tree before almost\nexactly 3 years ago:\n\n    https://public-inbox.org/git/CACBZZX6j6q2DUN_Z-Pnent1u714dVNPFBrL_PiEQyLmCzLUVxg@mail.gmail.com/\n    https://public-inbox.org/git/1461367997-28745-1-git-send-email-avarab@gmail.com/\n\nIf you haven't seen those threads you might find them interesting. In\nparticular there's a previous discussion about the \"exit on first fail\"\nv.s. \"run them all\" semantics. I'll elaborate elsewhere in this thread.\n\nThe only bit that landed from that was 867ad08a26 (\"hooks: allow\ncustomizing where the hook directory is\", 2016-05-04), which, in reply\nto JN below...:\n\n>> I've talked with some people about this approach, and they've indicated\n>> they would prefer a configuration-based approach.\n>\n> I would, too, mostly because that reduces the problem of securing\n> hooks to securing configuration.  See\n> https://public-inbox.org/git/20171002234517.GV19555@aiede.mtv.corp.google.com/\n> for more on this subject.\n>\n> More precisely, a few problems with the current hooks system:\n>\n>  1. There's not a standard way to have multiple hooks for a single event.\n>     That's what this series is about.\n>\n>     (The recommended workaround has been to use a trampoline script as\n>     your hook, and to make that trampoline script implement whatever\n>     policy for the order of invocation and accumulation of results is\n>     appropriate, but that's a bit of a cop-out.)\n>\n>  2. Because they are stored in the Git repository, they do not have a\n>     way to be automatically updated.\n>\n>     (The recommended workaround is to use a trampoline script as your\n>     hook and put the actual hook somewhere standard like $PATH where\n>     it can be upgraded system-wide.  But that's a bit of a cop-out.)\n\nYou can accomplish this with core.hooksPath, and presumably a\ncombination of core.hooksPath and brian's patches here. That was my\ntwo-step plan in 2016, but I obviously never got to step #2.\n\nSo in /etc/gitconfig on your server you set core.hooksPath=/etc/githooks\nand then your pre-receive hook will be /etc/githooks/pre-receive, or\n/etc/githooks/pre-receive.d/*.\n\n>  3. Because they are part of the Git repository, it is very easy to\n>     compromise a user's account by tricking them into running an\n>     attacker-authored hook.  Attacks include \"hey admin, can you tell\n>     me why 'git commit' is failing in this repo?\" and \"here's a zip file\n>     containing a Git repository with our fancy software.  Feel free\n>     to look around, run 'git pull', etc\".\n>\n>     Similar attacks, probably even worse, apply to shell prompt scripts\n>     using commands from attacker-controlled .git/config.\n>\n>     (The recommended workaround is to inspect .git/config and\n>     .git/hooks whenever you're looking at an untrusted repository, and\n>     to write your shell prompt script defensively.)\n>\n> Solving (1) without (2) feels like a bit of a missed opportunity to\n> me.  Ideally, what I would like is\n>\n>    i. A central registry of trustworthy Git hooks that can be upgraded\n>       using the system package manager to address (2).  Perhaps just\n>       git-hook-* commands on the $PATH.\n>\n>   ii. Instead of putting hooks in .git/hooks, put a list of hooks to\n>       run for each event in .git/config.\n>\n>  iii. For backward compatibility, perform a multi-stage migration.\n>       In the stage I am most interested in:\n>\n>       When encountering a hook in .git/hooks, don't run it, but print\n>       a message about how to migrate it to the modern scheme.\n>\n>       To make migration to the modern scheme painless, stick a\n>       standard trampoline script in .git/hooks in all converted and\n>       all newly \"git init\"ed repositories to allow old versions of Git\n>       to respect the configuration from (i) and (ii).\n>\n> That doesn't handle core.pager et al, but those we can handle\n> separately (for example by, at least optionally, not respecting values\n> for them in per-repo config at all).\n>\n> Thanks for tackling this.  What do you think?\n>\n> Thanks,\n> Jonathan\n"},{"id":"374375","messageId":"87tvensu1a.fsf@evledraar.gmail.com","threadId":"50980","inReplyTo":"20190424004948.728326-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH 0/5] Multiple hook support","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-04-24T08:10:25Z","receivedAt":"2019-04-24T08:10:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Apr 24 2019, brian m. carlson wrote:\n\n> Oftentimes, people want to use multiple of the same kind of hook. This\n> may be because software or a project they use requires a given hook, but\n> they would also like to have a custom hook, or because they're using\n> multiple pieces of software that both require involvement with the same\n> hook.\n>\n> This series introduces support for multiple hooks by using a \".d\"\n> directory. For example, instead of using a single\n> \".git/hooks/post-checkout\" file, you'd use multiple files within\n> \".git/hooks/post-checkout.d\". This is the standard Unix paradigm for\n> multiple files and should be familiar to most people. Hooks are executed\n> in sorted order.\n\nI think it's interesting if people can chime in with current in-the-wild\nimplementations of this.\n\nI know GitLab's the best, not because I was in any way involved in it,\nI've just dealt with writing hooks for it:\nhttps://docs.gitlab.com/ee/administration/custom_hooks.html#chained-hooks-support\n\nThere:\n\n 1. The instance has a 'hooks' dir in the .git repo that's a symlink to\n    /my/global/hooks. They could also use a /etc/gitconfig\n    core.hooksPath for this part, but whatever.\n\n 2. That /my/global/hooks has e.g. a /my/global/hooks/pre-receive that\n    git itself runs, which is a trampoline script that runs all over the\n    place and executes global/per-project hooks (which live in\n    .git/custom_hooks/).\n\n 3. \"The hooks of the same type are executed in order and execution\n     stops on the first script exiting with a non-zero value.\"\n\nI wonder if the eventual goal of this facility would be to get such\nusers on board with using git's native feature for this. This series is\nmost of the way there for GitLab's case, but not quite.\n\n> To preserve backwards compatibility, we don't run the hooks in the \".d\"\n> directory if the single file is a valid hook (i.e. it exists and is\n> executable). This is because some people already have multiple hook\n> scripts configured, and if we ran them both, we'd run the hooks twice.\n> This would be bad for e.g. the prepare-commit-msg hook. This is also the\n> least surprising behavior.\n>\n> We check each hook for its exit status, even if the hook normally\n> ignores exit status, and if it fails, we abort executing further hooks.\n> This provides an easy way to reason about what the exit status is when a\n> hook fails; we need not consider how to handle multiple failing hooks.\n> It's also consistent among all hooks, whether they care about exit\n> status or not.\n\nOthers have replied to this already. I linked to the 2016 discussion of\nmy RFC for this in\nhttps://public-inbox.org/git/87wojjsv9p.fsf@evledraar.gmail.com where I\nmade the same choice. Some points on that:\n\n * There was the mention of \"but what if someone wants to run them all\",\n   e.g. for logging to N logging systems where one fails, that's been\n   brought up again this time around by others.\n\n * The case I find more interesting is the ability to run the hooks in\n   parallel. Saying \"glob order and exit on first fail\" categorically\n   closes the door to that.\n\n> I've talked with some people about this approach, and they've indicated\n> they would prefer a configuration-based approach. I've tried to make the\n> series such that it can be replaced with such an approach if that's the\n> decision we make. It should be easy enough to simply replace find_hooks\n> with an appropriate implementation and update the test framework.\n\nI think whatever opinions we all have on the current implementation it's\nOK to get this in in *some* form and just made it configurable or\nwhatever later.\n\nMost of the work is the ability to run N hooks, how exactly that happens\ncan be tweaked later, and if this series lands and someone isn't 100%\nhappy with the semantics they're no worse off than they are now.\n\nI.e. we could get something like this in its current form, and later have:\n\n    core.hooksOrder = glob | random\n    core.hooksHaltOnError = true | never\n\nWhere we'd say that what this series does is core.hooksOrder=glob and\ncore.hooksHaltOnError=true, but that e.g. parallel \"run them all\" could\nbe done in the future with core.hooksOrder=random &\ncore.hooksHaltOnError=never\n"},{"id":"374376","messageId":"87sgu7sttp.fsf@evledraar.gmail.com","threadId":"50980","inReplyTo":"20190424022240.GH6316@genre.crustytoothpaste.net","subject":"Re: [PATCH 0/5] Multiple hook support","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-04-24T08:14:58Z","receivedAt":"2019-04-24T08:15:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Apr 24 2019, brian m. carlson wrote:\n\n> On Wed, Apr 24, 2019 at 11:09:10AM +0900, Junio C Hamano wrote:\n>> \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n>>\n>> > To preserve backwards compatibility, we don't run the hooks in the \".d\"\n>> > directory if the single file is a valid hook (i.e. it exists and is\n>> > executable). This is because some people already have multiple hook\n>> > scripts configured, and if we ran them both, we'd run the hooks twice.\n>> > This would be bad for e.g. the prepare-commit-msg hook. This is also the\n>> > least surprising behavior.\n>>\n>> OK.  An obvious alternative may be to see if the expected hooks path\n>> is a directory and use the contents.  If \".git/hooks/pre-commit\" is\n>> a single file, we know it is the single hook as before, and if it is\n>> a directory, we know that is not a custom made (i.e. from the world\n>> before this series supported in the core-git) multi-hook setup.\n>\n> That's an idea I hadn't considered. I'm interested to hear other folks'\n> ideas on it, but that certainly avoids a lot of the problems in my\n> approach.\n\nTwo things on that:\n\n 1. As noted upthread we have some in-the-wild users who use the *.d\n    semantics already. Would be nice to be able to migrate them, also\n    *.d is a commonly-used pattern in other software.\n\n 2. It's more of a pain in some configuration management systems like\n    e.g. puppet to change a managed file to a directory than removing an\n    existing file and adding a directory.\n"},{"id":"374377","messageId":"87r29rsthz.fsf@evledraar.gmail.com","threadId":"50980","inReplyTo":"20190424023438.GE98980@google.com","subject":"Re: [PATCH 0/5] Multiple hook support","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-04-24T08:22:00Z","receivedAt":"2019-04-24T08:22:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Apr 24 2019, Jonathan Nieder wrote:\n\n> Hi,\n>\n> brian m. carlson wrote:\n>\n>> I've talked with some people about this approach, and they've indicated\n>> they would prefer a configuration-based approach.\n>\n> I would, too, mostly because that reduces the problem of securing\n> hooks to securing configuration.  See\n> https://public-inbox.org/git/20171002234517.GV19555@aiede.mtv.corp.google.com/\n> for more on this subject.\n\nI hadn't noticed this E-Mail when I later wrote similar musings on the\nsubject:\n\nhttps://public-inbox.org/git/87zi6eakkt.fsf@evledraar.gmail.com/\nhttps://public-inbox.org/git/874lkq11ug.fsf@evledraar.gmail.com/\n\nI.e. what I wanted out of it was an in-repository .gitconfig instead of\ninspecting some some random repo with a .git/config.\n\nBut the problem to be solved is the same: The ability to carefully poke\na config file with a stick at a distance, and carefully whitelist which\nparts of it (if any) you want to trust.\n"},{"id":"374379","messageId":"CACsJy8CKwiNknEkEH_-DxuXYiBmpYvfdRAZvmSJCcqko_OYnTw@mail.gmail.com","threadId":"50980","inReplyTo":"20190424004948.728326-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH 0/5] Multiple hook support","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-04-24T09:49:54Z","receivedAt":"2019-04-24T09:50:22Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Apr 24, 2019 at 7:54 AM brian m. carlson\n<sandals@crustytoothpaste.net> wrote:\n>\n> Oftentimes, people want to use multiple of the same kind of hook. This\n> may be because software or a project they use requires a given hook, but\n> they would also like to have a custom hook, or because they're using\n> multiple pieces of software that both require involvement with the same\n> hook.\n\nHeh you beat me to it. My config-hooks branch [1] has not been updated\nfor half a year. I only skimmed through quickly so no useful comments,\nbut I went with a slightly different design, introducing\nfor_each_hook() instead (see run-command.[ch] in the last patch).\n\nBack then I wasn't sure what would be the right policy for running\nmultiple hooks. By keeping the hook loop in one place, we could change\nthat policy much easier later (e.g. continue anyway in case of\nfailure, or stop as soon as you see a problem, or even run some\nspecial hooks at the end even when there is an error earlier).\n\nI'm not saying that should be the way to go. Just pointing it out. You\ndecide. Personally I think open coding the loop is just as well.\n\n[1] https://gitlab.com/pclouds/git/commits/config-hooks\n-- \nDuy\n"},{"id":"374380","messageId":"ed0312f5-d174-76e0-fea7-5de93f5840b8@gmail.com","threadId":"50980","inReplyTo":"20190424004948.728326-4-sandals@crustytoothpaste.net","subject":"Re: [PATCH 3/5] sequencer: add support for multiple hooks","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-04-24T09:51:56Z","receivedAt":"2019-04-24T09:52:00Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 24/04/2019 01:49, brian m. carlson wrote:\n> Add support for multiple post-rewrite hooks, both for \"git commit\n> --amend\" and \"git rebase\".\n> \n> Additionally add support for multiple prepare-commit-msg hooks.\n> \n> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n> ---\n>  builtin/am.c                       | 28 ++++++---\n\nHaving read the patch subject I was surprised to see this touching\nbulitin/am.c\n\n>  sequencer.c                        | 96 +++++++++++++++++++-----------\n>  t/t5407-post-rewrite-hook.sh       | 15 +++++\n>  t/t7505-prepare-commit-msg-hook.sh |  9 +++\n>  4 files changed, 105 insertions(+), 43 deletions(-)\n> \n> diff --git a/builtin/am.c b/builtin/am.c\n> index 4fb107a9d1..303abc98c2 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -442,22 +442,32 @@ static int run_applypatch_msg_hook(struct am_state *state)\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> +\tstruct child_process proc;\n> +\tstruct string_list *hooks;\n> +\tstruct string_list_item *p;\n>  \tint ret;\n>  \n> -\tif (!hook)\n> +\thooks = find_hooks(\"post-rewrite\");\n> +\tif (!hooks)\n>  \t\treturn 0;\n>  \n> -\targv_array_push(&cp.args, hook);\n> -\targv_array_push(&cp.args, \"rebase\");\n> +\tfor_each_string_list_item(p, hooks) {\n> +\t\tchild_process_init(&proc);\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> +\t\targv_array_push(&cp.args, p->string);\n> +\t\targv_array_push(&cp.args, \"rebase\");\n>  \n> -\tret = run_command(&cp);\n> +\t\tcp.in = xopen(am_path(state, \"rewritten\"), O_RDONLY);\n> +\t\tcp.stdout_to_stderr = 1;\n> +\t\tcp.trace2_hook_name = \"post-rewrite\";\n>  \n> -\tclose(cp.in);\n> +\t\tret = run_command(&cp);\n> +\n> +\t\tclose(cp.in);\n> +\t\tif (ret)\n> +\t\t\tbreak;\n> +\t}\n> +\tfree_hooks(hooks);\n>  \treturn ret;\n>  }\n>  \n> diff --git a/sequencer.c b/sequencer.c\n> index 79a046d748..3a616ceba6 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -1088,33 +1088,69 @@ int update_head_with_reflog(const struct commit *old_head,\n>  static int run_rewrite_hook(const struct object_id *oldoid,\n>  \t\t\t    const struct object_id *newoid)\n>  {\n> -\tstruct child_process proc = CHILD_PROCESS_INIT;\n> +\tstruct child_process proc;\n> +\tstruct string_list *hooks;\n> +\tstruct string_list_item *p;\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> +\thooks = find_hooks(\"post-rewrite\");\n> +\tif (!hooks)\n>  \t\treturn 0;\n>  \n> -\targv[1] = \"amend\";\n> -\targv[2] = NULL;\n> +\tfor_each_string_list_item(p, hooks) {\n> +\t\tchild_process_init(&proc);\n>  \n> -\tproc.argv = argv;\n> -\tproc.in = -1;\n> -\tproc.stdout_to_stderr = 1;\n> -\tproc.trace2_hook_name = \"post-rewrite\";\n> +\t\targv[0] = p->string;\n> +\t\targv[1] = \"amend\";\n> +\t\targv[2] = NULL;\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> -\tsigchain_push(SIGPIPE, SIG_IGN);\n> -\twrite_in_full(proc.in, sb.buf, sb.len);\n> -\tclose(proc.in);\n> -\tstrbuf_release(&sb);\n> -\tsigchain_pop(SIGPIPE);\n> -\treturn finish_command(&proc);\n> +\t\tproc.argv = argv;\n> +\t\tproc.in = -1;\n> +\t\tproc.stdout_to_stderr = 1;\n> +\t\tproc.trace2_hook_name = \"post-rewrite\";\n> +\n> +\t\tcode = start_command(&proc);\n> +\t\tif (code)\n> +\t\t\treturn code;\n> +\t\tstrbuf_addf(&sb, \"%s %s\\n\", oid_to_hex(oldoid), oid_to_hex(newoid));\n> +\t\tsigchain_push(SIGPIPE, SIG_IGN);\n> +\t\twrite_in_full(proc.in, sb.buf, sb.len);\n> +\t\tclose(proc.in);\n> +\t\tstrbuf_release(&sb);\n> +\t\tsigchain_pop(SIGPIPE);\n> +\t\tcode = finish_command(&proc);\n> +\t\tif (code)\n> +\t\t\tbreak;\n> +\t}\n> +\tfree_hooks(hooks);\n> +\treturn code;\n> +}\n> +\n> +static void run_interactive_rewrite_hook(void)\n> +{\n> +\tstruct string_list *hooks;\n> +\tstruct string_list_item *p;\n> +\tstruct child_process child;\n> +\n> +\thooks = find_hooks(\"post-rewrite\");\n> +\tif (!hooks)\n> +\t\treturn;\n> +\n> +\tfor_each_string_list_item(p, hooks) {\n> +\t\tchild_process_init(&child);\n> +\n> +\t\tchild.in = open(rebase_path_rewritten_list(),\n> +\t\t\tO_RDONLY);\n> +\t\tchild.stdout_to_stderr = 1;\n> +\t\tchild.trace2_hook_name = \"post-rewrite\";\n> +\t\targv_array_push(&child.args, p->string);\n> +\t\targv_array_push(&child.args, \"rebase\");\n> +\t\tif (run_command(&child))\n> +\t\t\tbreak;\n> +\t}\n> +\tfree_hooks(hooks);\n>  }\n\nIf you're adding a function to do this it would be nice to use it from\nam.c as well rather than duplicating essentially the same code. Is there\nany way to use a helper to run all the hooks, rather than introducing a\nsimilar loop everywhere where we call a hook?\n\n>  void commit_post_rewrite(struct repository *r,\n> @@ -1326,6 +1362,7 @@ static int try_to_commit(struct repository *r,\n>  \tchar *amend_author = NULL;\n>  \tconst char *hook_commit = NULL;\n>  \tenum commit_msg_cleanup_mode cleanup;\n> +\tstruct string_list *hooks;\n>  \tint res = 0;\n>  \n>  \tif (parse_head(r, &current_head))\n> @@ -1369,7 +1406,10 @@ static int try_to_commit(struct repository *r,\n>  \t\tgoto out;\n>  \t}\n>  \n> -\tif (find_hook(\"prepare-commit-msg\")) {\n> +\thooks = find_hooks(\"prepare-commit-msg\");\n> +\tif (hooks) {\n> +\t\tfree_hooks(hooks);\n\nI think you forgot to update run_prepare_commit_msg_hook(), it should\nprobably be passed this list now. It might be outside the scope of this\nseries but unifying this with builtin/commit.c\n\n> +\n>  \t\tres = run_prepare_commit_msg_hook(r, msg, hook_commit);\n>  \t\tif (res)\n>  \t\t\tgoto out;\n> @@ -3771,8 +3811,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> @@ -3782,18 +3820,8 @@ 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\trun_interactive_rewrite_hook();\n>  \t\t}\n>  \t\tapply_autostash(opts);\n>  \n> diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh\n> index 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\n> diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\n> index ba8bd1b514..5b83f037b5 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> +commit_command () {\n> +\techo \"$1\" >>file &&\n> +\tgit add file &&\n> +\tgit commit -m \"$1\"\n> +}\n> +\n> +test_multiple_hooks prepare-commit-msg commit_command\n\nIt's not clear to me that this is testing the sequencer\n\nBest Wishes\n\nPhillip\n\n> +\n>  test_done\n> \n\n"},{"id":"374381","messageId":"551bcdfa-d098-48a8-8712-05b7e1516bd8@gmail.com","threadId":"50980","inReplyTo":"87tvensu1a.fsf@evledraar.gmail.com","subject":"Re: [PATCH 0/5] Multiple hook support","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-04-24T09:55:37Z","receivedAt":"2019-04-24T09:55:42Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 24/04/2019 09:10, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Apr 24 2019, brian m. carlson wrote:\n> \n>> Oftentimes, people want to use multiple of the same kind of hook. This\n>> may be because software or a project they use requires a given hook, but\n>> they would also like to have a custom hook, or because they're using\n>> multiple pieces of software that both require involvement with the same\n>> hook.\n>>\n>> This series introduces support for multiple hooks by using a \".d\"\n>> directory. For example, instead of using a single\n>> \".git/hooks/post-checkout\" file, you'd use multiple files within\n>> \".git/hooks/post-checkout.d\". This is the standard Unix paradigm for\n>> multiple files and should be familiar to most people. Hooks are executed\n>> in sorted order.\n> \n> I think it's interesting if people can chime in with current in-the-wild\n> implementations of this.\n> \n> I know GitLab's the best, not because I was in any way involved in it,\n> I've just dealt with writing hooks for it:\n> https://docs.gitlab.com/ee/administration/custom_hooks.html#chained-hooks-support\n> \n> There:\n> \n>  1. The instance has a 'hooks' dir in the .git repo that's a symlink to\n>     /my/global/hooks. They could also use a /etc/gitconfig\n>     core.hooksPath for this part, but whatever.\n> \n>  2. That /my/global/hooks has e.g. a /my/global/hooks/pre-receive that\n>     git itself runs, which is a trampoline script that runs all over the\n>     place and executes global/per-project hooks (which live in\n>     .git/custom_hooks/).\n\nHaving global and local hooks could be useful. I've got some tooling\nthat rewrites multiple branches and has hooks to prevent committing and\nrebasing in worktrees where the branch is being rewritten by a rewrite\nin another worktree. I'm using core.hooksPath at the moment, but if I\never work with a project that uses its own hooks I'll have to add some\nsort of trampoline like gitlab.\n\nBest Wishes\n\nPhillip\n\n> \n>  3. \"The hooks of the same type are executed in order and execution\n>      stops on the first script exiting with a non-zero value.\"\n> \n> I wonder if the eventual goal of this facility would be to get such\n> users on board with using git's native feature for this. This series is\n> most of the way there for GitLab's case, but not quite.\n> \n>> To preserve backwards compatibility, we don't run the hooks in the \".d\"\n>> directory if the single file is a valid hook (i.e. it exists and is\n>> executable). This is because some people already have multiple hook\n>> scripts configured, and if we ran them both, we'd run the hooks twice.\n>> This would be bad for e.g. the prepare-commit-msg hook. This is also the\n>> least surprising behavior.\n>>\n>> We check each hook for its exit status, even if the hook normally\n>> ignores exit status, and if it fails, we abort executing further hooks.\n>> This provides an easy way to reason about what the exit status is when a\n>> hook fails; we need not consider how to handle multiple failing hooks.\n>> It's also consistent among all hooks, whether they care about exit\n>> status or not.\n> \n> Others have replied to this already. I linked to the 2016 discussion of\n> my RFC for this in\n> https://public-inbox.org/git/87wojjsv9p.fsf@evledraar.gmail.com where I\n> made the same choice. Some points on that:\n> \n>  * There was the mention of \"but what if someone wants to run them all\",\n>    e.g. for logging to N logging systems where one fails, that's been\n>    brought up again this time around by others.\n> \n>  * The case I find more interesting is the ability to run the hooks in\n>    parallel. Saying \"glob order and exit on first fail\" categorically\n>    closes the door to that.\n> \n>> I've talked with some people about this approach, and they've indicated\n>> they would prefer a configuration-based approach. I've tried to make the\n>> series such that it can be replaced with such an approach if that's the\n>> decision we make. It should be easy enough to simply replace find_hooks\n>> with an appropriate implementation and update the test framework.\n> \n> I think whatever opinions we all have on the current implementation it's\n> OK to get this in in *some* form and just made it configurable or\n> whatever later.\n> \n> Most of the work is the ability to run N hooks, how exactly that happens\n> can be tweaked later, and if this series lands and someone isn't 100%\n> happy with the semantics they're no worse off than they are now.\n> \n> I.e. we could get something like this in its current form, and later have:\n> \n>     core.hooksOrder = glob | random\n>     core.hooksHaltOnError = true | never\n> \n> Where we'd say that what this series does is core.hooksOrder=glob and\n> core.hooksHaltOnError=true, but that e.g. parallel \"run them all\" could\n> be done in the future with core.hooksOrder=random &\n> core.hooksHaltOnError=never\n> \n\n"},{"id":"374414","messageId":"CAGyf7-E2tf7hymOOUSAO6325NyyADWmmMjSRx87-UKVhQd-ufw@mail.gmail.com","threadId":"50980","inReplyTo":"87tvensu1a.fsf@evledraar.gmail.com","subject":"Re: [PATCH 0/5] Multiple hook support","fromName":"Bryan Turner","fromEmail":"bturner@atlassian.com","sentAt":"2019-04-24T18:29:09Z","receivedAt":"2019-04-24T18:29:22Z","isPatch":true,"sender":{"key":"bturner@atlassian.com","avatar":"https://gravatar.com/avatar/16bcf3167981c1ef7c804e502642366d888a35b0d0b0a4ca01fdc442aa1acb1e?d=mp&s=160"},"body":"On Wed, Apr 24, 2019 at 1:10 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n> On Wed, Apr 24 2019, brian m. carlson wrote:\n>\n> > Oftentimes, people want to use multiple of the same kind of hook. This\n> > may be because software or a project they use requires a given hook, but\n> > they would also like to have a custom hook, or because they're using\n> > multiple pieces of software that both require involvement with the same\n> > hook.\n> >\n> > This series introduces support for multiple hooks by using a \".d\"\n> > directory. For example, instead of using a single\n> > \".git/hooks/post-checkout\" file, you'd use multiple files within\n> > \".git/hooks/post-checkout.d\". This is the standard Unix paradigm for\n> > multiple files and should be familiar to most people. Hooks are executed\n> > in sorted order.\n>\n> I think it's interesting if people can chime in with current in-the-wild\n> implementations of this.\n>\n\nBitbucket Server also uses trampoline scripts for pre-receive and\npost-receive that loop over scripts in matching *.d directories and\nrun them.\n\n* It passes --template to \"git init\" (new repositories) and \"git\nclone\" (forks) to have Git copy hook structure into place as part of\ncreating the repository.\n* To avoid looping over thousands or tens of thousands of repositories\nif those scripts need to change, the scripts put in place (both at the\npre-receive/post-receive level and for *.d/20_bitbucket_callback) are\nessentially placeholders that use environment variables exported when\nrunning \"git http-backend\" and \"git receive-pack\" to delegate to\nhelper scripts that actually do the work.\n* The helper scripts only exist in one place, so they can easily be\nupdated if they need to change.\n* When looping over *.d scripts, the helper script stops after the\nfirst non-zero return.\n\nOn Tue, Apr 23, 2019 at 7:41 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> One downside is that the transition from the old to the new world\n> order becomes a bigger deal.  You could have been using a precursor\n> of this series implemented entirely as a shell script\n>\n>         $ cat >.git/pre-commit <<\\EOF\n>         #!/bin/sh\n>         for x in .git/pre-commit.d/*\n>         do\n>                 \"$x\" \"$@\" || exit 1\n>         done\n>         exit 0\n>         EOF\n>         $ chmod +x .git/pre-commit\n>\n> and with your approach, the user can keep using that set-up and then\n> simply remove that single file once all the Git executables the user\n> uses are natively aware of the multi-hook setup.  With \"either a\n> file or directory\" approach, the transition would be removing the\n> file *and* renaming pre-commit.d directory to pre-commit.\n>\n\nThis would be true for Bitbucket Server, since its trampoline scripts\nare really simple: It could just delete them and let Git loop over the\nexisting *.d directories itself, so the current approach offers a\nreally straightforward upgrade path.\n\nOf course, since Bitbucket Server supports a range of Git versions on\nthe server, and for compatibility reasons the minimum is only raised\nin major version releases, it would likely be quite some time before\nthat could happen; the minimum supported Git version would need this\nseries to have landed in or before it. That's a big part of why\nBitbucket Server doesn't use core.hooksPath; it's too new. (Bitbucket\nServer 6.0 raised the minimum Git version on the server to 2.11.0, so\nit's the first major series that could use core.hooksPath; Bitbucket\nServer 5.x supported back to Git 2.2.0, which doesn't have it.)\n\nBest regards,\nBryan Turner\n"},{"id":"374416","messageId":"8f79d251-58d9-f63b-7171-7f1fbd11c6f9@kdbg.org","threadId":"50980","inReplyTo":"xmqqo94w2l3k.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/5] run-command: add preliminary support for multiple hooks","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-04-24T18:48:32Z","receivedAt":"2019-04-24T18:48:37Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 24.04.19 um 04:27 schrieb Junio C Hamano:\n> \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n>> +static int has_hook(struct strbuf *path, int strip)\n>> +{\n>> +\tif (access(path->buf, X_OK) < 0) {\n> \n> Does \".git/post-commit\" that is not an executable exist?\n> \n> It was perfectly fine for find_hook() to say \"there is no hook for\n> post-commit\" in the old world in such a case, because the\n> unexecutable file it found is not going to be run anyway.\n> \n> But it is not clear if has_hook(), that affects \"there is no single\n> hook file for post-commit, so let's look at post-commit.d\" decision\n> made by find_hooks(), should behave that way.  It somehow feels more\n> intuitive if a post-commit file that is not executable, by merely\n> existing, stops post-commit.d directory from being scanned, at least\n> to me.\n\nFurthermore, basing a decision on whether a file is executable won't\nwork on Windows as intended. So, it is better to aim for an existence check.\n\n-- Hannes\n"},{"id":"374427","messageId":"20190424223240.GI6316@genre.crustytoothpaste.net","threadId":"50980","inReplyTo":"xmqqo94w2l3k.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/5] run-command: add preliminary support for multiple hooks","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-04-24T22:32:40Z","receivedAt":"2019-04-24T22:32:49Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Apr 24, 2019 at 11:27:59AM +0900, Junio C Hamano wrote:\n> \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n> > diff --git a/run-command.c b/run-command.c\n> > index 3449db319b..669af5ebc7 100644\n> > --- a/run-command.c\n> > +++ b/run-command.c\n> > @@ -1308,58 +1308,137 @@ int async_with_fork(void)\n> >  #endif\n> >  }\n> >  \n> > +static int has_hook(struct strbuf *path, int strip)\n> > +{\n> > +\tif (access(path->buf, X_OK) < 0) {\n> \n> Does \".git/post-commit\" that is not an executable exist?\n> \n> It was perfectly fine for find_hook() to say \"there is no hook for\n> post-commit\" in the old world in such a case, because the\n> unexecutable file it found is not going to be run anyway.\n> \n> But it is not clear if has_hook(), that affects \"there is no single\n> hook file for post-commit, so let's look at post-commit.d\" decision\n> made by find_hooks(), should behave that way.  It somehow feels more\n> intuitive if a post-commit file that is not executable, by merely\n> existing, stops post-commit.d directory from being scanned, at least\n> to me.\n\nUnfortunately, we used to have Git versions that wrote an non-executable\nfile in place instead of a \".sample\" file when initializing the\nrepository. We have a warning for that, but I have many repositories\nthat have lived long enough that they're still affected (and I've turned\noff the warning for that reason). I feel like we'll be creating\nsurprising behavior for long-term users.\n\nAlso, if someone is using an old manager script that uses the \".d\"\ndirectory or some other workaround, it's easy enough for them to simply\nclear the executable bit; they need not delete it, and can restore it at\nany time simply by toggling the bit.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"374428","messageId":"20190424224626.GJ6316@genre.crustytoothpaste.net","threadId":"50980","inReplyTo":"ed0312f5-d174-76e0-fea7-5de93f5840b8@gmail.com","subject":"Re: [PATCH 3/5] sequencer: add support for multiple hooks","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-04-24T22:46:27Z","receivedAt":"2019-04-24T22:46:35Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Apr 24, 2019 at 10:51:56AM +0100, Phillip Wood wrote:\n> On 24/04/2019 01:49, brian m. carlson wrote:\n> > Add support for multiple post-rewrite hooks, both for \"git commit\n> > --amend\" and \"git rebase\".\n> > \n> > Additionally add support for multiple prepare-commit-msg hooks.\n> > \n> > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n> > ---\n> >  builtin/am.c                       | 28 ++++++---\n> \n> Having read the patch subject I was surprised to see this touching\n> bulitin/am.c\n\nI can rewrite the subject. Unfortunately, the same hook for rebase goes\nthrough two wildly different modules, so in order to completely convert\nthe post-rewrite hook, I have to update both at the same time.\n\n> > +static void run_interactive_rewrite_hook(void)\n> > +{\n> > +\tstruct string_list *hooks;\n> > +\tstruct string_list_item *p;\n> > +\tstruct child_process child;\n> > +\n> > +\thooks = find_hooks(\"post-rewrite\");\n> > +\tif (!hooks)\n> > +\t\treturn;\n> > +\n> > +\tfor_each_string_list_item(p, hooks) {\n> > +\t\tchild_process_init(&child);\n> > +\n> > +\t\tchild.in = open(rebase_path_rewritten_list(),\n> > +\t\t\tO_RDONLY);\n> > +\t\tchild.stdout_to_stderr = 1;\n> > +\t\tchild.trace2_hook_name = \"post-rewrite\";\n> > +\t\targv_array_push(&child.args, p->string);\n> > +\t\targv_array_push(&child.args, \"rebase\");\n> > +\t\tif (run_command(&child))\n> > +\t\t\tbreak;\n> > +\t}\n> > +\tfree_hooks(hooks);\n> >  }\n> \n> If you're adding a function to do this it would be nice to use it from\n> am.c as well rather than duplicating essentially the same code. Is there\n> any way to use a helper to run all the hooks, rather than introducing a\n> similar loop everywhere where we call a hook?\n\nIt's becoming clear to me that a helper is probably going to be cleaner,\nso I'll add one in for v2.\n\n> >  void commit_post_rewrite(struct repository *r,\n> > @@ -1326,6 +1362,7 @@ static int try_to_commit(struct repository *r,\n> >  \tchar *amend_author = NULL;\n> >  \tconst char *hook_commit = NULL;\n> >  \tenum commit_msg_cleanup_mode cleanup;\n> > +\tstruct string_list *hooks;\n> >  \tint res = 0;\n> >  \n> >  \tif (parse_head(r, &current_head))\n> > @@ -1369,7 +1406,10 @@ static int try_to_commit(struct repository *r,\n> >  \t\tgoto out;\n> >  \t}\n> >  \n> > -\tif (find_hook(\"prepare-commit-msg\")) {\n> > +\thooks = find_hooks(\"prepare-commit-msg\");\n> > +\tif (hooks) {\n> > +\t\tfree_hooks(hooks);\n> \n> I think you forgot to update run_prepare_commit_msg_hook(), it should\n> probably be passed this list now. It might be outside the scope of this\n> series but unifying this with builtin/commit.c\n\nrun_prepare_commit_msg_hook calls run_hook_le, which looks up that value\nitself. It's unfortunate that we have to do it twice, but we need to\nknow whether we need to re-read the commit msg or not. I can explain\nthis further in the commit message.\n\n> > diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\n> > index ba8bd1b514..5b83f037b5 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> > +commit_command () {\n> > +\techo \"$1\" >>file &&\n> > +\tgit add file &&\n> > +\tgit commit -m \"$1\"\n> > +}\n> > +\n> > +test_multiple_hooks prepare-commit-msg commit_command\n> \n> It's not clear to me that this is testing the sequencer\n\nYou're right. I need to adopt a different approach here.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"374437","messageId":"20190424224945.GK6316@genre.crustytoothpaste.net","threadId":"50980","inReplyTo":"CACsJy8CKwiNknEkEH_-DxuXYiBmpYvfdRAZvmSJCcqko_OYnTw@mail.gmail.com","subject":"Re: [PATCH 0/5] Multiple hook support","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-04-24T22:49:45Z","receivedAt":"2019-04-24T22:49:57Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Apr 24, 2019 at 04:49:54PM +0700, Duy Nguyen wrote:\n> Heh you beat me to it. My config-hooks branch [1] has not been updated\n> for half a year. I only skimmed through quickly so no useful comments,\n> but I went with a slightly different design, introducing\n> for_each_hook() instead (see run-command.[ch] in the last patch).\n\nI figured this was a common enough request that people wanted that it\nwas worth doing. I'll take a look at your series and I may steal some\ncode.\n\n> Back then I wasn't sure what would be the right policy for running\n> multiple hooks. By keeping the hook loop in one place, we could change\n> that policy much easier later (e.g. continue anyway in case of\n> failure, or stop as soon as you see a problem, or even run some\n> special hooks at the end even when there is an error earlier).\n\nYeah, I think from feedback elsewhere, I'm going to roll it into a\nsingle function. The nature of callbacks in C makes it potentially\nmessy, but I'll try to see if there's a tidy way to do it.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"374438","messageId":"20190424230744.GL6316@genre.crustytoothpaste.net","threadId":"50980","inReplyTo":"20190424023438.GE98980@google.com","subject":"Re: [PATCH 0/5] Multiple hook support","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-04-24T23:07:44Z","receivedAt":"2019-04-24T23:07:54Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, Apr 23, 2019 at 07:34:38PM -0700, Jonathan Nieder wrote:\n> Hi,\n> \n> brian m. carlson wrote:\n> \n> > I've talked with some people about this approach, and they've indicated\n> > they would prefer a configuration-based approach.\n> \n> I would, too, mostly because that reduces the problem of securing\n> hooks to securing configuration.  See\n> https://public-inbox.org/git/20171002234517.GV19555@aiede.mtv.corp.google.com/\n> for more on this subject.\n\nI know this is a common issue, but fixing it is a non-goal for this\nseries. Anything we do here is going to have to be backwards compatible,\nso we can't make any changes to the security model.\n\n> Solving (1) without (2) feels like a bit of a missed opportunity to\n> me.  Ideally, what I would like is\n> \n>    i. A central registry of trustworthy Git hooks that can be upgraded\n>       using the system package manager to address (2).  Perhaps just\n>       git-hook-* commands on the $PATH.\n> \n>   ii. Instead of putting hooks in .git/hooks, put a list of hooks to\n>       run for each event in .git/config.\n\nThe problem I had with this when discussing it was that our\nconfiguration system lacks a good way to control inheritance from outer\nfiles. I recently was working with a system-wide gitconfig file that\nreferred to files I didn't have, and my Git installation was subtly\nbroken in a variety of ways.\n\nIf I have a system-wide hook to run for company code, but I have a\ncheckout for my personal dotfiles on my machine where I don't want to\nrun that hook, our configuration lacks a way for me to disable that\nsystem-wide configuration. However, using our current system, I can\noverride core.hooksPath in this case and everything works fine.\n\nI mentioned this for completeness, and because I hope that some of those\npeople will get some time to chime in here, but I think without that\nfeature, we end up with a worse experience than we have now.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"374440","messageId":"20190424232631.GA186980@google.com","threadId":"50980","inReplyTo":"20190424230744.GL6316@genre.crustytoothpaste.net","subject":"Re: [PATCH 0/5] Multiple hook support","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-04-24T23:26:31Z","receivedAt":"2019-04-24T23:26:36Z","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 Tue, Apr 23, 2019 at 07:34:38PM -0700, Jonathan Nieder wrote:\n>> brian m. carlson wrote:\n\n>>> I've talked with some people about this approach, and they've indicated\n>>> they would prefer a configuration-based approach.\n>>\n>> I would, too, mostly because that reduces the problem of securing\n>> hooks to securing configuration.  See\n>> https://public-inbox.org/git/20171002234517.GV19555@aiede.mtv.corp.google.com/\n>> for more on this subject.\n>\n> I know this is a common issue, but fixing it is a non-goal for this\n> series. Anything we do here is going to have to be backwards compatible,\n> so we can't make any changes to the security model.\n\nI think it's worth bringing up because we should have some idea of where\nwe want to head.\n\nI think the backward compatibility part is actually one of the easier\naspects of this one.  We don't have to change the security model right\naway because there are similar places to exploit like core.pager.  To\naddress that, we need a notion of configuration that individual repos\nand worktrees can't override, and using such a configuration item, we\ncan provide a way to opt in to the new security model.  That provides\na smooth path forward.\n\n[...]\n>> Solving (1) without (2) feels like a bit of a missed opportunity to\n>> me.  Ideally, what I would like is\n>>\n>>    i. A central registry of trustworthy Git hooks that can be upgraded\n>>       using the system package manager to address (2).  Perhaps just\n>>       git-hook-* commands on the $PATH.\n>>\n>>   ii. Instead of putting hooks in .git/hooks, put a list of hooks to\n>>       run for each event in .git/config.\n>\n> The problem I had with this when discussing it was that our\n> configuration system lacks a good way to control inheritance from outer\n> files.\n\nThe standard approach in lists defined in Git configuration is for\nassigning an empty item to clear / restart the list.  See\nhttp.extraHeader for an example.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"374441","messageId":"20190424234045.GN6316@genre.crustytoothpaste.net","threadId":"50980","inReplyTo":"CACsJy8CKwiNknEkEH_-DxuXYiBmpYvfdRAZvmSJCcqko_OYnTw@mail.gmail.com","subject":"Re: [PATCH 0/5] Multiple hook support","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-04-24T23:40:45Z","receivedAt":"2019-04-24T23:40:53Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Apr 24, 2019 at 04:49:54PM +0700, Duy Nguyen wrote:\n> Heh you beat me to it. My config-hooks branch [1] has not been updated\n> for half a year. I only skimmed through quickly so no useful comments,\n> but I went with a slightly different design, introducing\n> for_each_hook() instead (see run-command.[ch] in the last patch).\n\nIf I grab code from this series, may I apply your sign-off to it?\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"374442","messageId":"CACsJy8CS-9339TFJHkwbK17mhS85g2KYg0ysr+8PxupSqps_Tg@mail.gmail.com","threadId":"50980","inReplyTo":"20190424234045.GN6316@genre.crustytoothpaste.net","subject":"Re: [PATCH 0/5] Multiple hook support","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-04-25T00:08:15Z","receivedAt":"2019-04-25T00:08:50Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Apr 25, 2019 at 6:41 AM brian m. carlson\n<sandals@crustytoothpaste.net> wrote:\n>\n> On Wed, Apr 24, 2019 at 04:49:54PM +0700, Duy Nguyen wrote:\n> > Heh you beat me to it. My config-hooks branch [1] has not been updated\n> > for half a year. I only skimmed through quickly so no useful comments,\n> > but I went with a slightly different design, introducing\n> > for_each_hook() instead (see run-command.[ch] in the last patch).\n>\n> If I grab code from this series, may I apply your sign-off to it?\n\nYes\n-- \nDuy\n"},{"id":"374445","messageId":"xmqqo94uzyxa.fsf@gitster-ct.c.googlers.com","threadId":"50980","inReplyTo":"8f79d251-58d9-f63b-7171-7f1fbd11c6f9@kdbg.org","subject":"Re: [PATCH 1/5] run-command: add preliminary support for multiple hooks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-25T00:55:13Z","receivedAt":"2019-04-25T00:55:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> Furthermore, basing a decision on whether a file is executable won't\n> work on Windows as intended. So, it is better to aim for an existence check.\n\nThat is a good point.\n\nSo it may be OK for \"do we have a single hook script for this hook\nname?\" to say \"no\" when the path exists but not executable on\nPOSIXPERM systems, but it is better to say \"yes\" for consistency\nacross platforms (I think that is one of the reasons why we use\n.sample suffix these days).\n\nAnd for the same reason, for the purpose of deciding \"because we do\nnot have a single hook script, let's peek into .d directory\nourselves\", mere presence of the file with that name, regardless of\nthe executable bit, should signal that we should not handle the .d\ndirectory.\n\nIOW, you think access(X_OK) should be more like access(F_OK)?\n\n"},{"id":"374460","messageId":"87lfzys9t1.fsf@evledraar.gmail.com","threadId":"50980","inReplyTo":"xmqqo94uzyxa.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/5] run-command: add preliminary support for multiple hooks","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-04-25T09:39:38Z","receivedAt":"2019-04-25T09:39:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Apr 25 2019, Junio C Hamano wrote:\n\n> Johannes Sixt <j6t@kdbg.org> writes:\n>\n>> Furthermore, basing a decision on whether a file is executable won't\n>> work on Windows as intended. So, it is better to aim for an existence check.\n>\n> That is a good point.\n>\n> So it may be OK for \"do we have a single hook script for this hook\n> name?\" to say \"no\" when the path exists but not executable on\n> POSIXPERM systems, but it is better to say \"yes\" for consistency\n> across platforms (I think that is one of the reasons why we use\n> .sample suffix these days).\n>\n> And for the same reason, for the purpose of deciding \"because we do\n> not have a single hook script, let's peek into .d directory\n> ourselves\", mere presence of the file with that name, regardless of\n> the executable bit, should signal that we should not handle the .d\n> directory.\n>\n> IOW, you think access(X_OK) should be more like access(F_OK)?\n\nTo me this is another point in favor of bypassing this problem entirely\nand adopting the semantics GitLab (and it seems others) use. I.e. in\norder execute:\n\n    .git/hooks/pre-receive .git/hooks/pre-receive.d/*\n\nInstead of going further down this avenue of:\n\n    if exists_or_executable_or_whatever .git/hooks/pre-receive\n    then\n        .git/hooks/pre-receive\n    else\n        for hook in .git/hooks/pre-receive.d/*\n        do\n            $hook\n        done\n    fi\n\nIt also:\n\n 1) Makes it easier for users to experiment with more granular hooks if\n    they have one big pre-receive hook by adding pre-receive.d/* hooks\n    without having to move their existing pre-receive to\n    pre-receive.d/000-existing hook (which will be incompatible across\n    git versions!).\n\n 2) Is compatible with any existing trampoline scripts you might want to\n    migrate from that *don't* use pre-receive.d/*, e.g. one script in\n    our infrastructure (that I didn't write) does:\n\n        my ($hook_phase, $dir) = fileparse($0);\n        my $exit = 0;\n        my @hooks = glob(\"${dir}${hook_phase}-*\");\n        for my $hook (@hooks) {\n            next unless -x $hook;\n            $exit |= system $hook;\n        }\n        exit ($exit >> 8);\n\n   I.e. you have a \".git/hooks/pre-receive\" trampoline and it runs\n   \".git/hooks/pre-receive-*\" scripts.\n\nIt occurs to me that we might want to make things configurable for the\n#2 case. I.e. have a core.hooksDSuffix=\".d/\" by default, but you could\nalso set it to \"-\". So we'd then construct a glob of either\n\"pre-receive.d/*\" or \"pre-receive-*\" from that.\n"},{"id":"374486","messageId":"xmqqwojiwgd4.fsf@gitster-ct.c.googlers.com","threadId":"50980","inReplyTo":"87lfzys9t1.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/5] run-command: add preliminary support for multiple hooks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-25T10:04:23Z","receivedAt":"2019-04-25T10:04:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> To me this is another point in favor of bypassing this problem entirely\n> and adopting the semantics GitLab (and it seems others) use. I.e. in\n> order execute:\n>\n>     .git/hooks/pre-receive .git/hooks/pre-receive.d/*\n\nBut isn't that exactly what Brian wanted to avoid?  Consider those\nwho run the current version of Git (which considers pre-receive.d/\nis just a floating cruft Git does not care), with their own\nimplementation of pre-receive hook that goes over the contents of\npre-receive.d/ directory and executes each in order.  If a new\nversion of Git you release runs pre-receive followed by each of the\nfiles in pre-receive.d/ directory, especially because you designed\nto use *.d convention to match existing practice, doesn't your new\nversion of Git end up running the scripts in *.d directory twice?\n"},{"id":"374487","messageId":"87k1fis8gq.fsf@evledraar.gmail.com","threadId":"50980","inReplyTo":"20190424230744.GL6316@genre.crustytoothpaste.net","subject":"How to undo previously set configuration? (again)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-04-25T10:08:37Z","receivedAt":"2019-04-25T10:08:42Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Apr 25 2019, brian m. carlson wrote:\n\n> On Tue, Apr 23, 2019 at 07:34:38PM -0700, Jonathan Nieder wrote:\n>> Hi,\n>>\n>> brian m. carlson wrote:\n>>\n>> > I've talked with some people about this approach, and they've indicated\n>> > they would prefer a configuration-based approach.\n>>\n>> I would, too, mostly because that reduces the problem of securing\n>> hooks to securing configuration.  See\n>> https://public-inbox.org/git/20171002234517.GV19555@aiede.mtv.corp.google.com/\n>> for more on this subject.\n>\n> I know this is a common issue, but fixing it is a non-goal for this\n> series. Anything we do here is going to have to be backwards compatible,\n> so we can't make any changes to the security model.\n\nAgreed on the problem, and on the non-goal for solving it as part of\nthis series.\n\n>> Solving (1) without (2) feels like a bit of a missed opportunity to\n>> me.  Ideally, what I would like is\n>>\n>>    i. A central registry of trustworthy Git hooks that can be upgraded\n>>       using the system package manager to address (2).  Perhaps just\n>>       git-hook-* commands on the $PATH.\n>>\n>>   ii. Instead of putting hooks in .git/hooks, put a list of hooks to\n>>       run for each event in .git/config.\n>\n> The problem I had with this when discussing it was that our\n> configuration system lacks a good way to control inheritance from outer\n> files. I recently was working with a system-wide gitconfig file that\n> referred to files I didn't have, and my Git installation was subtly\n> broken in a variety of ways.\n>\n> If I have a system-wide hook to run for company code, but I have a\n> checkout for my personal dotfiles on my machine where I don't want to\n> run that hook, our configuration lacks a way for me to disable that\n> system-wide configuration. However, using our current system, I can\n> override core.hooksPath in this case and everything works fine.\n>\n> I mentioned this for completeness, and because I hope that some of those\n> people will get some time to chime in here, but I think without that\n> feature, we end up with a worse experience than we have now.\n\nI sent a proposal for this last year \"How to undo previously set\nconfiguration?\":\nhttps://public-inbox.org/git/874lkq11ug.fsf@evledraar.gmail.com/\n\nObviously the main bottleneck is someone like me working on patching it,\nbut in this case it would be very useful if those who are interested in\nthis could look that proposal over and bikeshed it / point out issues I\nmay have missed, i.e. \"no, this categorically won't work with this\nproposed syntax due to XYZ you haven't thought of...\".\n\nBecause we don't have some general config facility for this it keeps\ncoming up, and various existing/proposed options have their own little\ncustom hacks for doing it, e.g. this for Barret Rhoden's proposed \"blame\nskip commits\" feature\nhttps://public-inbox.org/git/878swhfzxb.fsf@evledraar.gmail.com/\n(b.t.w. I *meant* /dev/null in that E-Mail, but due to PEBCAK wrote\n/dev/zero).\n"},{"id":"374492","messageId":"CACsJy8DSvEPfqJVGBL=G147B-mqoXd-XDeNK7jQZLQgRftWRpQ@mail.gmail.com","threadId":"50980","inReplyTo":"87k1fis8gq.fsf@evledraar.gmail.com","subject":"Re: How to undo previously set configuration? (again)","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-04-25T10:43:21Z","receivedAt":"2019-04-25T10:43:49Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Apr 25, 2019 at 5:08 PM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n> >> Solving (1) without (2) feels like a bit of a missed opportunity to\n> >> me.  Ideally, what I would like is\n> >>\n> >>    i. A central registry of trustworthy Git hooks that can be upgraded\n> >>       using the system package manager to address (2).  Perhaps just\n> >>       git-hook-* commands on the $PATH.\n> >>\n> >>   ii. Instead of putting hooks in .git/hooks, put a list of hooks to\n> >>       run for each event in .git/config.\n> >\n> > The problem I had with this when discussing it was that our\n> > configuration system lacks a good way to control inheritance from outer\n> > files. I recently was working with a system-wide gitconfig file that\n> > referred to files I didn't have, and my Git installation was subtly\n> > broken in a variety of ways.\n> >\n> > If I have a system-wide hook to run for company code, but I have a\n> > checkout for my personal dotfiles on my machine where I don't want to\n> > run that hook, our configuration lacks a way for me to disable that\n> > system-wide configuration. However, using our current system, I can\n> > override core.hooksPath in this case and everything works fine.\n> >\n> > I mentioned this for completeness, and because I hope that some of those\n> > people will get some time to chime in here, but I think without that\n> > feature, we end up with a worse experience than we have now.\n>\n> I sent a proposal for this last year \"How to undo previously set\n> configuration?\":\n> https://public-inbox.org/git/874lkq11ug.fsf@evledraar.gmail.com/\n\nWhile reading that mail, it occurs to me that perhaps we can reuse the\n.gitignore idea.\n\nInstead of having a list of untracked files, we have a list of config\nkeys. Instead of having .gitignore files associated to different\ndirectories to apply the rules to those dirs only, we have ignore\nrules that should apply on certain config files (probably based on\npath).\n\nA few differences from your reject/accept/priority example:\n\n- we don't redefine priority, inheritance rules apply the same way\n- reject/accept is handled the same way as positive/negative ignore\nrules. If we're lucky, we could even reuse the exclude code.\n- instead of special section names like\n\n    [config \"section\"]\n\nwe have something more like\n\n    [config \"/this/path\"] # (or pattern)\n\nthis lets us handle even other config files included by [include] or [includeIf]\n\nSo, some examples\n\n[exclude]            # exclude from all inherited files\n    key = core.*     # exclude core.*\n    key = !core.bar  # but keep core.bar\n\n[excludeIf \"path:/etc/config\"] # rules apply for only this file\n   key = ...\n\n[excludeIf \"glob:/home/*\"]     # rules apply for these config paths\n   key = ...\n\n[excludeIf \"system\"]           # special names for convenience maybe\n   key = ...\n\n> Obviously the main bottleneck is someone like me working on patching it,\n\nYes, manpower is always the problem.\n\n> but in this case it would be very useful if those who are interested in\n> this could look that proposal over and bikeshed it / point out issues I\n> may have missed, i.e. \"no, this categorically won't work with this\n> proposed syntax due to XYZ you haven't thought of...\".\n-- \nDuy\n"},{"id":"374495","messageId":"87imv2s3dz.fsf@evledraar.booking.com","threadId":"50980","inReplyTo":"CACsJy8DSvEPfqJVGBL=G147B-mqoXd-XDeNK7jQZLQgRftWRpQ@mail.gmail.com","subject":"Re: How to undo previously set configuration? (again)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-04-25T11:58:16Z","receivedAt":"2019-04-25T11:58:22Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Apr 25 2019, Duy Nguyen wrote:\n\n> On Thu, Apr 25, 2019 at 5:08 PM Ævar Arnfjörð Bjarmason\n> <avarab@gmail.com> wrote:\n>> >> Solving (1) without (2) feels like a bit of a missed opportunity to\n>> >> me.  Ideally, what I would like is\n>> >>\n>> >>    i. A central registry of trustworthy Git hooks that can be upgraded\n>> >>       using the system package manager to address (2).  Perhaps just\n>> >>       git-hook-* commands on the $PATH.\n>> >>\n>> >>   ii. Instead of putting hooks in .git/hooks, put a list of hooks to\n>> >>       run for each event in .git/config.\n>> >\n>> > The problem I had with this when discussing it was that our\n>> > configuration system lacks a good way to control inheritance from outer\n>> > files. I recently was working with a system-wide gitconfig file that\n>> > referred to files I didn't have, and my Git installation was subtly\n>> > broken in a variety of ways.\n>> >\n>> > If I have a system-wide hook to run for company code, but I have a\n>> > checkout for my personal dotfiles on my machine where I don't want to\n>> > run that hook, our configuration lacks a way for me to disable that\n>> > system-wide configuration. However, using our current system, I can\n>> > override core.hooksPath in this case and everything works fine.\n>> >\n>> > I mentioned this for completeness, and because I hope that some of those\n>> > people will get some time to chime in here, but I think without that\n>> > feature, we end up with a worse experience than we have now.\n>>\n>> I sent a proposal for this last year \"How to undo previously set\n>> configuration?\":\n>> https://public-inbox.org/git/874lkq11ug.fsf@evledraar.gmail.com/\n>\n> While reading that mail, it occurs to me that perhaps we can reuse the\n> .gitignore idea.\n>\n> Instead of having a list of untracked files, we have a list of config\n> keys. Instead of having .gitignore files associated to different\n> directories to apply the rules to those dirs only, we have ignore\n> rules that should apply on certain config files (probably based on\n> path).\n>\n> A few differences from your reject/accept/priority example:\n>\n> - we don't redefine priority, inheritance rules apply the same way\n> - reject/accept is handled the same way as positive/negative ignore\n> rules. If we're lucky, we could even reuse the exclude code.\n> - instead of special section names like\n>\n>     [config \"section\"]\n>\n> we have something more like\n>\n>     [config \"/this/path\"] # (or pattern)\n>\n> this lets us handle even other config files included by [include] or [includeIf]\n>\n> So, some examples\n>\n> [exclude]            # exclude from all inherited files\n>     key = core.*     # exclude core.*\n>     key = !core.bar  # but keep core.bar\n>\n> [excludeIf \"path:/etc/config\"] # rules apply for only this file\n>    key = ...\n>\n> [excludeIf \"glob:/home/*\"]     # rules apply for these config paths\n>    key = ...\n>\n> [excludeIf \"system\"]           # special names for convenience maybe\n>    key = ...\n>\n>> Obviously the main bottleneck is someone like me working on patching it,\n>\n> Yes, manpower is always the problem.\n>\n>> but in this case it would be very useful if those who are interested in\n>> this could look that proposal over and bikeshed it / point out issues I\n>> may have missed, i.e. \"no, this categorically won't work with this\n>> proposed syntax due to XYZ you haven't thought of...\".\n\nThanks, I like this syntax/proposal much better than my initial one,\nespecially re-using the syntax we have in .gitignore. Also in that it's\nmore similar to the existing include syntax, which in retrospect with an\nexample is the obviously better choice both in terms of UI consistency\nand flexibility.\n\nI.e. I didn't want config files by globs, because depending on compile\noptions the /etc/gitconfig may be in /opt/etc/gitconfig, but as your\n'[excludeIf \"system\"]' and '[excludeIf \"path:/etc/config\"]' examples\nshow we can have our cake and eat it too, and as you demonstrate there's\nother reasons to do path globs that excluding the \"git config\n--{system,global,local,worktree}\" file doesn't cover.\n\nRe priorities: My \"I don't really have a use-case for that\" in 2018 is\nstill 95% true, just a couple of things:\n\n 1. Having it would be a really nice smoke test for this working\n    properly, i.e. now all the config parsing is \"streamed\" to its\n    consumers via callbacks, having priorities would require the ability\n    to pre-buffer and re-arrange it, the \"pre-buffer\" you'd need for any\n    exclude mechanism anyway.\n\n    Once we have that \"priorities\" should just be a quick sort of what\n    order we loop over the files in.\n\n 2. There is the use-case of \"I don't want to exclude core.* from config\n    file <A>, I just want file <B> to override it\". I can imagine\n    especially if/when we have in-repo .gitconfig that you'd want to\n    trust say core.* from there, but have you ~/.gitconfig override it\n    if you've bothered to set any of them yourself.\n\n    But I think most of that use-case doesn't need priorities. It could\n    just be another \"exclude\" syntax for common cases, e.g.:\n\n        # ...Having done something else previously to opt-in to scary\n        # in-repo .gitconfig...\n        [excludeIf \"repo\"]\n        key = core.* # exclude core.*\n        [excludeIf \"repo\"]\n        existingKey = true # exclude any existing key\n\n    So e.g. you'd keep that .gitconfig's gc.bigPackThreshold or\n    whatever, unless your ~/.gitconfig (parsed before, lower priority)\n    had already set it.\n"},{"id":"374500","messageId":"20190425143614.GA91608@google.com","threadId":"50980","inReplyTo":"87k1fis8gq.fsf@evledraar.gmail.com","subject":"Re: How to undo previously set configuration? (again)","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-04-25T14:36:14Z","receivedAt":"2019-04-25T14:36:20Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nÆvar Arnfjörð Bjarmason wrote:\n\n> Because we don't have some general config facility for this it keeps\n> coming up, and various existing/proposed options have their own little\n> custom hacks for doing it, e.g. this for Barret Rhoden's proposed \"blame\n> skip commits\" feature\n> https://public-inbox.org/git/878swhfzxb.fsf@evledraar.gmail.com/\n> (b.t.w. I *meant* /dev/null in that E-Mail, but due to PEBCAK wrote\n> /dev/zero).\n\nI'm confused.  Isn't that bog-standard Git usage, not a custom hack?\nThat is, I thought the intended behavior is always\n\n 1. For single-valued options, last value wins.\n 2. For multi-valued options, empty clears the list.\n 3. When there is a special behavior triggered by not supplying the\n    option at all, offer an explicit value like \"default\" that triggers\n    the same behavior, too.\n\nand that any instance of a command that isn't following that is a bug.\n\nWhich of course leaves room for improvement in documentation and how\nwe organize the implementation (as Peff discussed in [1]), but isn't\nit nice to already have something in place that doesn't require\ninventing a new syntax?\n\nThanks,\nJonahtan\n\n[1] https://public-inbox.org/git/20180406175044.GA32228@sigill.intra.peff.net/\n"},{"id":"374501","messageId":"74ea0269-3e4f-1f6a-c060-c5ac969b91e8@google.com","threadId":"50980","inReplyTo":"20190425143614.GA91608@google.com","subject":"Re: How to undo previously set configuration? (again)","fromName":"Barret Rhoden","fromEmail":"brho@google.com","sentAt":"2019-04-25T14:43:37Z","receivedAt":"2019-04-25T14:43:42Z","isPatch":false,"sender":{"key":"brho@google.com","avatar":null},"body":"Hi -\n\nOn 4/25/19 10:36 AM, Jonathan Nieder wrote:\n> Hi,\n> \n> Ævar Arnfjörð Bjarmason wrote:\n> \n>> Because we don't have some general config facility for this it keeps\n>> coming up, and various existing/proposed options have their own little\n>> custom hacks for doing it, e.g. this for Barret Rhoden's proposed \"blame\n>> skip commits\" feature\n>> https://public-inbox.org/git/878swhfzxb.fsf@evledraar.gmail.com/\n>> (b.t.w. I*meant*  /dev/null in that E-Mail, but due to PEBCAK wrote\n>> /dev/zero).\n> I'm confused.  Isn't that bog-standard Git usage, not a custom hack?\n> That is, I thought the intended behavior is always\n> \n>   1. For single-valued options, last value wins.\n>   2. For multi-valued options, empty clears the list.\n>   3. When there is a special behavior triggered by not supplying the\n>      option at all, offer an explicit value like \"default\" that triggers\n>      the same behavior, too.\n> \n> and that any instance of a command that isn't following that is a bug.\n\nNot sure if it's meant to be the standard, but I just went with the \nstyle used by credential.helper.  This was suggested to me by Johannes \nin [1]:\n\nOn 2019-01-18 at 10:47 Johannes Schindelin <Johannes.Schindelin@gmx.de>\nwrote:\n> A better idea IMHO would be to use an OPT_STRING_LIST() for\n> `--ignore-revs-file`, too, and to allow for multiple\n> `blame.ignoreRevsFile` config entries (with our usual trick of handling an\n> empty setting by resetting the list of paths that were accumulated so\n> far, see e.g. how `credential.helper` is handled).\n\n[1] \nhttps://public-inbox.org/git/nycvar.QRO.7.76.6.1901181038540.41@tvgsbejvaqbjf.bet/\n\nBarret\n\n"},{"id":"374504","messageId":"52917cba-85b3-cf06-b8df-6b56225d4ff8@gmail.com","threadId":"50980","inReplyTo":"20190424224626.GJ6316@genre.crustytoothpaste.net","subject":"Re: [PATCH 3/5] sequencer: add support for multiple hooks","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-04-25T14:59:45Z","receivedAt":"2019-04-25T14:59:52Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 24/04/2019 23:46, brian m. carlson wrote:\n> On Wed, Apr 24, 2019 at 10:51:56AM +0100, Phillip Wood wrote:\n>> On 24/04/2019 01:49, brian m. carlson wrote:\n>>> Add support for multiple post-rewrite hooks, both for \"git commit\n>>> --amend\" and \"git rebase\".\n>>>\n>>> Additionally add support for multiple prepare-commit-msg hooks.\n>>>\n>>> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n>>> ---\n>>>   builtin/am.c                       | 28 ++++++---\n>>\n>> Having read the patch subject I was surprised to see this touching\n>> bulitin/am.c\n> \n> I can rewrite the subject. Unfortunately, the same hook for rebase goes\n> through two wildly different modules, so in order to completely convert\n> the post-rewrite hook, I have to update both at the same time.\n\nIt makes sense to convert both in the same commit, it's just that when I \nsee \"sequencer\" I think of the subset of rebase (-k/-i/-m/-r/-x) that \nuses sequencer.c\n\n>>> +static void run_interactive_rewrite_hook(void)\n>>> +{\n>>> +\tstruct string_list *hooks;\n>>> +\tstruct string_list_item *p;\n>>> +\tstruct child_process child;\n>>> +\n>>> +\thooks = find_hooks(\"post-rewrite\");\n>>> +\tif (!hooks)\n>>> +\t\treturn;\n>>> +\n>>> +\tfor_each_string_list_item(p, hooks) {\n>>> +\t\tchild_process_init(&child);\n>>> +\n>>> +\t\tchild.in = open(rebase_path_rewritten_list(),\n>>> +\t\t\tO_RDONLY);\n>>> +\t\tchild.stdout_to_stderr = 1;\n>>> +\t\tchild.trace2_hook_name = \"post-rewrite\";\n>>> +\t\targv_array_push(&child.args, p->string);\n>>> +\t\targv_array_push(&child.args, \"rebase\");\n>>> +\t\tif (run_command(&child))\n>>> +\t\t\tbreak;\n>>> +\t}\n>>> +\tfree_hooks(hooks);\n>>>   }\n>>\n>> If you're adding a function to do this it would be nice to use it from\n>> am.c as well rather than duplicating essentially the same code. Is there\n>> any way to use a helper to run all the hooks, rather than introducing a\n>> similar loop everywhere where we call a hook?\n> \n> It's becoming clear to me that a helper is probably going to be cleaner,\n> so I'll add one in for v2.\n> \n>>>   void commit_post_rewrite(struct repository *r,\n>>> @@ -1326,6 +1362,7 @@ static int try_to_commit(struct repository *r,\n>>>   \tchar *amend_author = NULL;\n>>>   \tconst char *hook_commit = NULL;\n>>>   \tenum commit_msg_cleanup_mode cleanup;\n>>> +\tstruct string_list *hooks;\n>>>   \tint res = 0;\n>>>   \n>>>   \tif (parse_head(r, &current_head))\n>>> @@ -1369,7 +1406,10 @@ static int try_to_commit(struct repository *r,\n>>>   \t\tgoto out;\n>>>   \t}\n>>>   \n>>> -\tif (find_hook(\"prepare-commit-msg\")) {\n>>> +\thooks = find_hooks(\"prepare-commit-msg\");\n>>> +\tif (hooks) {\n>>> +\t\tfree_hooks(hooks);\n>>\n>> I think you forgot to update run_prepare_commit_msg_hook(), it should\n>> probably be passed this list now. It might be outside the scope of this\n>> series but unifying this with builtin/commit.c\n> \n> run_prepare_commit_msg_hook calls run_hook_le, which looks up that value\n> itself. It's unfortunate that we have to do it twice, but we need to\n> know whether we need to re-read the commit msg or not. I can explain\n> this further in the commit message.\n\nThanks for explaining that, it would be nice to have that in the commit \nmessage (I probably should have read the previous two patches to see \nwhat run_hook_le() was doing).\n\nBest Wishes\n\nPhillip\n\n\n>>> diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\n>>> index ba8bd1b514..5b83f037b5 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>>> +commit_command () {\n>>> +\techo \"$1\" >>file &&\n>>> +\tgit add file &&\n>>> +\tgit commit -m \"$1\"\n>>> +}\n>>> +\n>>> +test_multiple_hooks prepare-commit-msg commit_command\n>>\n>> It's not clear to me that this is testing the sequencer\n> \n> You're right. I need to adopt a different approach here.\n> \n"},{"id":"374506","messageId":"87ef5qrtt4.fsf@evledraar.gmail.com","threadId":"50980","inReplyTo":"20190425143614.GA91608@google.com","subject":"Re: How to undo previously set configuration? (again)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-04-25T15:25:11Z","receivedAt":"2019-04-25T15:25:19Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Apr 25 2019, Jonathan Nieder wrote:\n\n> Ævar Arnfjörð Bjarmason wrote:\n>\n>> Because we don't have some general config facility for this it keeps\n>> coming up, and various existing/proposed options have their own little\n>> custom hacks for doing it, e.g. this for Barret Rhoden's proposed \"blame\n>> skip commits\" feature\n>> https://public-inbox.org/git/878swhfzxb.fsf@evledraar.gmail.com/\n>> (b.t.w. I *meant* /dev/null in that E-Mail, but due to PEBCAK wrote\n>> /dev/zero).\n>\n> I'm confused.  Isn't that bog-standard Git usage, not a custom hack?\n> That is, I thought the intended behavior is always\n>\n>  1. For single-valued options, last value wins.\n>  2. For multi-valued options, empty clears the list.\n>  3. When there is a special behavior triggered by not supplying the\n>     option at all, offer an explicit value like \"default\" that triggers\n>     the same behavior, too.\n>\n> and that any instance of a command that isn't following that is a bug.\n>\n> Which of course leaves room for improvement in documentation and how\n> we organize the implementation (as Peff discussed in [1]), but isn't\n> it nice to already have something in place that doesn't require\n> inventing a new syntax?\n\nWe can quibble over the wording, but I'd say that's a \"hack\". Yeah for\n*some* options setting it to an empty value clears whether it's a\nsingle-value or multi-value.\n\nBut it's entirely dependent on the specific callback implemented for\nthat option, sometimes there's no way to \"reset\" something, and in the\nbest case you'd need to exhaustively enumerate everything (e.g. for the\nsendemail.* stuff) and hope nobody adds a new option tomorrow, you can't\nsay \"clear this wildard\" or \"no config from system-wide\".\n\nSo I think it makes sense to think about how we could promote this to\nthe config syntax itself so that what somewhat-mostly-but-not-quite\nworks now by convention would be guaranteed to work, and things that you\ncan't do at all today (e.g. ignore system config, or selectively ignore\nsomething) would be supported.\n"},{"id":"374507","messageId":"87d0lartq2.fsf@evledraar.gmail.com","threadId":"50980","inReplyTo":"74ea0269-3e4f-1f6a-c060-c5ac969b91e8@google.com","subject":"Re: How to undo previously set configuration? (again)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-04-25T15:27:01Z","receivedAt":"2019-04-25T15:27:08Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Apr 25 2019, Barret Rhoden wrote:\n\n> Hi -\n>\n> On 4/25/19 10:36 AM, Jonathan Nieder wrote:\n>> Hi,\n>>\n>> Ævar Arnfjörð Bjarmason wrote:\n>>\n>>> Because we don't have some general config facility for this it keeps\n>>> coming up, and various existing/proposed options have their own little\n>>> custom hacks for doing it, e.g. this for Barret Rhoden's proposed \"blame\n>>> skip commits\" feature\n>>> https://public-inbox.org/git/878swhfzxb.fsf@evledraar.gmail.com/\n>>> (b.t.w. I*meant*  /dev/null in that E-Mail, but due to PEBCAK wrote\n>>> /dev/zero).\n>> I'm confused.  Isn't that bog-standard Git usage, not a custom hack?\n>> That is, I thought the intended behavior is always\n>>\n>>   1. For single-valued options, last value wins.\n>>   2. For multi-valued options, empty clears the list.\n>>   3. When there is a special behavior triggered by not supplying the\n>>      option at all, offer an explicit value like \"default\" that triggers\n>>      the same behavior, too.\n>>\n>> and that any instance of a command that isn't following that is a bug.\n>\n> Not sure if it's meant to be the standard, but I just went with the\n> style used by credential.helper.  This was suggested to me by Johannes\n> in [1]:\n\nJust to be clear I'm not picking on your patch, I think doing it that\nway makes perfect sense in the current system.\n\nJust as noted upthread using it as an example (that was fresh in my\nmind...) of a case where we want this, but it's a bespoke thing for\nevery use-case, and not consistent (i.e. just supported for your new\nthing, but not for the existing fsck code that's mostly the same).\n\n> On 2019-01-18 at 10:47 Johannes Schindelin <Johannes.Schindelin@gmx.de>\n> wrote:\n>> A better idea IMHO would be to use an OPT_STRING_LIST() for\n>> `--ignore-revs-file`, too, and to allow for multiple\n>> `blame.ignoreRevsFile` config entries (with our usual trick of handling an\n>> empty setting by resetting the list of paths that were accumulated so\n>> far, see e.g. how `credential.helper` is handled).\n>\n> [1]\n> https://public-inbox.org/git/nycvar.QRO.7.76.6.1901181038540.41@tvgsbejvaqbjf.bet/\n>\n> Barret\n"},{"id":"374525","messageId":"2a511c3c-ef8d-07c5-80e2-398780ee6ed4@kdbg.org","threadId":"50980","inReplyTo":"xmqqo94uzyxa.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/5] run-command: add preliminary support for multiple hooks","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-04-25T19:40:34Z","receivedAt":"2019-04-25T19:40:40Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 25.04.19 um 02:55 schrieb Junio C Hamano:\n> Johannes Sixt <j6t@kdbg.org> writes:\n> \n>> Furthermore, basing a decision on whether a file is executable won't\n>> work on Windows as intended. So, it is better to aim for an existence check.\n> \n> That is a good point.\n> \n> So it may be OK for \"do we have a single hook script for this hook\n> name?\" to say \"no\" when the path exists but not executable on\n> POSIXPERM systems, but it is better to say \"yes\" for consistency\n> across platforms (I think that is one of the reasons why we use\n> .sample suffix these days).\n\nAll correct.\n\n> And for the same reason, for the purpose of deciding \"because we do\n> not have a single hook script, let's peek into .d directory\n> ourselves\", mere presence of the file with that name, regardless of\n> the executable bit, should signal that we should not handle the .d\n> directory.\n> \n> IOW, you think access(X_OK) should be more like access(F_OK)?\n\nYes, that's my conclusion.\n\n-- Hannes\n"},{"id":"374535","messageId":"xmqqmukdwm32.fsf@gitster-ct.c.googlers.com","threadId":"50980","inReplyTo":"20190425143614.GA91608@google.com","subject":"Re: How to undo previously set configuration? (again)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-26T02:13:05Z","receivedAt":"2019-04-26T02:13:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> I'm confused.  Isn't that bog-standard Git usage, not a custom hack?\n\nDepends on the definition of \"hack\".\n\nThe only native provision at the config API level that lets the\ncalling programs realize the \"desired behaviour\" you listed below\nwithout doing anything special is \"for a single-valued one, the last\none wins\", for which the newer style config_get_value() will give\nyou the \"last one wins\" semantics.  With the original style parsing\nusing the git_config() with callback still needs to do the \"last one\nwins\" by the caller of the config API.\n\nThe \"empty clears the list\" for multi-valued option is totally up to\nthe application.  Even the newer configset__get_value_multi() API\nfunction does not reset a list to empty when it sees an empty\nstring.  The caller gets a list of value strings that happens to\nhave an empty string in the middle.\n\nSo, I think the \"bog-standard\" Git convention must be maintained by\neach application codepath to implement it with a custom hack.\n\nPerhaps you did not mean with \"hack\" a \"hacky implementation\", but a\n\"hacky design\".  If that is the case, yeah, I agree with you that\nthe items #1 and #2 below are what we try to make sure our programs\nfollow; I am not sure about #3 myself, as I do not think offhand a\ngood example of it, though.\n\n> That is, I thought the intended behavior is always\n>\n>  1. For single-valued options, last value wins.\n>  2. For multi-valued options, empty clears the list.\n>  3. When there is a special behavior triggered by not supplying the\n>     option at all, offer an explicit value like \"default\" that triggers\n>     the same behavior, too.\n>\n> and that any instance of a command that isn't following that is a bug.\n\nIf we make that declaration, we could enforce the \"empty clears the\nlist\" at the API level when the configset API is in use.\n"},{"id":"374550","messageId":"CACsJy8B5j2K=RU7N+h-i9HszuYuYd+fNmAftVDW-4nJ_o5z8Sw@mail.gmail.com","threadId":"50980","inReplyTo":"20190425143614.GA91608@google.com","subject":"Re: How to undo previously set configuration? (again)","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-04-26T09:36:40Z","receivedAt":"2019-04-26T09:37:09Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Apr 25, 2019 at 9:36 PM Jonathan Nieder <jrnieder@gmail.com> wrote:\n>\n> Hi,\n>\n> Ævar Arnfjörð Bjarmason wrote:\n>\n> > Because we don't have some general config facility for this it keeps\n> > coming up, and various existing/proposed options have their own little\n> > custom hacks for doing it, e.g. this for Barret Rhoden's proposed \"blame\n> > skip commits\" feature\n> > https://public-inbox.org/git/878swhfzxb.fsf@evledraar.gmail.com/\n> > (b.t.w. I *meant* /dev/null in that E-Mail, but due to PEBCAK wrote\n> > /dev/zero).\n>\n> I'm confused.  Isn't that bog-standard Git usage, not a custom hack?\n> That is, I thought the intended behavior is always\n>\n>  1. For single-valued options, last value wins.\n>  2. For multi-valued options, empty clears the list.\n\nI didn't know this! Should it be documented? At least a quick skimming\nthrough config.txt does not mention anything about empty value\nclearing multi-valued options.\n\nI also wanted to see if it's true. However, the first var I checked,\nbranch.*.merge does not follow this rule. I got disappointed and\nstopped.\n\n>  3. When there is a special behavior triggered by not supplying the\n>     option at all, offer an explicit value like \"default\" that triggers\n>     the same behavior, too.\n>\n> and that any instance of a command that isn't following that is a bug.\n\nNot sure how many we need to fix. The behaviour does make sense to me though.\n\n> Which of course leaves room for improvement in documentation and how\n> we organize the implementation (as Peff discussed in [1]), but isn't\n> it nice to already have something in place that doesn't require\n> inventing a new syntax?\n\nThis cannot undefine a variable though, especially those single-valued\nones. But I think for most cases, the user just needs to find out what\nis the default value and set to that one.\n\nI don't know if there is any case where the lack of a variable behaves\ndifferently (or the default value is too dynamic to be set manually)\nthough.\n\n> Thanks,\n> Jonahtan\n>\n> [1] https://public-inbox.org/git/20180406175044.GA32228@sigill.intra.peff.net/\n\n\n\n-- \nDuy\n"},{"id":"374566","messageId":"871s1osskt.fsf@evledraar.gmail.com","threadId":"50980","inReplyTo":"87imv2s3dz.fsf@evledraar.booking.com","subject":"Re: How to undo previously set configuration? (again)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-04-26T15:18:42Z","receivedAt":"2019-04-26T15:18:48Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Apr 25 2019, Ævar Arnfjörð Bjarmason wrote:\n\n> On Thu, Apr 25 2019, Duy Nguyen wrote:\n>\n>> On Thu, Apr 25, 2019 at 5:08 PM Ævar Arnfjörð Bjarmason\n>> <avarab@gmail.com> wrote:\n>>> >> Solving (1) without (2) feels like a bit of a missed opportunity to\n>>> >> me.  Ideally, what I would like is\n>>> >>\n>>> >>    i. A central registry of trustworthy Git hooks that can be upgraded\n>>> >>       using the system package manager to address (2).  Perhaps just\n>>> >>       git-hook-* commands on the $PATH.\n>>> >>\n>>> >>   ii. Instead of putting hooks in .git/hooks, put a list of hooks to\n>>> >>       run for each event in .git/config.\n>>> >\n>>> > The problem I had with this when discussing it was that our\n>>> > configuration system lacks a good way to control inheritance from outer\n>>> > files. I recently was working with a system-wide gitconfig file that\n>>> > referred to files I didn't have, and my Git installation was subtly\n>>> > broken in a variety of ways.\n>>> >\n>>> > If I have a system-wide hook to run for company code, but I have a\n>>> > checkout for my personal dotfiles on my machine where I don't want to\n>>> > run that hook, our configuration lacks a way for me to disable that\n>>> > system-wide configuration. However, using our current system, I can\n>>> > override core.hooksPath in this case and everything works fine.\n>>> >\n>>> > I mentioned this for completeness, and because I hope that some of those\n>>> > people will get some time to chime in here, but I think without that\n>>> > feature, we end up with a worse experience than we have now.\n>>>\n>>> I sent a proposal for this last year \"How to undo previously set\n>>> configuration?\":\n>>> https://public-inbox.org/git/874lkq11ug.fsf@evledraar.gmail.com/\n>>\n>> While reading that mail, it occurs to me that perhaps we can reuse the\n>> .gitignore idea.\n>>\n>> Instead of having a list of untracked files, we have a list of config\n>> keys. Instead of having .gitignore files associated to different\n>> directories to apply the rules to those dirs only, we have ignore\n>> rules that should apply on certain config files (probably based on\n>> path).\n>>\n>> A few differences from your reject/accept/priority example:\n>>\n>> - we don't redefine priority, inheritance rules apply the same way\n>> - reject/accept is handled the same way as positive/negative ignore\n>> rules. If we're lucky, we could even reuse the exclude code.\n>> - instead of special section names like\n>>\n>>     [config \"section\"]\n>>\n>> we have something more like\n>>\n>>     [config \"/this/path\"] # (or pattern)\n>>\n>> this lets us handle even other config files included by [include] or [includeIf]\n>>\n>> So, some examples\n>>\n>> [exclude]            # exclude from all inherited files\n>>     key = core.*     # exclude core.*\n>>     key = !core.bar  # but keep core.bar\n>>\n>> [excludeIf \"path:/etc/config\"] # rules apply for only this file\n>>    key = ...\n>>\n>> [excludeIf \"glob:/home/*\"]     # rules apply for these config paths\n>>    key = ...\n>>\n>> [excludeIf \"system\"]           # special names for convenience maybe\n>>    key = ...\n>>\n>>> Obviously the main bottleneck is someone like me working on patching it,\n>>\n>> Yes, manpower is always the problem.\n>>\n>>> but in this case it would be very useful if those who are interested in\n>>> this could look that proposal over and bikeshed it / point out issues I\n>>> may have missed, i.e. \"no, this categorically won't work with this\n>>> proposed syntax due to XYZ you haven't thought of...\".\n>\n> Thanks, I like this syntax/proposal much better than my initial one,\n> especially re-using the syntax we have in .gitignore. Also in that it's\n> more similar to the existing include syntax, which in retrospect with an\n> example is the obviously better choice both in terms of UI consistency\n> and flexibility.\n>\n> I.e. I didn't want config files by globs, because depending on compile\n> options the /etc/gitconfig may be in /opt/etc/gitconfig, but as your\n> '[excludeIf \"system\"]' and '[excludeIf \"path:/etc/config\"]' examples\n> show we can have our cake and eat it too, and as you demonstrate there's\n> other reasons to do path globs that excluding the \"git config\n> --{system,global,local,worktree}\" file doesn't cover.\n>\n> Re priorities: My \"I don't really have a use-case for that\" in 2018 is\n> still 95% true, just a couple of things:\n>\n>  1. Having it would be a really nice smoke test for this working\n>     properly, i.e. now all the config parsing is \"streamed\" to its\n>     consumers via callbacks, having priorities would require the ability\n>     to pre-buffer and re-arrange it, the \"pre-buffer\" you'd need for any\n>     exclude mechanism anyway.\n>\n>     Once we have that \"priorities\" should just be a quick sort of what\n>     order we loop over the files in.\n>\n>  2. There is the use-case of \"I don't want to exclude core.* from config\n>     file <A>, I just want file <B> to override it\". I can imagine\n>     especially if/when we have in-repo .gitconfig that you'd want to\n>     trust say core.* from there, but have you ~/.gitconfig override it\n>     if you've bothered to set any of them yourself.\n>\n>     But I think most of that use-case doesn't need priorities. It could\n>     just be another \"exclude\" syntax for common cases, e.g.:\n>\n>         # ...Having done something else previously to opt-in to scary\n>         # in-repo .gitconfig...\n>         [excludeIf \"repo\"]\n>         key = core.* # exclude core.*\n>         [excludeIf \"repo\"]\n>         existingKey = true # exclude any existing key\n>\n>     So e.g. you'd keep that .gitconfig's gc.bigPackThreshold or\n>     whatever, unless your ~/.gitconfig (parsed before, lower priority)\n>     had already set it.\n\nA #3 I just encountered[1] where this settable \"config priority\" might\nbe handy, but perhaps it's still stupid and exclusions are enough:\n\n * Vendor's git server wants to run 'git -c gc.writeCommitGraph gc' to\n   get commit graphs. I might want to override it.\n\n * The vendor can't just add that to /etc/gitconfig because they don't\n   want to screw with the OS, or might not know which \"git\" they'll use\n   (their own or OS, so system \"gitconfig\" in different\n   places/inconsistent)\n\nSo something where they can just do that and I can in what *I* know to\nbe the system \"gitconfig\" do:\n\n    [configPriority \"cli-at-cwd:/var/lib/vendor/git-storage/*\"]\n    value = 5\n\nIf I know they'll be be running those commands on that path, and I'd\nlike to s/100/5/ the priority for \"-c\" there so it goes last (see the\nsuggested priority numbers in [2]).\n\nOr maybe alternatively, we'd have something like \"-c\" (unfortunately\n\"-C\" is taken) to add a new config file to the mix, without making it an\nall-or-nothing like GIT_CONFIG_NOSYSTEM=1 and GIT_CONFIG=<path>). So\nthey:\n\n    git --add-this-config-last-please=/var/lib/vendor/etc/gitaly/gitconfig gc\n\nAnd then I do in /etc/gitconfig:\n\n    [excludeIf \"glob:/var/lib/vendor/etc/gitaly/gitconfig\"]\n    key = gc.*\n\nBut priorities might still be sensible. This use-case could be\nalternatively done without them with a more sensible version of\n\"excludeIf.existingKey\" discussed in the last mail. I.e. having\n\"existingKey\" be a glob, not \"true\":\n\n    [excludeIf \"glob:/var/lib/vendor/etc/gitaly/gitconfig\"]\n    existingKey = gc.*\n\nDitto for \"-c\" values:\n\n    [excludeIf \"cli-at-cwd:/var/lib/vendor/git-storage/*\"]\n    existingKey = gc.*\n\nSo maybe I've managed to talk myself out this whole notion of\npriorities.\n\nI.e. maybe we can always process config in a pre-determined order and\njust allow people to reach forward/backward with [excludeIf path/glob/-c\nat cwd] & [exclude], respectively.\n\nThere's still the *theoretical* use-case of a user saying \"I know the\nsysadmin here knows better, I want their /etc/gitconfig to go after my\n~/.gitconfig\", but does anyone need/want it in practice? I don't know...\n\n1. https://gitlab.com/gitlab-org/gitaly/issues/1643\n2. https://public-inbox.org/git/874lkq11ug.fsf@evledraar.gmail.com/\n"},{"id":"374576","messageId":"20190426205859.GO6316@genre.crustytoothpaste.net","threadId":"50980","inReplyTo":"2a511c3c-ef8d-07c5-80e2-398780ee6ed4@kdbg.org","subject":"Re: [PATCH 1/5] run-command: add preliminary support for multiple hooks","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-04-26T20:58:59Z","receivedAt":"2019-04-26T20:59:07Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Thu, Apr 25, 2019 at 09:40:34PM +0200, Johannes Sixt wrote:\n> Am 25.04.19 um 02:55 schrieb Junio C Hamano:\n> > Johannes Sixt <j6t@kdbg.org> writes:\n> > \n> >> Furthermore, basing a decision on whether a file is executable won't\n> >> work on Windows as intended. So, it is better to aim for an existence check.\n> > \n> > That is a good point.\n> > \n> > So it may be OK for \"do we have a single hook script for this hook\n> > name?\" to say \"no\" when the path exists but not executable on\n> > POSIXPERM systems, but it is better to say \"yes\" for consistency\n> > across platforms (I think that is one of the reasons why we use\n> > .sample suffix these days).\n> \n> All correct.\n\nI would like to point out that we still have to perform an executability\ncheck before we run the hook or we'll get errors printed to the user. As\nI mentioned, there are many people with repositories that have the\nnon-.sample files. For me, any repository older than about five years\nwill likely have those files.\n\n> > And for the same reason, for the purpose of deciding \"because we do\n> > not have a single hook script, let's peek into .d directory\n> > ourselves\", mere presence of the file with that name, regardless of\n> > the executable bit, should signal that we should not handle the .d\n> > directory.\n> > \n> > IOW, you think access(X_OK) should be more like access(F_OK)?\n> \n> Yes, that's my conclusion.\n\nRight now, we have a standard way to handle the way we handle hooks: if\nthey are not executable, we warn and pretend there's no hook. With this\nnew paradigm, we have to check whether the main hook is executable, and\nif it's not, we then have to check whether it's present, and if so, we\nskip the multiple hooks.\n\nI understand the executable bit is not useful on Windows, but on Unix,\nwe should be consistent with how we treat the hooks.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"374580","messageId":"cf9818d5-6b58-49bf-8897-28ddb407f557@kdbg.org","threadId":"50980","inReplyTo":"20190426205859.GO6316@genre.crustytoothpaste.net","subject":"Re: [PATCH 1/5] run-command: add preliminary support for multiple hooks","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-04-26T21:53:39Z","receivedAt":"2019-04-26T21:53:45Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 26.04.19 um 22:58 schrieb brian m. carlson:\n> On Thu, Apr 25, 2019 at 09:40:34PM +0200, Johannes Sixt wrote:\n> I would like to point out that we still have to perform an executability\n> check before we run the hook or we'll get errors printed to the user.\n\nThat's fine. On Windows, when a hook is present, it is also executable.\n\n> Right now, we have a standard way to handle the way we handle hooks: if\n> they are not executable, we warn and pretend there's no hook. With this\n> new paradigm, we have to check whether the main hook is executable, and\n> if it's not, we then have to check whether it's present, and if so, we\n> skip the multiple hooks.\n> \n> I understand the executable bit is not useful on Windows, but on Unix,\n> we should be consistent with how we treat the hooks.\n\nWe want to check for two vastly different conditions:\n\n- Do we have to inspect the multi-hook directory? That decision should\nbe based on existence.\n\n- Do we have to issue a warning? That can be based on the executable\nflag. (As I understand, this is just a convenience warning because we do\nnot want the user to see a cryptic \"cannot execute this thing\" error or\nsomething.)\n\nI can see that you sense an inconsistency when you treat \"not\nexecutable\" as \"does not exist\". But that is just too subtle in my book,\nhard to explain, and not the practice that we are exercising these days.\n\nI'm more concerned about the platform differences that we would have to\nnote in the documentation:\n\n  \"To have multple hooks, do X and Y and make sure the standard hook\n   file is not executable. Oh, and by the way, if you are on Windows,\n   you have to remove the file to make it not executable.\"\n\nLet's not go there.\n\n-- Hannes\n"},{"id":"374716","messageId":"20190430211415.GB16290@sigill.intra.peff.net","threadId":"50980","inReplyTo":"CACsJy8B5j2K=RU7N+h-i9HszuYuYd+fNmAftVDW-4nJ_o5z8Sw@mail.gmail.com","subject":"Re: How to undo previously set configuration? (again)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-30T21:14:15Z","receivedAt":"2019-04-30T21:14:19Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 26, 2019 at 04:36:40PM +0700, Duy Nguyen wrote:\n\n> > I'm confused.  Isn't that bog-standard Git usage, not a custom hack?\n> > That is, I thought the intended behavior is always\n> >\n> >  1. For single-valued options, last value wins.\n> >  2. For multi-valued options, empty clears the list.\n> \n> I didn't know this! Should it be documented? At least a quick skimming\n> through config.txt does not mention anything about empty value\n> clearing multi-valued options.\n> \n> I also wanted to see if it's true. However, the first var I checked,\n> branch.*.merge does not follow this rule. I got disappointed and\n> stopped.\n\nIt's definitely not implemented universally; each consumer of the config\noption must decide on it (and it will probably always be that way to\nsome degree, since we don't know the semantics of each options; recall\nthat we may be holding config keys for other non-core programs, too).\nAnd we just haven't retro-fitted a lot of those older options because\nnobody has been bothered by it.\n\nThat said, I am a proponent of having some kind of clearing mechanism\n(and I was the one who added credential.helper's mechanism, which has\nbeen mentioned in this thread).  I think it makes things a lot less\ndifficult if we don't have to change the syntax of the config files to\ndo it. With that constraint, that pretty much leaves:\n\n  1. Some sentinel value like the empty string. That one _probably_\n     works in most cases, but there may be lists which want to represent\n     the empty value. There could be other sentinel values (e.g.,\n     \"CLEAR\") which are simply unlikely to be used as real values.\n\n  2. The boolean syntax (i.e., \"[foo]bar\" with no equals) is almost\n     always bogus for a list. So that can work as a sentinel that is\n     OK syntactically.\n\n> > Which of course leaves room for improvement in documentation and how\n> > we organize the implementation (as Peff discussed in [1]), but isn't\n> > it nice to already have something in place that doesn't require\n> > inventing a new syntax?\n> \n> This cannot undefine a variable though, especially those single-valued\n> ones. But I think for most cases, the user just needs to find out what\n> is the default value and set to that one.\n\nHaving \"default\" is a little more convenient, but it would need to be\nimplemented on a per-value basis anyway. In many cases the caller of the\nconfig code has implemented last-one-wins by overwriting an old value,\nand only it knows how to restore \"default\".\n\nI guess if we moved completely to a configset world, then asking for\ngit_config_get_string() could resolve \"default\" completely. But we are a\nways from that.\n\n-Peff\n"},{"id":"374717","messageId":"20190430213902.GC16290@sigill.intra.peff.net","threadId":"50980","inReplyTo":"20190424004948.728326-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH 0/5] Multiple hook support","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-30T21:39:02Z","receivedAt":"2019-04-30T21:39:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 24, 2019 at 12:49:43AM +0000, brian m. carlson wrote:\n\n> I've talked with some people about this approach, and they've indicated\n> they would prefer a configuration-based approach.\n\nI think I'm some people. :)\n\nI agree with the thoughts that Jonathan pointed out in [1], but I wanted\nto raise a few points that are more directly related to hook features:\n\n  1. Config is resolved at run-time, making it much easier to have\n     system or user-level hooks (as opposed to our current system of\n     on-disk files, which require copying or symlinking hooks ahead of\n     time into each repository you want to impact).\n\n  2. Config values let you easily run hooks from multiple sources (e.g.,\n     a hook specified in /etc/gitconfig, one in ~/.gitconfig, and then a\n     repo-level hook in .git/config). Even with a \"hook.d\" feature like\n     this, you are back to doing lots of symlinks within the \".d\"\n     directory to get this behavior.\n\n     I specifically worry that adding \".d\" directories is a step in the\n     wrong direction because our solution will probably make this point\n     _worse_ than whatever custom trampolines people are already using.\n\n  3. A well-designed config schema can leave room for more\n     configuration. E.g., one of the big questions with multi-hooks is\n     the error semantics. But what if we had:\n\n       [hook \"pre-receive\"]\n       command = my-hook-cmd\n       command = another-hook-cmd\n       # stop running and return failure at first non-zero exit\n       errorBehavior = stop-on-first\n       # ...or run all and return error if _any_ failed\n       errorBehavior = report-any-error\n       # ...or run and report if any _accepted_\n       errorBehavior = report-any-success\n\n      Those are just off the top of my head. But my point is that by\n      staking out a config section for each hook, it gives us a place to\n      naturally add new config options. And we can do it on a per-hook\n      basis, which I think will be important since each hook has its own\n      semantics.\n\n      Now that's not _strictly_ necessary. We could still have\n      \"hook.pre-receive.errorBehavior\" and just assume\n      \"hook.pre-receive.command\" is \"$GIT_DIR/hooks/pre-receive\". But I\n      think doing the whole thing from config makes the behavior simple\n      and consistent (and the backwards compatibility is easy -- if they\n      aren't using the new command config, we really do behave \"as if\"\n      they had set it to the file in the hooks directory).\n\nSo I agree with your general sentiment that the multi-hook support\nis conceptually orthogonal to switching to a config-based system. But I\nthink it's worth considering whether we want to do something\nconfig-based first:\n\n  - if we introduce it later, it saves us from having _three_ ways to do\n    the same thing\n\n  - I think it provides a more natural way to express the options that\n    will inevitably grow once we have multi-hook support\n\n-Peff\n\n[1] https://public-inbox.org/git/20190424023438.GE98980@google.com/\n"},{"id":"374751","messageId":"CACsJy8B+hDqKnu+0tkPC42w+_6RhzYac1BxYtdyxctcARG=VCg@mail.gmail.com","threadId":"50980","inReplyTo":"20190430211415.GB16290@sigill.intra.peff.net","subject":"Re: How to undo previously set configuration? (again)","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-01T11:41:24Z","receivedAt":"2019-05-01T11:41:53Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, May 1, 2019 at 4:14 AM Jeff King <peff@peff.net> wrote:\n> It's definitely not implemented universally; each consumer of the config\n> option must decide on it (and it will probably always be that way to\n> some degree, since we don't know the semantics of each options; recall\n> that we may be holding config keys for other non-core programs, too).\n> And we just haven't retro-fitted a lot of those older options because\n> nobody has been bothered by it.\n>\n> That said, I am a proponent of having some kind of clearing mechanism\n> (and I was the one who added credential.helper's mechanism, which has\n> been mentioned in this thread).  I think it makes things a lot less\n> difficult if we don't have to change the syntax of the config files to\n> do it. With that constraint, that pretty much leaves:\n>\n>   1. Some sentinel value like the empty string. That one _probably_\n>      works in most cases, but there may be lists which want to represent\n>      the empty value. There could be other sentinel values (e.g.,\n>      \"CLEAR\") which are simply unlikely to be used as real values.\n>\n>   2. The boolean syntax (i.e., \"[foo]bar\" with no equals) is almost\n>      always bogus for a list. So that can work as a sentinel that is\n>      OK syntactically.\n\nAnother question about the universal clearing mechanism, how does \"git\nconfig\" fit into this? It would be great if the user can actually see\nthe same thing a git command sees to troubleshoot. Option 1 leaves the\ninterpretation/guessing to the user, \"git config\" simply gives the raw\ninput list before \"clear\" is processed. Option 2, \"git config\"\nprobably can be taught to optionally clear when it sees the boolean\nsyntax.\n-- \nDuy\n"},{"id":"374752","messageId":"87r29iqsf9.fsf@evledraar.gmail.com","threadId":"50980","inReplyTo":"CACsJy8B+hDqKnu+0tkPC42w+_6RhzYac1BxYtdyxctcARG=VCg@mail.gmail.com","subject":"Re: How to undo previously set configuration? (again)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-05-01T12:18:34Z","receivedAt":"2019-05-01T12:18:40Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, May 01 2019, Duy Nguyen wrote:\n\n> On Wed, May 1, 2019 at 4:14 AM Jeff King <peff@peff.net> wrote:\n>> It's definitely not implemented universally; each consumer of the config\n>> option must decide on it (and it will probably always be that way to\n>> some degree, since we don't know the semantics of each options; recall\n>> that we may be holding config keys for other non-core programs, too).\n>> And we just haven't retro-fitted a lot of those older options because\n>> nobody has been bothered by it.\n>>\n>> That said, I am a proponent of having some kind of clearing mechanism\n>> (and I was the one who added credential.helper's mechanism, which has\n>> been mentioned in this thread).  I think it makes things a lot less\n>> difficult if we don't have to change the syntax of the config files to\n>> do it. With that constraint, that pretty much leaves:\n>>\n>>   1. Some sentinel value like the empty string. That one _probably_\n>>      works in most cases, but there may be lists which want to represent\n>>      the empty value. There could be other sentinel values (e.g.,\n>>      \"CLEAR\") which are simply unlikely to be used as real values.\n>>\n>>   2. The boolean syntax (i.e., \"[foo]bar\" with no equals) is almost\n>>      always bogus for a list. So that can work as a sentinel that is\n>>      OK syntactically.\n>\n> Another question about the universal clearing mechanism, how does \"git\n> config\" fit into this? It would be great if the user can actually see\n> the same thing a git command sees to troubleshoot. Option 1 leaves the\n> interpretation/guessing to the user, \"git config\" simply gives the raw\n> input list before \"clear\" is processed. Option 2, \"git config\"\n> probably can be taught to optionally clear when it sees the boolean\n> syntax.\n\nWe can make it fancier, but we already deal with this, e.g. if you do\n\"git config -l\" we'll show \"include{,if}\" directives at the same \"level\"\nas other \"normal\" keys.\n\nWe also provide no way in \"git config\" to properly interpret a\nvalue. E.g. does a \"user.email\" showing up twice for me mean I have two\nE-Mails at the same time, or does the last one win? We both know the\nanswer, but git-config itself doesn't, and that information lives in\ndocs/code outside of it.\n\nSimilarly we'd just print a sequence of:\n\n    user.name=foo\n    user.email=bar\n    exclude.key=user.*\n    user.name=baz\n\nAnd it would be up to some \"smarter\" reader of the config data to\nrealize that the end result is one where we have no \"user.email\" set,\nand \"user.name=baz\".\n\nBut yeah, optionally having some new --list-normalized or\n--list-after-excludes or whatever would be great, and presumably not\nhard if we had some central \"excludes\" mechanism...\n"},{"id":"374753","messageId":"CACsJy8CSPBbxrJm1sh4a+f_B03MFXxDXSrOvb6yFCRa4Qa2wMQ@mail.gmail.com","threadId":"50980","inReplyTo":"87r29iqsf9.fsf@evledraar.gmail.com","subject":"Re: How to undo previously set configuration? (again)","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-01T12:32:20Z","receivedAt":"2019-05-01T12:32:49Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, May 1, 2019 at 7:18 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>\n>\n> On Wed, May 01 2019, Duy Nguyen wrote:\n>\n> > On Wed, May 1, 2019 at 4:14 AM Jeff King <peff@peff.net> wrote:\n> >> It's definitely not implemented universally; each consumer of the config\n> >> option must decide on it (and it will probably always be that way to\n> >> some degree, since we don't know the semantics of each options; recall\n> >> that we may be holding config keys for other non-core programs, too).\n> >> And we just haven't retro-fitted a lot of those older options because\n> >> nobody has been bothered by it.\n> >>\n> >> That said, I am a proponent of having some kind of clearing mechanism\n> >> (and I was the one who added credential.helper's mechanism, which has\n> >> been mentioned in this thread).  I think it makes things a lot less\n> >> difficult if we don't have to change the syntax of the config files to\n> >> do it. With that constraint, that pretty much leaves:\n> >>\n> >>   1. Some sentinel value like the empty string. That one _probably_\n> >>      works in most cases, but there may be lists which want to represent\n> >>      the empty value. There could be other sentinel values (e.g.,\n> >>      \"CLEAR\") which are simply unlikely to be used as real values.\n> >>\n> >>   2. The boolean syntax (i.e., \"[foo]bar\" with no equals) is almost\n> >>      always bogus for a list. So that can work as a sentinel that is\n> >>      OK syntactically.\n> >\n> > Another question about the universal clearing mechanism, how does \"git\n> > config\" fit into this? It would be great if the user can actually see\n> > the same thing a git command sees to troubleshoot. Option 1 leaves the\n> > interpretation/guessing to the user, \"git config\" simply gives the raw\n> > input list before \"clear\" is processed. Option 2, \"git config\"\n> > probably can be taught to optionally clear when it sees the boolean\n> > syntax.\n>\n> We can make it fancier, but we already deal with this, e.g. if you do\n> \"git config -l\" we'll show \"include{,if}\" directives at the same \"level\"\n> as other \"normal\" keys.\n>\n> We also provide no way in \"git config\" to properly interpret a\n> value. E.g. does a \"user.email\" showing up twice for me mean I have two\n> E-Mails at the same time, or does the last one win?\n\nActually --get knows this. Single-valued options can be handled\ncorrectly quite easily. It's --get-all (or rather, the future\n--get-multi because we can't change --get-all's behavior) which can't\ninterpret values because there's no standardized way of doing it.\n\n> We both know the\n> answer, but git-config itself doesn't, and that information lives in\n> docs/code outside of it.\n>\n> Similarly we'd just print a sequence of:\n>\n>     user.name=foo\n>     user.email=bar\n>     exclude.key=user.*\n>     user.name=baz\n>\n> And it would be up to some \"smarter\" reader of the config data to\n> realize that the end result is one where we have no \"user.email\" set,\n> and \"user.name=baz\".\n>\n> But yeah, optionally having some new --list-normalized or\n> --list-after-excludes or whatever would be great, and presumably not\n> hard if we had some central \"excludes\" mechanism...\n-- \nDuy\n"},{"id":"374789","messageId":"20190501210944.GA15902@sigill.intra.peff.net","threadId":"50980","inReplyTo":"CACsJy8CSPBbxrJm1sh4a+f_B03MFXxDXSrOvb6yFCRa4Qa2wMQ@mail.gmail.com","subject":"Re: How to undo previously set configuration? (again)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-01T21:09:44Z","receivedAt":"2019-05-01T21:13:31Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 01, 2019 at 07:32:20PM +0700, Duy Nguyen wrote:\n\n> > We also provide no way in \"git config\" to properly interpret a\n> > value. E.g. does a \"user.email\" showing up twice for me mean I have two\n> > E-Mails at the same time, or does the last one win?\n> \n> Actually --get knows this. Single-valued options can be handled\n> correctly quite easily. It's --get-all (or rather, the future\n> --get-multi because we can't change --get-all's behavior) which can't\n> interpret values because there's no standardized way of doing it.\n\nRight. We need a hint from the caller about how they expect us to\ninterpret the values. And I agree we should probably introduce a new\nverb instead of modifying --get-all, which some callers might be\nexpecting to do their own list processing.\n\nLikewise in the C code. We probably want to leave existing callers of\ngit_config_get_value_multi() alone, and give them a new\ngit_config_get_string_list() or something.\n\n-Peff\n"},{"id":"374790","messageId":"20190501211525.GB15902@sigill.intra.peff.net","threadId":"50980","inReplyTo":"87r29iqsf9.fsf@evledraar.gmail.com","subject":"Re: How to undo previously set configuration? (again)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-01T21:15:25Z","receivedAt":"2019-05-01T21:15:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 01, 2019 at 02:18:34PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> We can make it fancier, but we already deal with this, e.g. if you do\n> \"git config -l\" we'll show \"include{,if}\" directives at the same \"level\"\n> as other \"normal\" keys.\n\nWe show them, but we _do_ interpret them if the caller asks for it with\n--includes (which defaults to on when doing the usual \"look in all\nfiles\").\n\nI think we'd have something similar here, where the caller can ask to\napply excludes or not.\n\n> We also provide no way in \"git config\" to properly interpret a\n> value. E.g. does a \"user.email\" showing up twice for me mean I have two\n> E-Mails at the same time, or does the last one win? We both know the\n> answer, but git-config itself doesn't, and that information lives in\n> docs/code outside of it.\n> \n> Similarly we'd just print a sequence of:\n> \n>     user.name=foo\n>     user.email=bar\n>     exclude.key=user.*\n>     user.name=baz\n> \n> And it would be up to some \"smarter\" reader of the config data to\n> realize that the end result is one where we have no \"user.email\" set,\n> and \"user.name=baz\".\n> \n> But yeah, optionally having some new --list-normalized or\n> --list-after-excludes or whatever would be great, and presumably not\n> hard if we had some central \"excludes\" mechanism...\n\nI think that is all because \"--list\" really is just about dumping all\nvalues, not about interpreting. If we had an exclude mechanism, then I'd\nexpect:\n\n  git config user.name\n\nto apply it just like git_config_get_string() would.\n\nBecause of its lack of interpretation, I don't think --list is actually\ngood for much besides debugging. Some scripts do use it to avoid making\na bunch of individual git-config calls, but they'd be much better served\nby a --stdin mode which let you feed in a sequence of operations (\"I want\nx.y.z, as a single last-one-wins value, and interpreted as a bool\") and\nget a sequence of outputs.\n\n-Peff\n"}]}