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

add check on pre-push stage: exit if staged files not committed yet #3113

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

aless10
Copy link

@aless10 aless10 commented Jan 18, 2024

This PR aims to solve the issue #2486. The idea is to follow the same solution used for #1418 and add the check on the hook name and if there are staged files not committed yet.

About the error message
I did not want to print the list of the staged files, because it can too long and not very useful. I used logger.error to print the message, following what is done in the other checks above.

Alternatives:

  • instead of using the function git.get_staged_files, we can just check for the number of staged files and fail if it is > 0, something like git diff --cached --numstat | wc -l

fixes #2486

Comment on lines +1227 to +1223
def test_pre_push_fails_if_staged_files(
cap_out, store, repo_with_passing_hook,
):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test is probably sufficient -- then you don't need the additional one above

Comment on lines 374 to 376
if (
args.hook_stage == 'pre-push' and git.get_staged_files()
):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the extra parens here do nothing

Comment on lines 377 to 379
logger.error(
'Staged files found. Please commit before pushing',
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this easily fits on one line

.gitignore Outdated
@@ -4,3 +4,4 @@
/.tox
/dist
.vscode/
.idea
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don't touch gitignore files in projects you don't own -- please rebase this out thanks!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about that. Removed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

pre-push should forbid unrelated staged changes while pushing
2 participants