summaryrefslogtreecommitdiffstats
path: root/hook.c (follow)
Commit message (Collapse)AuthorAgeFilesLines
* Merge branch 'bf/explicit-config-set-in-advice-messages'Junio C Hamano2024-12-161-1/+1
|\ | | | | | | | | | | | | | | The advice messages now tell the newer 'git config set' command to set the advice.token configuration variable to squelch a message. * bf/explicit-config-set-in-advice-messages: advice: suggest using subcommand "git config set"
| * advice: suggest using subcommand "git config set"Bence Ferdinandy2024-12-061-1/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | The advice message currently suggests using "git config advice..." to disable advice messages, but since 00bbdde141 (builtin/config: introduce "set" subcommand, 2024-05-06) we have the "set" subcommand for config. Since using the subcommand is more in-line with the modern interface, any advice should be promoting its usage. Change the disable advice message to use the subcommand instead. Change all uses of "git config advice" in the tests to use the subcommand. Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | hooks: remove implicit dependency on `the_repository`Patrick Steinhardt2024-08-131-10/+11
|/ | | | | | | | | We implicitly depend on `the_repository` in our hook subsystem because we use `strbuf_git_path()` to compute hook paths. Remove this dependency by accepting a `struct repository` as parameter instead. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* Merge branch 'fixes/2.45.1/2.41' into fixes/2.45.1/2.42Junio C Hamano2024-05-251-33/+0
|\ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | * fixes/2.45.1/2.41: Revert "fsck: warn about symlink pointing inside a gitdir" Revert "Add a helper function to compare file contents" clone: drop the protections where hooks aren't run tests: verify that `clone -c core.hooksPath=/dev/null` works again Revert "core.hooksPath: add some protection while cloning" init: use the correct path of the templates directory again hook: plug a new memory leak ci: stop installing "gcc-13" for osx-gcc ci: avoid bare "gcc" for osx-gcc job ci: drop mention of BREW_INSTALL_PACKAGES variable send-email: avoid creating more than one Term::ReadLine object send-email: drop FakeTerm hack
| * Merge branch 'fixes/2.45.1/2.40' into fixes/2.45.1/2.41Junio C Hamano2024-05-251-33/+0
| |\ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | * fixes/2.45.1/2.40: Revert "fsck: warn about symlink pointing inside a gitdir" Revert "Add a helper function to compare file contents" clone: drop the protections where hooks aren't run tests: verify that `clone -c core.hooksPath=/dev/null` works again Revert "core.hooksPath: add some protection while cloning" init: use the correct path of the templates directory again hook: plug a new memory leak ci: stop installing "gcc-13" for osx-gcc ci: avoid bare "gcc" for osx-gcc job ci: drop mention of BREW_INSTALL_PACKAGES variable send-email: avoid creating more than one Term::ReadLine object send-email: drop FakeTerm hack
| | * Merge branch 'jc/fix-2.45.1-and-friends-for-2.39' into fixes/2.45.1/2.40Junio C Hamano2024-05-241-32/+0
| | |\ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Revert overly aggressive "layered defence" that went into 2.45.1 and friends, which broke "git-lfs", "git-annex", and other use cases, so that we can rebuild necessary counterparts in the open. * jc/fix-2.45.1-and-friends-for-2.39: Revert "fsck: warn about symlink pointing inside a gitdir" Revert "Add a helper function to compare file contents" clone: drop the protections where hooks aren't run tests: verify that `clone -c core.hooksPath=/dev/null` works again Revert "core.hooksPath: add some protection while cloning" init: use the correct path of the templates directory again hook: plug a new memory leak ci: stop installing "gcc-13" for osx-gcc ci: avoid bare "gcc" for osx-gcc job ci: drop mention of BREW_INSTALL_PACKAGES variable send-email: avoid creating more than one Term::ReadLine object send-email: drop FakeTerm hack
| | | * clone: drop the protections where hooks aren't runJohannes Schindelin2024-05-211-34/+0
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | As part of the security bug-fix releases v2.39.4, ..., v2.45.1, I introduced logic to safeguard `git clone` from running hooks that were installed _during_ the clone operation. The rationale was that Git's CVE-2024-32002, CVE-2021-21300, CVE-2019-1354, CVE-2019-1353, CVE-2019-1352, and CVE-2019-1349 should have been low-severity vulnerabilities but were elevated to critical/high severity by the attack vector that allows a weakness where files inside `.git/` can be inadvertently written during a `git clone` to escalate to a Remote Code Execution attack by virtue of installing a malicious `post-checkout` hook that Git will then run at the end of the operation without giving the user a chance to see what code is executed. Unfortunately, Git LFS uses a similar strategy to install its own `post-checkout` hook during a `git clone`; In fact, Git LFS is installing four separate hooks while running the `smudge` filter. While this pattern is probably in want of being improved by introducing better support in Git for Git LFS and other tools wishing to register hooks to be run at various stages of Git's commands, let's undo the clone protections to unbreak Git LFS-enabled clones. This reverts commit 8db1e8743c0 (clone: prevent hooks from running during a clone, 2024-03-28). Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| | | * hook: plug a new memory leakJohannes Schindelin2024-05-211-1/+3
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | In 8db1e8743c0 (clone: prevent hooks from running during a clone, 2024-03-28), I introduced an inadvertent memory leak that was unfortunately not caught before v2.45.1 was released. Here is a fix. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | | | Sync with 2.41.1Johannes Schindelin2024-04-191-7/+46
|\| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | * maint-2.41: (38 commits) Git 2.41.1 Git 2.40.2 Git 2.39.4 fsck: warn about symlink pointing inside a gitdir core.hooksPath: add some protection while cloning init.templateDir: consider this config setting protected clone: prevent hooks from running during a clone Add a helper function to compare file contents init: refactor the template directory discovery into its own function find_hook(): refactor the `STRIP_EXTENSION` logic clone: when symbolic links collide with directories, keep the latter entry: report more colliding paths t5510: verify that D/F confusion cannot lead to an RCE submodule: require the submodule path to contain directories only clone_submodule: avoid using `access()` on directories submodules: submodule paths must not contain symlinks clone: prevent clashing git dirs when cloning submodule in parallel t7423: add tests for symlinked submodule directories has_dir_name(): do not get confused by characters < '/' docs: document security issues around untrusted .git dirs ...
| * | | Sync with 2.40.2Johannes Schindelin2024-04-191-7/+46
| |\| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | * maint-2.40: (39 commits) Git 2.40.2 Git 2.39.4 fsck: warn about symlink pointing inside a gitdir core.hooksPath: add some protection while cloning init.templateDir: consider this config setting protected clone: prevent hooks from running during a clone Add a helper function to compare file contents init: refactor the template directory discovery into its own function find_hook(): refactor the `STRIP_EXTENSION` logic clone: when symbolic links collide with directories, keep the latter entry: report more colliding paths t5510: verify that D/F confusion cannot lead to an RCE submodule: require the submodule path to contain directories only clone_submodule: avoid using `access()` on directories submodules: submodule paths must not contain symlinks clone: prevent clashing git dirs when cloning submodule in parallel t7423: add tests for symlinked submodule directories has_dir_name(): do not get confused by characters < '/' docs: document security issues around untrusted .git dirs upload-pack: disable lazy-fetching by default ...
| | * | Sync with 2.39.4Johannes Schindelin2024-04-191-7/+43
| | |\| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | * maint-2.39: (38 commits) Git 2.39.4 fsck: warn about symlink pointing inside a gitdir core.hooksPath: add some protection while cloning init.templateDir: consider this config setting protected clone: prevent hooks from running during a clone Add a helper function to compare file contents init: refactor the template directory discovery into its own function find_hook(): refactor the `STRIP_EXTENSION` logic clone: when symbolic links collide with directories, keep the latter entry: report more colliding paths t5510: verify that D/F confusion cannot lead to an RCE submodule: require the submodule path to contain directories only clone_submodule: avoid using `access()` on directories submodules: submodule paths must not contain symlinks clone: prevent clashing git dirs when cloning submodule in parallel t7423: add tests for symlinked submodule directories has_dir_name(): do not get confused by characters < '/' docs: document security issues around untrusted .git dirs upload-pack: disable lazy-fetching by default fetch/clone: detect dubious ownership of local repositories ...
| | | * clone: prevent hooks from running during a cloneJohannes Schindelin2024-04-191-0/+32
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Critical security issues typically combine relatively common vulnerabilities such as case confusion in file paths with other weaknesses in order to raise the severity of the attack. One such weakness that has haunted the Git project in many a submodule-related CVE is that any hooks that are found are executed during a clone operation. Examples are the `post-checkout` and `fsmonitor` hooks. However, Git's design calls for hooks to be disabled by default, as only disabled example hooks are copied over from the templates in `<prefix>/share/git-core/templates/`. As a defense-in-depth measure, let's prevent those hooks from running. Obviously, administrators can choose to drop enabled hooks into the template directory, though, _and_ it is also possible to override `core.hooksPath`, in which case the new check needs to be disabled. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
| | | * find_hook(): refactor the `STRIP_EXTENSION` logicJohannes Schindelin2024-04-171-7/+11
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | When looking for a hook and not finding one, and when `STRIP_EXTENSION` is available (read: if we're on Windows and `.exe` is the required extension for executable programs), we want to look also for a hook with that extension. Previously, we added that handling into the conditional block that was meant to handle when no hook was found (possibly providing some advice for the user's benefit). If the hook with that file extension was found, we'd return early from that function instead of writing out said advice, of course. However, we're about to introduce a safety valve to prevent hooks from being run during a clone, to reduce the attack surface of bugs that allow writing files to be written into arbitrary locations. To prepare for that, refactor the logic to avoid the early return, by separating the `STRIP_EXTENSION` handling from the conditional block handling the case when no hook was found. This commit is best viewed with `--patience`. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
* | | | abspath: move related functions to abspathCalvin Wan2023-06-121-0/+1
|/ / / | | | | | | | | | | | | | | | | | | | | | | | | Move abspath-related functions from strbuf.[ch] to abspath.[ch] so that strbuf is focused on string manipulation routines with minimal dependencies. Signed-off-by: Calvin Wan <calvinwan@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | | hash-ll.h: split out of hash.h to remove dependency on repository.hElijah Newren2023-04-241-0/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | hash.h depends upon and includes repository.h, due to the definition and use of the_hash_algo (defined as the_repository->hash_algo). However, most headers trying to include hash.h are only interested in the layout of the structs like object_id. Move the parts of hash.h that do not depend upon repository.h into a new file hash-ll.h (the "low level" parts of hash.h), and adjust other files to use this new header where the convenience inline functions aren't needed. This allows hash.h and object.h to be fairly small, minimal headers. It also exposes a lot of hidden dependencies on both path.h (which was brought in by repository.h) and repository.h (which was previously implicitly brought in by object.h), so also adjust other files to be more explicit about what they depend upon. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | | Merge branch 'jk/unused-post-2.39-part2'Junio C Hamano2023-03-171-6/+6
|\ \ \ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | More work towards -Wunused. * jk/unused-post-2.39-part2: (21 commits) help: mark unused parameter in git_unknown_cmd_config() run_processes_parallel: mark unused callback parameters userformat_want_item(): mark unused parameter for_each_commit_graft(): mark unused callback parameter rewrite_parents(): mark unused callback parameter fetch-pack: mark unused parameter in callback function notes: mark unused callback parameters prio-queue: mark unused parameters in comparison functions for_each_object: mark unused callback parameters list-objects: mark unused callback parameters mark unused parameters in signal handlers run-command: mark error routine parameters as unused mark "pointless" data pointers in callbacks ref-filter: mark unused callback parameters http-backend: mark unused parameters in virtual functions http-backend: mark argc/argv unused object-name: mark unused parameters in disambiguate callbacks serve: mark unused parameters in virtual functions serve: use repository pointer to get config ls-refs: drop config caching ...
| * | | run_processes_parallel: mark unused callback parametersJeff King2023-02-241-6/+6
| |/ / | | | | | | | | | | | | | | | | | | | | | | | | Our parallel process API takes several callbacks via function pointers in the run_process_paralell_opts struct. Not every callback needs every parameter; let's mark the unused ones to make -Wunused-parameter happy. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* / / treewide: replace cache.h with more direct headers, where possibleElijah Newren2023-02-241-1/+4
|/ / | | | | | | | | Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* / hook API: support passing stdin to hooks, convert am's 'post-rewrite'Emily Shaffer2023-02-081-0/+5
|/ | | | | | | | | | | | | | | | Convert the invocation of the 'post-rewrite' hook run by 'git am' to use the hook.h library. To do this we need to add a "path_to_stdin" member to "struct run_hooks_opt". In our API this is supported by asking for a file path, rather than by reading stdin. Reading directly from stdin would involve caching the entire stdin (to memory or to disk) once the hook API is made to support "jobs" larger than 1, along with support for executing N hooks at a time (i.e. the upcoming config-based hooks). Signed-off-by: Emily Shaffer <emilyshaffer@google.com> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* run-command API: have run_process_parallel() take an "opts" structÆvar Arnfjörð Bjarmason2022-10-121-9/+14
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | As noted in fd3aaf53f71 (run-command: add an "ungroup" option to run_process_parallel(), 2022-06-07) which added the "ungroup" passing it to "run_process_parallel()" via the global "run_processes_parallel_ungroup" variable was a compromise to get the smallest possible regression fix for "maint" at the time. This follow-up to that is a start at passing that parameter and others via a new "struct run_process_parallel_opts", as the earlier version[1] of what became fd3aaf53f71 did. Since we need to change all of the occurrences of "n" to "opt->SOMETHING" let's take the opportunity and rename the terse "n" to "processes". We could also have picked "max_processes", "jobs", "threads" etc., but as the API is named "run_processes_parallel()" let's go with "processes". Since the new "run_processes_parallel()" function is able to take an optional "tr2_category" and "tr2_label" via the struct we can at this point migrate all of the users of "run_processes_parallel_tr2()" over to it. But let's not migrate all the API users yet, only the two users that passed the "ungroup" parameter via the "run_processes_parallel_ungroup" global 1. https://lore.kernel.org/git/cover-v2-0.8-00000000000-20220518T195858Z-avarab@gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* Merge branch 'ab/hooks-regression-fix'Junio C Hamano2022-08-151-7/+0
|\ | | | | | | | | | | | | A follow-up fix to a fix for a regression in 2.36. * ab/hooks-regression-fix: hook API: don't segfault on strbuf_addf() to NULL "out"
| * hook API: don't segfault on strbuf_addf() to NULL "out"Ævar Arnfjörð Bjarmason2022-08-051-7/+0
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Fix a logic error in a082345372e (hook API: fix v2.36.0 regression: hooks should be connected to a TTY, 2022-06-07). When it started using the "ungroup" API added in fd3aaf53f71 (run-command: add an "ungroup" option to run_process_parallel(), 2022-06-07) it should have made the same sort of change that fd3aaf53f71 itself made in "t/helper/test-run-command.c". The correct way to emit this "Couldn't start" output with "ungroup" would be: fprintf(stderr, _("Couldn't start hook '%s'\n"), hook_path); But we should instead remove the emitting of this output. As the added test shows we already emit output when we can't run the child. The "cannot run" output here is emitted by run-command.c's child_err_spew(). So the addition of the "Couldn't start hook" output here in 96e7225b310 (hook: add 'run' subcommand, 2021-12-22) was always redundant. For the pre-commit hook we'll now emit exactly the same output as we did before f443246b9f2 (commit: convert {pre-commit,prepare-commit-msg} hook to hook.h, 2021-12-22) (and likewise for others). We could at this point add this to the pick_next_hook() callbacks in hook.c: assert(!out); assert(!*pp_task_cb); And this to notify_start_failure() and notify_hook_finished() (in the latter case the parameter is called "pp_task_cp"): assert(!out); assert(!pp_task_cb); But let's leave any such instrumentation for some eventual cleanup of the "ungroup" API. Reported-by: Ilya K <me@0upti.me> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Emily Shaffer <emilyshaffer@google.com> Reviewed-by: Đoàn Trần Công Danh <congdanhqx@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | Merge branch 'ab/hooks-regression-fix'Junio C Hamano2022-06-141-0/+1
|\| | | | | | | | | | | | | | | | | | | | | In Git 2.36 we revamped the way how hooks are invoked. One change that is end-user visible is that the output of a hook is no longer directly connected to the standard output of "git" that spawns the hook, which was noticed post release. This is getting corrected. * ab/hooks-regression-fix: hook API: fix v2.36.0 regression: hooks should be connected to a TTY run-command: add an "ungroup" option to run_process_parallel()
| * hook API: fix v2.36.0 regression: hooks should be connected to a TTYÆvar Arnfjörð Bjarmason2022-06-071-0/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Fix a regression reported[1] against f443246b9f2 (commit: convert {pre-commit,prepare-commit-msg} hook to hook.h, 2021-12-22): Due to using the run_process_parallel() API in the earlier 96e7225b310 (hook: add 'run' subcommand, 2021-12-22) we'd capture the hook's stderr and stdout, and thus lose the connection to the TTY in the case of e.g. the "pre-commit" hook. As a preceding commit notes GNU parallel's similar --ungroup option also has it emit output faster. While we're unlikely to have hooks that emit truly massive amounts of output (or where the performance thereof matters) it's still informative to measure the overhead. In a similar "seq" test we're now ~30% faster: $ cat .git/hooks/seq-hook; git hyperfine -L rev origin/master,HEAD~0 -s 'make CFLAGS=-O3' './git hook run seq-hook' #!/bin/sh seq 100000000 Benchmark 1: ./git hook run seq-hook' in 'origin/master Time (mean ± σ): 787.1 ms ± 13.6 ms [User: 701.6 ms, System: 534.4 ms] Range (min … max): 773.2 ms … 806.3 ms 10 runs Benchmark 2: ./git hook run seq-hook' in 'HEAD~0 Time (mean ± σ): 603.4 ms ± 1.6 ms [User: 573.1 ms, System: 30.3 ms] Range (min … max): 601.0 ms … 606.2 ms 10 runs Summary './git hook run seq-hook' in 'HEAD~0' ran 1.30 ± 0.02 times faster than './git hook run seq-hook' in 'origin/master' 1. https://lore.kernel.org/git/CA+dzEBn108QoMA28f0nC8K21XT+Afua0V2Qv8XkR8rAeqUCCZw@mail.gmail.com/ Reported-by: Anthony Sottile <asottile@umich.edu> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> [jc: minor fix-up to tests for consistency] Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | run-command API: rename "env_array" to "env"Ævar Arnfjörð Bjarmason2022-06-021-1/+1
|/ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Start following-up on the rename mentioned in c7c4bdeccf3 (run-command API: remove "env" member, always use "env_array", 2021-11-25) of "env_array" to "env". The "env_array" name was picked in 19a583dc39e (run-command: add env_array, an optional argv_array for env, 2014-10-19) because "env" was taken. Let's not forever keep the oddity of "*_array" for this "struct strvec", but not for its "args" sibling. This commit is almost entirely made with a coccinelle rule[1]. The only manual change here is in run-command.h to rename the struct member itself and to change "env_array" to "env" in the CHILD_PROCESS_INIT initializer. The rest of this is all a result of applying [1]: * make contrib/coccinelle/run_command.cocci.patch * patch -p1 <contrib/coccinelle/run_command.cocci.patch * git add -u 1. cat contrib/coccinelle/run_command.pending.cocci @@ struct child_process E; @@ - E.env_array + E.env @@ struct child_process *E; @@ - E->env_array + E->env I've avoided changing any comments and derived variable names here, that will all be done in the next commit. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* hooks: fix an obscure TOCTOU "did we just run a hook?" raceÆvar Arnfjörð Bjarmason2022-03-071-0/+7
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Fix a Time-of-check to time-of-use (TOCTOU) race in code added in 680ee550d72 (commit: skip discarding the index if there is no pre-commit hook, 2017-08-14). This obscure race condition can occur if we e.g. ran the "pre-commit" hook and it modified the index, but hook_exists() returns false later on (e.g., because the hook itself went away, the directory became unreadable, etc.). Then we won't call discard_cache() when we should have. The race condition itself probably doesn't matter, and users would have been unlikely to run into it in practice. This problem has been noted on-list when 680ee550d72 was discussed[1], but had not been fixed. This change is mainly intended to improve the readability of the code involved, and to make reasoning about it more straightforward. It wasn't as obvious what we were trying to do here, but by having an "invoked_hook" it's clearer that e.g. our discard_cache() is happening because of the earlier hook execution. Let's also change this for the push-to-checkout hook. Now instead of checking if the hook exists and either doing a push to checkout or a push to deploy we'll always attempt a push to checkout. If the hook doesn't exist we'll fall back on push to deploy. The same behavior as before, without the TOCTOU race. See 0855331941b (receive-pack: support push-to-checkout hook, 2014-12-01) for the introduction of the previous behavior. This leaves uses of hook_exists() in two places that matter. The "reference-transaction" check in refs.c, see 67541597670 (refs: implement reference transaction hook, 2020-06-19), and the "prepare-commit-msg" hook, see 66618a50f9c (sequencer: run 'prepare-commit-msg' hook, 2018-01-24). In both of those cases we're saving ourselves CPU time by not preparing data for the hook that we'll then do nothing with if we don't have the hook. So using this "invoked_hook" pattern doesn't make sense in those cases. The "reference-transaction" and "prepare-commit-msg" hook also aren't racy. In those cases we'll skip the hook runs if we race with a new hook being added, whereas in the TOCTOU races being fixed here we were incorrectly skipping the required post-hook logic. 1. https://lore.kernel.org/git/20170810191613.kpmhzg4seyxy3cpq@sigill.intra.peff.net/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* hooks: convert worktree 'post-checkout' hook to hook libraryEmily Shaffer2022-01-081-0/+8
| | | | | | | | | | | | | | | | | | Move the running of the 'post-checkout' hook away from run-command.h to the new hook.h library in builtin/worktree.c. For this special case we need a change to the hook API to teach it to run the hook from a given directory. We cannot skip the "absolute_path" flag and just check if "dir" is specified as we'd then fail to find our hook in the new dir we'd chdir() to. We currently don't have a use-case for running a hook not in our "base" repository at a given absolute path, so let's have "dir" imply absolute_path(find_hook(hook_name)). Signed-off-by: Emily Shaffer <emilyshaffer@google.com> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Acked-by: Emily Shaffer <emilyshaffer@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* hook API: add a run_hooks_l() wrapperÆvar Arnfjörð Bjarmason2022-01-081-0/+14
| | | | | | | | | | Add a run_hooks_l() wrapper, we'll use it in subsequent commits for the simple cases of wanting to run a single hook under a given name along with a list of arguments. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Emily Shaffer <emilyshaffer@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* hook API: add a run_hooks() wrapperÆvar Arnfjörð Bjarmason2022-01-081-0/+7
| | | | | | | | | | Add a run_hooks() wrapper, we'll use it in subsequent commits for the simple cases of wanting to run a single hook under a given name, without providing options such as "env" or "args". Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Emily Shaffer <emilyshaffer@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* hook: add 'run' subcommandEmily Shaffer2022-01-081-0/+102
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | In order to enable hooks to be run as an external process, by a standalone Git command, or by tools which wrap Git, provide an external means to run all configured hook commands for a given hook event. Most of our hooks require more complex functionality than this, but let's start with the bare minimum required to support our simplest hooks. In terms of implementation the usage_with_options() and "goto usage" pattern here mirrors that of builtin/{commit-graph,multi-pack-index}.c. Some of the implementation here, such as a function being named run_hooks_opt() when it's tasked with running one hook, to using the run_processes_parallel_tr2() API to run with jobs=1 is somewhere between a bit odd and and an overkill for the current features of this "hook run" command and the hook.[ch] API. This code will eventually be able to run multiple hooks declared in config in parallel, by starting out with these names and APIs we reduce the later churn of renaming functions, switching from the run_command() to run_processes_parallel_tr2() API etc. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Acked-by: Emily Shaffer <emilyshaffer@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* hook.c: add a hook_exists() wrapper and use it in bugreport.cEmily Shaffer2021-09-271-0/+5
| | | | | | | | | | Add a boolean version of the find_hook() function for those callers who are only interested in checking whether the hook exists, not what the path to it is. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* hook.[ch]: move find_hook() from run-command.c to hook.cÆvar Arnfjörð Bjarmason2021-09-271-0/+37
Move the find_hook() function from run-command.c to a new hook.c library. This change establishes a stub library that's pretty pointless right now, but will see much wider use with Emily Shaffer's upcoming "configuration-based hooks" series. Eventually all the hook related code will live in hook.[ch]. Let's start that process by moving the simple find_hook() function over as-is. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>