-
Notifications
You must be signed in to change notification settings - Fork 280
Default to subrepo option to use --no-verify #660
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,6 +77,7 @@ file= Specify a commit message file | |
| r,remote= Specify the upstream remote to push/pull/fetch | ||
| s,squash Squash commits on push | ||
| u,update Add the --branch and/or --remote overrides to .gitrepo | ||
| V,verify Run pre-commit and commit-msg hooks (default: skip hooks) | ||
|
|
||
| q,quiet Show minimal output | ||
| v,verbose Show verbose output | ||
|
|
@@ -100,6 +101,7 @@ main() { | |
| 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) | ||
|
|
||
| local quiet_wanted=false # Output should be quiet | ||
| local verbose_wanted=false # Output should be verbose | ||
|
|
@@ -366,7 +368,7 @@ command:config() { | |
| # shellcheck disable=2154 | ||
| o "Update '$subdir' configuration with $config_option=${config_value-}" | ||
|
|
||
| if [[ ! $config_option =~ ^(branch|cmdver|commit|method|remote|version)$ ]]; then | ||
| if [[ ! $config_option =~ ^(branch|cmdver|commit|method|remote|verify|version)$ ]]; then | ||
| error "Option $config_option not recognized" | ||
| fi | ||
|
|
||
|
|
@@ -377,8 +379,8 @@ command:config() { | |
| fi | ||
|
|
||
| if ! $force_wanted; then | ||
| # Only allow changing method without force | ||
| if [[ $config_option != method ]]; then | ||
| # Only allow changing method and verify without force | ||
| if [[ $config_option != method && $config_option != verify ]]; then | ||
| error "This option is autogenerated, use '--force' to override." | ||
| fi | ||
| fi | ||
|
|
@@ -389,6 +391,12 @@ command:config() { | |
| fi | ||
| fi | ||
|
|
||
| if [[ $config_option == verify ]]; then | ||
| if [[ ! $config_value =~ ^(true|false)$ ]]; then | ||
| error "Not a valid verify value. Valid options are 'true' or 'false'." | ||
| fi | ||
| fi | ||
|
|
||
| RUN git config --file="$gitrepo" "subrepo.$config_option" "$config_value" | ||
| say "Subrepo '$subdir' option '$config_option' set to '$config_value'." | ||
| } | ||
|
|
@@ -531,7 +539,10 @@ subrepo:init() { | |
|
|
||
| o "Commit new subrepo to the '$original_head_branch' branch." | ||
| subrepo_commit_ref=$original_head_commit | ||
| RUN git commit -m "$(get-commit-message)" | ||
| local no_verify_flag=' --no-verify' | ||
| $verify_wanted && no_verify_flag='' | ||
| # shellcheck disable=2086 | ||
| RUN git commit$no_verify_flag -m "$(get-commit-message)" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
|
||
| o "Create ref '$refs_subrepo_commit'." | ||
| git:make-ref "$refs_subrepo_commit" "$subrepo_commit_ref" | ||
|
|
@@ -710,10 +721,14 @@ subrepo:push() { | |
| commit_message=$(get-commit-message) | ||
| fi | ||
|
|
||
| local no_verify_flag=' --no-verify' | ||
| $verify_wanted && no_verify_flag='' | ||
| 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" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| fi | ||
| } | ||
|
|
||
|
|
@@ -919,12 +934,17 @@ subrepo:commit() { | |
| local edit_flag= | ||
| $edit_wanted && edit_flag=--edit | ||
|
|
||
| local no_verify_flag=' --no-verify' | ||
| $verify_wanted && no_verify_flag='' | ||
|
|
||
| o "Commit to the '$original_head_branch' branch." | ||
| if [[ $original_head_commit != none ]]; then | ||
| 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" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| fi | ||
| else | ||
| # We had cloned into an empty repo, side effect of prior git reset --mixed | ||
|
|
@@ -1116,6 +1136,7 @@ get-command-options() { | |
| -s) squash_wanted=true ;; | ||
| -u) update_wanted=true | ||
| commit_msg_args+=("--update") ;; | ||
| -V) verify_wanted=true ;; | ||
| -q) quiet_wanted=true ;; | ||
| -v) verbose_wanted=true ;; | ||
| -d) debug_wanted=true ;; | ||
|
|
@@ -1152,7 +1173,7 @@ get-command-options() { | |
| fi | ||
| commit_msg_args+=("${command_arguments[@]}") | ||
|
|
||
| for option in all ALL edit fetch force squash; do | ||
| for option in all ALL edit fetch force squash verify; do | ||
| var=${option}_wanted | ||
| if ${!var}; then | ||
| check_option $option | ||
|
|
@@ -1179,13 +1200,13 @@ get-command-options() { | |
| options_help='all' | ||
| options_branch='all fetch force' | ||
| options_clean='ALL all force' | ||
| options_clone='branch edit force message method' | ||
| options_clone='branch edit force message method verify' | ||
| options_config='force' | ||
| options_commit='edit fetch force message' | ||
| options_commit='edit fetch force message verify' | ||
| options_fetch='all branch force remote' | ||
| options_init='branch remote method' | ||
| options_pull='all branch edit force message remote update' | ||
| options_push='all branch force message remote squash update' | ||
| options_init='branch remote method verify' | ||
| options_pull='all branch edit force message remote update verify' | ||
| options_push='all branch force message remote squash update verify' | ||
| options_status='ALL all fetch' | ||
| check_option() { | ||
| local var=options_${command//-/_} | ||
|
|
@@ -1443,6 +1464,15 @@ read-gitrepo-file() { | |
| join_method=merge | ||
| fi | ||
|
|
||
| # Read verify config if not already set by command line | ||
| if ! $verify_wanted; then | ||
| FAIL=false \ | ||
| SAY=false OUT=true RUN git config --file="$gitrepo" subrepo.verify | ||
| if [[ $output == true ]]; then | ||
| verify_wanted=true | ||
| fi | ||
| fi | ||
|
|
||
| if [[ -z $subrepo_parent ]]; then | ||
| FAIL=false \ | ||
| SAY=false OUT=true RUN git config --file="$gitrepo" subrepo.former | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| #!/usr/bin/env bash | ||
|
|
||
| set -e | ||
|
|
||
| source test/setup | ||
|
|
||
| use Test::More | ||
|
|
||
| clone-foo-and-bar | ||
|
|
||
| # Create a directory to init as subrepo | ||
| { | ||
| ( | ||
| cd "$OWNER/foo" | ||
| mkdir -p testdir | ||
| echo "test content" > testdir/testfile | ||
| git add testdir/testfile | ||
| git commit -m "Add testdir" | ||
| ) &> /dev/null || die | ||
| } | ||
|
|
||
| # Create a pre-commit hook that will fail | ||
| { | ||
| hook_file="$OWNER/foo/.git/hooks/pre-commit" | ||
| mkdir -p "$(dirname "$hook_file")" | ||
| cat > "$hook_file" <<'EOF' | ||
| #!/bin/bash | ||
| # This hook will always fail | ||
| echo "Pre-commit hook triggered and failing" | ||
| exit 1 | ||
| EOF | ||
| chmod +x "$hook_file" | ||
| } | ||
|
|
||
| # Test that init without --verify succeeds (hooks bypassed by default) | ||
| { | ||
| init_status=0 | ||
| init_output=$( | ||
| cd "$OWNER/foo" | ||
| git subrepo init testdir 2>&1 | ||
| ) || init_status=$? | ||
|
|
||
| is "$init_status" "0" \ | ||
| 'subrepo init succeeds by default (hooks bypassed)' | ||
|
|
||
| unlike "$init_output" "Pre-commit hook triggered" \ | ||
| 'pre-commit hook was bypassed by default' | ||
|
|
||
| like "$init_output" "Subrepo created from 'testdir'" \ | ||
| 'init output is correct' | ||
| } | ||
|
|
||
| # Test that the .gitrepo file was created | ||
| { | ||
| test-exists "$OWNER/foo/testdir/.gitrepo" | ||
| } | ||
|
|
||
| # Clean up for next test | ||
| { | ||
| ( | ||
| cd "$OWNER/foo" | ||
| git reset --hard HEAD~1 | ||
| rm -f testdir/.gitrepo | ||
| ) &> /dev/null || true | ||
| } | ||
|
|
||
| # Test that init with --verify runs the hook and fails | ||
| { | ||
| init_status=0 | ||
| init_output=$( | ||
| cd "$OWNER/foo" | ||
| git subrepo init --verify testdir 2>&1 | ||
| ) || init_status=$? | ||
|
|
||
| isnt "$init_status" "0" \ | ||
| 'subrepo init with --verify fails when pre-commit hook fails' | ||
|
|
||
| like "$init_output" "Pre-commit hook triggered and failing" \ | ||
| 'pre-commit hook was triggered with --verify' | ||
| } | ||
|
|
||
| done_testing | ||
|
|
||
| teardown |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| #!/usr/bin/env bash | ||
|
|
||
| set -e | ||
|
|
||
| source test/setup | ||
|
|
||
| use Test::More | ||
|
|
||
| clone-foo-and-bar | ||
|
|
||
| # Create a pre-commit hook that will fail | ||
| { | ||
| hook_file="$OWNER/foo/.git/hooks/pre-commit" | ||
| mkdir -p "$(dirname "$hook_file")" | ||
| cat > "$hook_file" <<'EOF' | ||
| #!/bin/bash | ||
| # This hook will always fail | ||
| echo "Pre-commit hook triggered and failing" | ||
| exit 1 | ||
| EOF | ||
| chmod +x "$hook_file" | ||
| } | ||
|
|
||
| # Test that clone without --verify succeeds (hooks bypassed by default) | ||
| { | ||
| clone_status=0 | ||
| clone_output=$( | ||
| cd "$OWNER/foo" | ||
| git subrepo clone "$UPSTREAM/bar" 2>&1 | ||
| ) || clone_status=$? | ||
|
|
||
| is "$clone_status" "0" \ | ||
| 'subrepo clone succeeds by default (hooks bypassed)' | ||
|
|
||
| unlike "$clone_output" "Pre-commit hook triggered" \ | ||
| 'pre-commit hook was bypassed by default' | ||
|
|
||
| is "$clone_output" \ | ||
| "Subrepo '$UPSTREAM/bar' (master) cloned into 'bar'." \ | ||
| 'clone output is correct' | ||
| } | ||
|
|
||
| # Test that the subrepo was actually cloned | ||
| { | ||
| test-exists \ | ||
| "$OWNER/foo/bar/" \ | ||
| "$OWNER/foo/bar/Bar" \ | ||
| "$OWNER/foo/bar/.gitrepo" | ||
| } | ||
|
|
||
| # Clean up for next test | ||
| { | ||
| ( | ||
| cd "$OWNER/foo" | ||
| git reset --hard HEAD~1 | ||
| rm -rf bar | ||
| ) &> /dev/null || true | ||
| } | ||
|
|
||
| # Test that clone with --verify runs the hook and fails | ||
| { | ||
| clone_status=0 | ||
| clone_output=$( | ||
| cd "$OWNER/foo" | ||
| git subrepo clone --verify "$UPSTREAM/bar" 2>&1 | ||
| ) || clone_status=$? | ||
|
|
||
| isnt "$clone_status" "0" \ | ||
| 'subrepo clone with --verify fails when pre-commit hook fails' | ||
|
|
||
| like "$clone_output" "Pre-commit hook triggered and failing" \ | ||
| 'pre-commit hook was triggered with --verify' | ||
| } | ||
|
|
||
| done_testing | ||
|
|
||
| teardown |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.