hook: mark non-parallelizable hooks

Several hooks are known to be inherently non-parallelizable, so initialize
them with RUN_HOOKS_OPT_INIT_FORCE_SERIAL. This pins jobs=1 and overrides
any hook.jobs or runtime -j flags.

These hooks are:
applypatch-msg, pre-commit, prepare-commit-msg, commit-msg, post-commit,
post-checkout, and push-to-checkout.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/Documentation/config/hook.adoc b/Documentation/config/hook.adoc
index 94c7a9808e29eff98210be69686c3ef3a77d0c8c..6f60775c28a90218efeae2447d90ab6381998c15 100644 (file)
--- a/Documentation/config/hook.adoc
+++ b/Documentation/config/hook.adoc
@@ -36,6 +36,20 @@ hook.<friendly-name>.parallel::
 hook.jobs::
        Specifies how many hooks can be run simultaneously during parallelized
        hook execution. If unspecified, defaults to 1 (serial execution).
+       Some hooks always run sequentially regardless of this setting because
+       they operate on shared data and cannot safely be parallelized:
++
+--
+`applypatch-msg`;;
+`prepare-commit-msg`;;
+`commit-msg`;;
+       Receive a commit message file and may rewrite it in place.
+`pre-commit`;;
+`post-checkout`;;
+`push-to-checkout`;;
+`post-commit`;;
+       Access the working tree, index, or repository state.
+--
 +
 This setting has no effect unless all configured hooks for the event have
 `hook.<friendly-name>.parallel` set to `true`.

diff --git a/builtin/am.c b/builtin/am.c
index fe6e087eee9ff5223a690ee31e770b8248e82a3a..e9623b8307793f56f10c53525f3f13a5512f7436 100644 (file)
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -490,9 +490,11 @@ static int run_applypatch_msg_hook(struct am_state *state)
 
        assert(state->msg);
 
-       if (!state->no_verify)
-               ret = run_hooks_l(the_repository, "applypatch-msg",
-                                 am_path(state, "final-commit"), NULL);
+       if (!state->no_verify) {
+               struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_FORCE_SERIAL;
+               strvec_push(&opt.args, am_path(state, "final-commit"));
+               ret = run_hooks_opt(the_repository, "applypatch-msg", &opt);
+       }
 
        if (!ret) {
                FREE_AND_NULL(state->msg);

diff --git a/builtin/checkout.c b/builtin/checkout.c
index e031e6188613a6ceede132f4c3fe5023af36e6e9..ac0186a33e559a0ae2e6a9e63ae53373c58d3c1a 100644 (file)
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -31,6 +31,7 @@
 #include "resolve-undo.h"
 #include "revision.h"
 #include "setup.h"
+#include "strvec.h"
 #include "submodule.h"
 #include "symlinks.h"
 #include "trace2.h"
@@ -123,13 +124,19 @@ static void branch_info_release(struct branch_info *info)
 static int post_checkout_hook(struct commit *old_commit, struct commit *new_commit,
                              int changed)
 {
-       return run_hooks_l(the_repository, "post-checkout",
-                          oid_to_hex(old_commit ? &old_commit->object.oid : null_oid(the_hash_algo)),
-                          oid_to_hex(new_commit ? &new_commit->object.oid : null_oid(the_hash_algo)),
-                          changed ? "1" : "0", NULL);
-       /* "new_commit" can be NULL when checking out from the index before
-          a commit exists. */
+       struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_FORCE_SERIAL;
 
+       /*
+        * "new_commit" can be NULL when checking out from the index before
+        * a commit exists.
+        */
+       strvec_pushl(&opt.args,
+                    oid_to_hex(old_commit ? &old_commit->object.oid : null_oid(the_hash_algo)),
+                    oid_to_hex(new_commit ? &new_commit->object.oid : null_oid(the_hash_algo)),
+                    changed ? "1" : "0",
+                    NULL);
+
+       return run_hooks_opt(the_repository, "post-checkout", &opt);
 }
 
 static int update_some(const struct object_id *oid, struct strbuf *base,

diff --git a/builtin/clone.c b/builtin/clone.c
index fba3c9c508bc0635fa4bf5b9f4ed8bedfb1c4b7c..d23b0cafcfec308e15453253fdddfccd970b8e74 100644 (file)
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -647,6 +647,7 @@ static int checkout(int submodule_progress,
        struct tree *tree;
        struct tree_desc t;
        int err = 0;
+       struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT_FORCE_SERIAL;
 
        if (option_no_checkout)
                return 0;
@@ -697,8 +698,9 @@ static int checkout(int submodule_progress,
        if (write_locked_index(the_repository->index, &lock_file, COMMIT_LOCK))
                die(_("unable to write new index file"));
 
-       err |= run_hooks_l(the_repository, "post-checkout", oid_to_hex(null_oid(the_hash_algo)),
-                          oid_to_hex(&oid), "1", NULL);
+       strvec_pushl(&hook_opt.args, oid_to_hex(null_oid(the_hash_algo)),
+                    oid_to_hex(&oid), "1", NULL);
+       err |= run_hooks_opt(the_repository, "post-checkout", &hook_opt);
 
        if (!err && (option_recurse_submodules.nr > 0)) {
                struct child_process cmd = CHILD_PROCESS_INIT;

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index dada55884a0b0690aaa69cd6027ab0e0d30d6d9a..6da60f640ce3af21f11309a09d09b638170892fa 100644 (file)
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1455,7 +1455,8 @@ static const char *push_to_checkout(unsigned char *hash,
                                    struct strvec *env,
                                    const char *work_tree)
 {
-       struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
+       struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_FORCE_SERIAL;
+
        opt.invoked_hook = invoked_hook;
 
        strvec_pushf(env, "GIT_WORK_TREE=%s", absolute_path(work_tree));

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 4fd6f7575f9f764e3e30889e32a40ed45616c668..d21c43fde38b5e64a7ae1a3bbc4b5d81cb26244a 100644 (file)
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -609,7 +609,7 @@ done:
         * is_junk is cleared, but do return appropriate code when hook fails.
         */
        if (!ret && opts->checkout && !opts->orphan) {
-               struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
+               struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_FORCE_SERIAL;
 
                strvec_pushl(&opt.env, "GIT_DIR", "GIT_WORK_TREE", NULL);
                strvec_pushl(&opt.args,

diff --git a/commit.c b/commit.c
index 80d8d078757dbc4fd534bc038f4774850e57bf6c..4385ae4329e92170fae9ebcf36dde6dcf33c4934 100644 (file)
--- a/commit.c
+++ b/commit.c
@@ -1970,7 +1970,7 @@ size_t ignored_log_message_bytes(const char *buf, size_t len)
 int run_commit_hook(int editor_is_used, const char *index_file,
                    int *invoked_hook, const char *name, ...)
 {
-       struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
+       struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_FORCE_SERIAL;
        va_list args;
        const char *arg;
 

diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
index 4a978aff5e0c1e364d54c4462c3417198d8d3758..63fa25bca23c512d9c93217e55ca6aaf912bc782 100755 (executable)
--- a/t/t1800-hook.sh
+++ b/t/t1800-hook.sh
@@ -832,4 +832,20 @@ test_expect_success 'client hooks: pre-push runs in parallel when hook.jobs > 1'
        test_cmp expect repo-parallel/hook.order
 '
 
+test_expect_success 'hook.jobs=2 is ignored for force-serial hooks (pre-commit)' '
+       test_when_finished "rm -f sentinel.started sentinel.done hook.order" &&
+       test_config hook.hook-1.event pre-commit &&
+       test_config hook.hook-1.command \
+           "touch sentinel.started; sleep 2; touch sentinel.done" &&
+       test_config hook.hook-1.parallel true &&
+       test_config hook.hook-2.event pre-commit &&
+       test_config hook.hook-2.command \
+           "$(sentinel_detector sentinel hook.order)" &&
+       test_config hook.hook-2.parallel true &&
+       test_config hook.jobs 2 &&
+       git commit --allow-empty -m "test: verify force-serial on pre-commit" &&
+       echo serial >expect &&
+       test_cmp expect hook.order
+'
+
 test_done