From e7d349d6bbc5c1941855bcda6699b05d6634fd89 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 7 May 2025 09:26:07 -0400 Subject: [PATCH 1/5] fixup! hooks: add custom post-command hook config This change to use the 'info/' directory instead of the 'hooks/' directory is important because the hooks dir could be redirected to the source tree. Modify the test in such a way that this code change would fail due to being unable to write to the internal hooks directory. Signed-off-by: Derrick Stolee --- hook.c | 6 +++++- t/t0401-post-command-hook.sh | 12 +++++++++--- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/hook.c b/hook.c index 67bc0f65c036d8..72ce5545cf0e06 100644 --- a/hook.c +++ b/hook.c @@ -190,7 +190,11 @@ static char *get_post_index_change_sentinel_name(struct repository *r) if (slash) *slash = 0; - repo_git_path_replace(r, &path, "hooks/index-change-%s.snt", sid); + /* + * Do not write to hooks directory, as it could be redirected + * somewhere like the source tree. + */ + repo_git_path_replace(r, &path, "info/index-change-%s.snt", sid); return strbuf_detach(&path, NULL); } diff --git a/t/t0401-post-command-hook.sh b/t/t0401-post-command-hook.sh index 4da639e754504f..b1e93e2b79f6f3 100755 --- a/t/t0401-post-command-hook.sh +++ b/t/t0401-post-command-hook.sh @@ -32,14 +32,20 @@ test_expect_success 'with failing pre-command hook' ' ' test_expect_success 'with post-index-change config' ' - mkdir -p .git/hooks && - write_script .git/hooks/post-command <<-EOF && + mkdir -p internal-hooks && + write_script internal-hooks/post-command <<-EOF && echo ran >post-command.out EOF - write_script .git/hooks/post-index-change <<-EOF && + write_script internal-hooks/post-index-change <<-EOF && echo ran >post-index-change.out EOF + # prevent writing of sentinel files to this directory. + test_when_finished chmod 775 internal-hooks && + chmod a-w internal-hooks && + + git config core.hooksPath internal-hooks && + # First, show expected behavior. echo ran >expect && rm -f post-command.out post-index-change.out && From 3eb2bd0700e236009ffaa51935be6876ae53ac04 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 7 May 2025 09:59:39 -0400 Subject: [PATCH 2/5] hook: extract the internal hook replacement logic Before making the post-index-change logic more advanced for the postCommand.strategy=post-index-change feature, extract the code into a new method that will be easier to update in the future. Signed-off-by: Derrick Stolee --- hook.c | 40 +++++++++++++++++++++++++++++++--------- 1 file changed, 31 insertions(+), 9 deletions(-) diff --git a/hook.c b/hook.c index 72ce5545cf0e06..885cfe1f70b96a 100644 --- a/hook.c +++ b/hook.c @@ -233,6 +233,34 @@ static int post_index_change_sentinel_exists(struct repository *r) return res; } +/** + * See if we can replace the requested hook with an internal behavior. + * Returns 0 if the real hook should run. Returns nonzero if we instead + * executed custom internal behavior and the value to return is set to + * 'result'. + */ +static int handle_hook_replacement(struct repository *r, + const char *hook_name, + int *result) +{ + const char *strval; + if (repo_config_get_string_tmp(r, "postcommand.strategy", &strval) || + strcasecmp(strval, "post-index-change")) + return 0; + + if (!strcmp(hook_name, "post-index-change")) { + *result = write_post_index_change_sentinel(r); + return 1; + } + if (!strcmp(hook_name, "post-command") && + !post_index_change_sentinel_exists(r)) { + *result = 0; + return 1; + } + + return 0; +} + int run_hooks_opt(struct repository *r, const char *hook_name, struct run_hooks_opt *options) { @@ -260,15 +288,9 @@ int run_hooks_opt(struct repository *r, const char *hook_name, /* Interject hook behavior depending on strategy. */ if (r && r->gitdir) { - const char *strval; - if (!repo_config_get_string_tmp(r, "postcommand.strategy", &strval) && - !strcasecmp(strval, "post-index-change")) { - if (!strcmp(hook_name, "post-index-change")) - return write_post_index_change_sentinel(r); - if (!strcmp(hook_name, "post-command") && - !post_index_change_sentinel_exists(r)) - return 0; - } + int result = 0; + if (handle_hook_replacement(r, hook_name, &result)) + return result; } hook_path = find_hook(r, hook_name); From 8c178e8d9b1641fcfcef26c828888eb14114475c Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 7 May 2025 10:10:05 -0400 Subject: [PATCH 3/5] hook: restrict post-command hook even more The postCommand.strategy=post-index-change config was designed to replace some internal hook usage. This internal hook was more restrictive than the initial feature was designed. Specifically, it ignored index updates that did not update the worktree. With the existing implementation, this causes more work done in the post-command hook than intended. Update the code to write a sentinel file only when the worktree is changed. Signed-off-by: Derrick Stolee --- Documentation/config/postcommand.adoc | 2 +- hook.c | 10 ++++++++-- t/t0401-post-command-hook.sh | 8 +++++++- 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/Documentation/config/postcommand.adoc b/Documentation/config/postcommand.adoc index b1425cec989b08..bef334683f970b 100644 --- a/Documentation/config/postcommand.adoc +++ b/Documentation/config/postcommand.adoc @@ -9,5 +9,5 @@ postCommand.strategy:: `post-index-change`;; run the `post-command` hook only if the current process wrote to - the index. + the index and updated the worktree. ---- diff --git a/hook.c b/hook.c index 885cfe1f70b96a..1536bd1d9a2a15 100644 --- a/hook.c +++ b/hook.c @@ -241,6 +241,7 @@ static int post_index_change_sentinel_exists(struct repository *r) */ static int handle_hook_replacement(struct repository *r, const char *hook_name, + struct strvec *args, int *result) { const char *strval; @@ -249,7 +250,11 @@ static int handle_hook_replacement(struct repository *r, return 0; if (!strcmp(hook_name, "post-index-change")) { - *result = write_post_index_change_sentinel(r); + /* Create a sentinel file only if the worktree changed. */ + if (!strcmp(args->v[0], "1")) + *result = write_post_index_change_sentinel(r); + else + *result = 0; return 1; } if (!strcmp(hook_name, "post-command") && @@ -289,7 +294,8 @@ int run_hooks_opt(struct repository *r, const char *hook_name, /* Interject hook behavior depending on strategy. */ if (r && r->gitdir) { int result = 0; - if (handle_hook_replacement(r, hook_name, &result)) + if (handle_hook_replacement(r, hook_name, + &options->args, &result)) return result; } diff --git a/t/t0401-post-command-hook.sh b/t/t0401-post-command-hook.sh index b1e93e2b79f6f3..cea7933e03a8cc 100755 --- a/t/t0401-post-command-hook.sh +++ b/t/t0401-post-command-hook.sh @@ -72,9 +72,15 @@ test_expect_success 'with post-index-change config' ' test_path_is_missing post-command.out && echo stuff >>file && - # add updates the index and runs post-command. + # add keeps the worktree the same, so does not run post-command. git add file && test_path_is_missing post-index-change.out && + test_path_is_missing post-command.out && + + echo stuff >>file && + # reset --hard updates the worktree. + git reset --hard && + test_path_is_missing post-index-change.out && test_cmp expect post-command.out ' From e1cfb523e9c53e94650f6d115d92376d54377abd Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 7 May 2025 13:15:44 -0400 Subject: [PATCH 4/5] hooks: change postCommand.strategy config value The previously-named 'post-index-change' value for 'postCommand.strategy' did the following two things: 1. Avoid running the post-command hook unless the post-index-change hook _would_ run with a signal that the worktree changed. 2. Avoid running any installed post-index-change hooks. The additional restriction of the worktree change in (1) makes this name less appropriate. Also, item (2) seems to be a side-effect that we should avoid. We would like to allow both behaviors to exist. Update the value and behavior, including the tests. Signed-off-by: Derrick Stolee --- Documentation/config/postcommand.adoc | 2 +- hook.c | 7 +++++-- t/t0401-post-command-hook.sh | 10 ++++++---- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/Documentation/config/postcommand.adoc b/Documentation/config/postcommand.adoc index bef334683f970b..1ae7ce92f6ee22 100644 --- a/Documentation/config/postcommand.adoc +++ b/Documentation/config/postcommand.adoc @@ -7,7 +7,7 @@ postCommand.strategy:: `always`;; run the `post-command` hook on every process (default). -`post-index-change`;; +`worktree-change`;; run the `post-command` hook only if the current process wrote to the index and updated the worktree. ---- diff --git a/hook.c b/hook.c index 1536bd1d9a2a15..41ce69ebf6126e 100644 --- a/hook.c +++ b/hook.c @@ -246,7 +246,7 @@ static int handle_hook_replacement(struct repository *r, { const char *strval; if (repo_config_get_string_tmp(r, "postcommand.strategy", &strval) || - strcasecmp(strval, "post-index-change")) + strcasecmp(strval, "worktree-change")) return 0; if (!strcmp(hook_name, "post-index-change")) { @@ -255,10 +255,13 @@ static int handle_hook_replacement(struct repository *r, *result = write_post_index_change_sentinel(r); else *result = 0; - return 1; + + /* We don't skip post-index-change hooks that exist. */ + return 0; } if (!strcmp(hook_name, "post-command") && !post_index_change_sentinel_exists(r)) { + /* We skip the post-command hook in this case. */ *result = 0; return 1; } diff --git a/t/t0401-post-command-hook.sh b/t/t0401-post-command-hook.sh index cea7933e03a8cc..0cc41e6210b86e 100755 --- a/t/t0401-post-command-hook.sh +++ b/t/t0401-post-command-hook.sh @@ -63,24 +63,26 @@ test_expect_success 'with post-index-change config' ' test_cmp expect post-command.out && # Now, show configured behavior - git config postCommand.strategy post-index-change && - rm -f post-command.out post-index-change.out && + git config postCommand.strategy worktree-change && # rev-parse leaves index intact and thus skips post-command. + rm -f post-command.out post-index-change.out && git rev-parse HEAD && test_path_is_missing post-index-change.out && test_path_is_missing post-command.out && echo stuff >>file && # add keeps the worktree the same, so does not run post-command. + rm -f post-command.out post-index-change.out && git add file && - test_path_is_missing post-index-change.out && + test_cmp expect post-index-change.out && test_path_is_missing post-command.out && echo stuff >>file && # reset --hard updates the worktree. + rm -f post-command.out post-index-change.out && git reset --hard && - test_path_is_missing post-index-change.out && + test_cmp expect post-index-change.out && test_cmp expect post-command.out ' From aa035a2542b7e4b19b6ba042866d5750fc8087e7 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 7 May 2025 13:29:21 -0400 Subject: [PATCH 5/5] hooks: simplify handle_hook_replacement() Now that we don't skip the post-index-change hook, we don't need to pass the return value as an out-parameter in this method. Signed-off-by: Derrick Stolee --- hook.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/hook.c b/hook.c index 41ce69ebf6126e..be69f6a4527662 100644 --- a/hook.c +++ b/hook.c @@ -236,13 +236,11 @@ static int post_index_change_sentinel_exists(struct repository *r) /** * See if we can replace the requested hook with an internal behavior. * Returns 0 if the real hook should run. Returns nonzero if we instead - * executed custom internal behavior and the value to return is set to - * 'result'. + * executed custom internal behavior and the real hook should not run. */ static int handle_hook_replacement(struct repository *r, const char *hook_name, - struct strvec *args, - int *result) + struct strvec *args) { const char *strval; if (repo_config_get_string_tmp(r, "postcommand.strategy", &strval) || @@ -252,9 +250,7 @@ static int handle_hook_replacement(struct repository *r, if (!strcmp(hook_name, "post-index-change")) { /* Create a sentinel file only if the worktree changed. */ if (!strcmp(args->v[0], "1")) - *result = write_post_index_change_sentinel(r); - else - *result = 0; + write_post_index_change_sentinel(r); /* We don't skip post-index-change hooks that exist. */ return 0; @@ -262,7 +258,6 @@ static int handle_hook_replacement(struct repository *r, if (!strcmp(hook_name, "post-command") && !post_index_change_sentinel_exists(r)) { /* We skip the post-command hook in this case. */ - *result = 0; return 1; } @@ -295,12 +290,9 @@ int run_hooks_opt(struct repository *r, const char *hook_name, }; /* Interject hook behavior depending on strategy. */ - if (r && r->gitdir) { - int result = 0; - if (handle_hook_replacement(r, hook_name, - &options->args, &result)) - return result; - } + if (r && r->gitdir && + handle_hook_replacement(r, hook_name, &options->args)) + return 0; hook_path = find_hook(r, hook_name);