Skip to content

hook: reset path before early hook lookup - #1014

Open
Tyrie Vella (tyrielv) wants to merge 1 commit into
microsoft:vfs-2.56.0from
tyrielv:vfs-fix-early-hook-path-reset
Open

Tyrie Vella (tyrielv) wants to merge 1 commit into
microsoft:vfs-2.56.0from
tyrielv:vfs-fix-early-hook-path-reset

Conversation

@tyrielv

Copy link
Copy Markdown

Problem and Context

find_hook() stores the resolved hook path in a static strbuf. If the repository has no gitdir yet, find_hook() calls hook_path_early(). That function appends the hooks directory and the hook name to the buffer, but it does not reset the buffer first. In v2.53.0.vfs.0.0, find_hook() called strbuf_reset(&path) before either branch. The rebase onto v2.54.0, which includes upstream b51e238 (hook: allow out-of-repo 'git hook' invocations), rewrote find_hook() and dropped that reset. The gap is present from v2.54.0.vfs.0.0 through vfs-2.56.0.

As a result, a process that finds pre-command before setup cannot find post-command later in the same process. The second lookup produces <hooks>/pre-command[.exe]<hooks>/post-command, so access() fails and Git skips the hook. Built-ins that set up the repository first are not affected, because repo_git_path_replace() resets the buffer. These paths are affected:

  • dashed external commands (git foo runs git-foo)
  • built-ins that do not set up the repository, for example git version
  • bare git with no command (the usage text)
  • a dashed external that is not found, after which Git falls back to an alias

VFS for Git takes a lock in pre-command and releases it in post-command. microsoft/VFSForGit#2139 observed a deadlock with 2.55.0.vfs.0.10. In that case, a test deleted an external git-<name> after pre-command ran and before Git dispatched it. Git then expanded the alias and started a child Git process. The parent never ran post-command, so the child waited for the parent's lock while the parent waited for the child.

postCommand.strategy=worktree-change is a separate way to skip post-command. VFS for Git does not set it, and the new tests set always explicitly.

Changes

  • hook.c: Reset the static path buffer in find_hook() before either the early or the gitdir branch.
  • Add a test to t0401-post-command-hook.sh that runs a dashed external command and asserts that pre-command and post-command both run for it.
  • Add a test to t0401-post-command-hook.sh that runs git version and asserts that pre-command and post-command both run for it.
  • Add a test to t0401-post-command-hook.sh that uses a pre-command hook to delete a dashed external before Git dispatches it. The test asserts that Git falls back to the alias and that the hook order is pre git-vanish, post git-vanish, pre rev-parse, post rev-parse.
  • The new tests write their own hooks and remove them afterwards, so they do not depend on the hooks of earlier tests.

Hook consumers will see post-command invocations that these releases skipped. For a dashed external that is not found, the hook receives --exit_code=4294967295, because exit_code is -1 and Git prints it with %u. That formatting is unchanged by this PR. Bare git gets --exit_code=1.

Known gaps and deliberate decisions

  • Relative core.hooksPath from a subdirectory: hook_path_early() resolves a relative core.hooksPath against the current directory, not the worktree root. Early hook lookups from a subdirectory then find no hooks. This behavior exists before this change and does not change here. No follow-up is planned yet.
  • This is an early version of work already under review upstream.
  • This change only applies to interactions with Azure DevOps and the
    GVFS Protocol.
  • This change only applies to the virtualization hook and VFS for Git.

find_hook() keeps the resolved path in a static strbuf. If the
repository has no gitdir yet, it calls hook_path_early(). That function
appends the hooks directory and the hook name to the buffer, but it does
not reset the buffer first. In v2.53.0.vfs.0.0, find_hook() called
strbuf_reset() before either branch. The rebase onto v2.54.0, which
includes upstream b51e238 (hook: allow out-of-repo 'git hook'
invocations), rewrote find_hook() and dropped that reset.

Because of this, a process that finds the pre-command hook before the
gitdir is set up cannot find post-command later in the same process. The
second lookup appends to the first path, so access() fails and the hook
is skipped. This affects dashed external commands, built-ins that do
not set up the repository (for example 'git version'), and the case
where a dashed external is not found and Git falls back to an alias.

VFS for Git takes a lock in pre-command and releases it in post-command.
If an external disappears between pre-command and dispatch, Git falls
back to the alias and starts a child Git process. The parent never runs
post-command, so the child waits for the parent's lock and the parent
waits for the child.

Reset the buffer before either branch. Add tests that check the
pre-command and post-command order for a dashed external command, for
'git version', and for a dashed external that disappears before the
alias fallback.

Assisted-by: Claude Opus 5.5
Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
@tyrielv

Copy link
Copy Markdown
Author

Note

🤖 Machine-drafted comment, reviewed and approved by Tyrie Vella (@tyrielv) before posting.

Johannes Schindelin (@dscho) The v2.54 rebase dropped the strbuf_reset() in find_hook() when it took upstream b51e238, and this PR restores it. How can we keep it from being dropped again at the next rebase? Should it be squashed into "gvfs: add global command pre and post hook procs", or is a separate commit better?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant