Skip to content
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

Fix prepare_commit.sh #54918

Merged
merged 3 commits into from Oct 11, 2023
Merged

Fix prepare_commit.sh #54918

merged 3 commits into from Oct 11, 2023

Conversation

strk
Copy link
Contributor

@strk strk commented Oct 11, 2023

This was broken with 135c4cb
this commit fixes it

@strk
Copy link
Contributor Author

strk commented Oct 11, 2023

It was actually broken by b84393e in that a grep not matching anything results in the whole pipeline evaluating to false and set -e at the beginning of the file makes that a failure that interrupts the script and prevents commit

@strk
Copy link
Contributor Author

strk commented Oct 11, 2023

And b84393e comes from #54893

@strk
Copy link
Contributor Author

strk commented Oct 11, 2023

@troopa81 a commit in this PR also updates instruction as to how to properly set the pre-commit hook for it to work in any worktree, in case that's what prevented you from properly testing it (it surely prevented me)

@strk strk enabled auto-merge (rebase) October 11, 2023 16:07
@strk strk changed the title Fix run of shellcheck in prepare_commit.sh Fix prepare_commit.sh Oct 11, 2023
@strk strk added the Chore GitHub and other CI infrastructure changes label Oct 11, 2023
@troopa81
Copy link
Contributor

@troopa81 a commit in this PR also updates instruction as to how to properly set the pre-commit hook for it to work in any worktree, in case that's what prevented you from properly testing it (it surely prevented me)

My hook is correctly set at the root of my repository and I'm not sure the modification on CONTRIBUTING.md are worthy (it brings more complexity).

I didn't tested your modification. Same here, I didn't tested but it looks good to me.

@strk strk merged commit c8d00d0 into qgis:master Oct 11, 2023
30 of 31 checks passed
@strk strk deleted the prepare-commit-shellcheck branch October 11, 2023 21:40
@strk
Copy link
Contributor Author

strk commented Oct 11, 2023

@troopa81 "correctly set" how ? Making pre-commit a symlink as suggested in CONTRIBUTING.md before this change would not be "correctly set" as it would always run the prepare-commit.sh script from the branch checked out in the "toplevel" git repository, and never in other worktrees, where you might be experimenting with changes to that script (as I was doing).

@troopa81
Copy link
Contributor

@strk you're right, thanks for the tips

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Chore GitHub and other CI infrastructure changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants