hooks: skip post-command hook more often #813
Conversation
|
So with this change, if you have only a I guess this makes sense. It does mean that if you wanted a hook to, say, aid with typos or commands outside of a repo, then you couldn't use a |
If But as I write this reply, I realize that aliases or dash-commands may have some special behavior that must be accounted for. I'm investigating now.
The post-command hook only works within a |
derrickstolee
force-pushed
the
post-command-trivial
branch
from
56bdb28 to
58bc228
Compare
|
Thanks for the quick looks, but the alias scenario showed me some interesting complications. I'm pretty happy with the solution now, so please re-review! There are two commits now, where the first is only adding tests to demonstrate untested behavior. The second makes the behavior change and demonstrates how that impacts the new tests. |
derrickstolee
force-pushed
the
post-command-trivial
branch
from
58bc228 to
2242917
Compare
dscho
left a comment
dscho
left a comment
There was a problem hiding this comment.
I really like the structure and presentation of this PR!
derrickstolee
force-pushed
the
post-command-trivial
branch
from
2242917 to
ce221b0
Compare
When the top-level Git process is an alias, it doesn't load much config and thus the postCommand.strategy config setting is not loaded properly. This leads to the post-command hook being run more frequently than we want, including an alias for 'git add' running the hook even when the worktree did not change. Similar state is not loaded by 'git version' or 'git typo'. Signed-off-by: Derrick Stolee <stolee@gmail.com>
derrickstolee
force-pushed
the
post-command-trivial
branch
from
ce221b0 to
60ea01f
Compare
emilyyang-ms
left a comment
emilyyang-ms
left a comment
There was a problem hiding this comment.
The 2 commits demonstrated the PR really well, thanks!
wilbaker
left a comment
wilbaker
left a comment
There was a problem hiding this comment.
Thanks for handling the alias case as well!
I left one small question about formatting, thanks!
When the post-command hook runs, we could be in several custom states: 1. The command is a regular builtin, but the repo setup is incomplete. (Example: "git version", "git rev-parse HEAD") 2. The command is a dashed command, but the top level process uses a space in its call. (Example: "git http-request") 3. The command is an alias that goes to a builtin. 4. The command has no arguments or is only helptext. Each of these cases leads to a place where we previously had not loaded the postCommand.strategy config and would execute the post-command hook without looking for a sentinel file. There are two fixes here: First, we use early config parsing so we can get config details without fully setting up the repository. This is how core.hooksPath works in these situations. Second, we need to make sure handle_hook_replacement() is called even when the repo's gitdir is NULL. This requires a small amount of care to say that a sentinel file cannot exist if the gitdir isn't set, as we would not have written one without initializing the gitdir. This gets all of the desired behaviors we want for this setting without doing anything extreme with how pre- and post-command hooks run otherwise. Signed-off-by: Derrick Stolee <stolee@gmail.com>
derrickstolee
force-pushed
the
post-command-trivial
branch
from
60ea01f to
2734329
Compare
The postCommand.strategy=worktree-change config option allows for skipping the post-command hook when the Git command doesn't change the worktree. However, sometimes this config isn't loaded before the post-command hook is invoked, causing the hook to run in cases where we'd prefer it to not run. Examples include: `git version` or `git <typo>`. The tricky bit is that there are several places where we get here and standard config mechanisms can't load due to not having a `gitdir` in the repository struct. We fix this by: 1. Using the "load early config" mechanism also used by `core.hooksPath`. 2. Skipping the lookup for the sentinel file when there isn't a `gitdir` since we couldn't have written one without it. The change is given in two commits: the first expands the tests with the existing behavior and the second changes the behavior, showing the impact on those tests. * [X] This change only applies to microsoft/git specifics (post-command hook) See #736 and #748 for prior art in this space.
5 participants
The postCommand.strategy=worktree-change config option allows for skipping the post-command hook when the Git command doesn't change the worktree. However, sometimes this config isn't loaded before the post-command hook is invoked, causing the hook to run in cases where we'd prefer it to not run.
Examples include:
git versionorgit <typo>.The tricky bit is that there are several places where we get here and standard config mechanisms can't load due to not having a
gitdirin the repository struct. We fix this by:core.hooksPath.gitdirsince we couldn't have written one without it.The change is given in two commits: the first expands the tests with the existing behavior and the second changes the behavior, showing the impact on those tests.
See #736 and #748 for prior art in this space.