Default to subrepo option to use --no-verify #660
Conversation
There was a problem hiding this comment.
The idea is sounds, but as stated in the inline comments I would much rather have the default be that hooks run and subrepos must be configured to disable hooks. I would also prefer that there is only a setting the .gitrepo file if hooks are disabled.
I am ok with allowing a git config to set a default value for running to hooks. I think it would only apply to git subrepo clone and not override the behavior for existing subrepos.
I did not review the tests yet since I am asking for significant changes to how this would work.
| local fetch_wanted=false # Fetch requested before a command | ||
| local squash_wanted=false # Squash commits on push | ||
| local update_wanted=false # Update .gitrepo with --branch and/or --remote | ||
| local verify_wanted=false # Run pre-commit and commit-msg hooks (default: skip) |
There was a problem hiding this comment.
I have a lot of subrepos that use pre-commit (the tool) and work fine. I feel that it would be better to default to verifying and only disable verifying if explicitly configured. I would even be ok with adding a git config to globally set you want verify disabled.
| local no_verify_flag=' --no-verify' | ||
| $verify_wanted && no_verify_flag='' | ||
| # shellcheck disable=2086 | ||
| RUN git commit$no_verify_flag -m "$(get-commit-message)" |
There was a problem hiding this comment.
To prevent disabling 2086 this should use Parameter Expansion https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html#Shell-Parameter-Expansion-1
| if [[ $commit_msg_file ]]; then | ||
| RUN git command --file "$commit_msg_file" | ||
| # shellcheck disable=2086 | ||
| RUN git commit$no_verify_flag --file "$commit_msg_file" |
There was a problem hiding this comment.
To prevent disabling 2086 this should use Parameter Expansion https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html#Shell-Parameter-Expansion-1
| else | ||
| RUN git commit -m "$commit_message" | ||
| # shellcheck disable=2086 | ||
| RUN git commit$no_verify_flag -m "$commit_message" |
There was a problem hiding this comment.
To prevent disabling 2086 this should use Parameter Expansion https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html#Shell-Parameter-Expansion-1
| if [[ $commit_msg_file ]]; then | ||
| RUN git commit $edit_flag --file "$commit_msg_file" | ||
| # shellcheck disable=2086 | ||
| RUN git commit$no_verify_flag $edit_flag --file "$commit_msg_file" |
There was a problem hiding this comment.
To prevent disabling 2086 this should use Parameter Expansion https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html#Shell-Parameter-Expansion-1
| else | ||
| RUN git commit $edit_flag -m "$commit_message" | ||
| # shellcheck disable=2086 | ||
| RUN git commit$no_verify_flag $edit_flag -m "$commit_message" |
There was a problem hiding this comment.
To prevent disabling 2086 this should use Parameter Expansion https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html#Shell-Parameter-Expansion-1
|
The issue was not on the pre-commit hook inside the subrepo, but the one in the main repo. E.g., say there's a per-commit hook in the main repo, and say perhaps there's a python code formatter there If I were to now clone a subrepo git subrepo clone SOME_REPO_WITH_PYTHON_CODE new_subrepoThen if the python code formatter did changed the newly cloned subrepo, the whole repo is now at a dirty state. git-subrepo: Command failed: 'git commit -m git subrepo clone ../py
subrepo:
subdir: "py"
merged: "12641cf"
upstream:
origin: "../py"
branch: "master"
commit: "12641cf"
git-subrepo:
version: "0.4.9"
origin: "???"
commit: "???"'.
ruff format..............................................................Failed
- hook id: ruff-format
- files were modified by this hook
1 file reformattedIts at a non-recoverable state. If you want a clean history (of git subrepo clone), then you will need to (i) revert the change, (ii) manually disable your pre-commit hook, then (iii) perform the above clone, (iv) enable your pre-commit hook again, and then (v) perhaps peforms the code formatting in your subrepo to ensure consistency. In my opinion, it is non-consistent, it leaves things in a dirty state, and intuitively, the hook that people expect to run (if at all) should be
See #193 (comment) for the prior discussion. Since the hooks inside subrepo cannot be the ones that actually get executed, I don't see why the default behaviour should be to verify |
|
I understand that this is the repo containing the subrepo, and that is a feature. The git hooks are doing their job and should not allow you to accept code that doesn't pass their muster. As I said I have many repos that contain subrepos and both the subrepo and the containing repo have very aggressive hooks. I am happy to see that you are using Your (and the other issues) argument as I read it is that hooks should be disabled because it is inconvenient when they fail on "3rd party code". I counter argue that your hooks are not properly configured. Your hooks should understand they shouldn't run in git subrepos (there is porcelain commands to get a list of subrepos). After sleeping on it, I have completely changed my mind on this patch. I now believe (but obviously my mind can be changed) that the follow things are true.
In summary I would accept a patch that adds a --no-verify flag that behaves like git's, but not what was proposed here. Bonus material: To disable containing repo hooks for subrepo in pre-commit add to the top of the file: # Exclude code that is maintained outside this repo
exclude: >
(?x)^(
3rdparty/zlib/.*|
3rdpart/sqlite3.*
)$
repos:
- repo <path to clang-format>
rev:: latest
hooks:
- id: clang-format
exclude: ^$If you use something like clang-format you can also configure the tool to do the right thing, so either disable it for those directories in the tool configuration or better, submit a .clang-format configuration to the upstream project and leave it enabled and doing the right thing. Do remember that that repositories hooks won't run when you make changes to the containing repo in that directory, so when you git subrepo push you may have to fix up your patches to follow the rules of that repo. |
|
Hi thank you for the very detail write out (and including your reflection as well). I don't agree partially with some of your view points. Points not explicitly mentioned below could be view as agreement with you:
This part is true
The part about the containing repo has aggressive hooks has no bearing on the whole issue at all, as the inner hooks were never respected (I presume this part is clear to you, but mentioning them might muddle the focus on hooks that actually matters here (parent/root), especially to others stumble to this issue)
They are more than just inconvenient, they actually are confusing and for someone who don't fully understand the inner working of
Don't agree with it, with details below. But other methods like .gitattributes had always affect the user of the repo.
It requires the changing of hooks, which could also be part of other well established infrastructure.
This point is misleading, because hooks actually requires you to manually installed. Hence there's no "disabling" as enabling is not automatic. Not saying that I agree nor disagree whether hooks should be automatically enabled or not. But the argument that "official git has no way to declare disabling hooks, so git-subrepo should too" is invalid.
I reckon this first depends on the philosophy of
Thanks for the pointers on the exclude on solving pre-commit.config. It is indeed a solution, tho only for that kind of hooks |
2 participants
This closes #193, and potentially adresses #323
Since the clone / pull / ... command in
git subrepoare operating on a different repo, it often doesn't make much sense to trigger the hook, e.g., pre-commit hooks in the main repo.This PR defaults to use
--no-verifyflag on these related operation, and the behaviour can also be configured viaThe
.gitrepofile: