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

Ignore commit amend message files in .git when running forbid-tabs & co by default #74

Closed
xaver-k opened this issue May 16, 2023 · 7 comments

Comments

@xaver-k
Copy link
Contributor

xaver-k commented May 16, 2023

When amending to an existing commit, forbid-tabs detects tabs in the git/COMMIT_EDITMSG and fails the check. The tab is part of the message that is auto-generated by git to inform about the history of the commit, so it is not the user's fault.

Example output:

$ git commit --amend
No-tabs checker..........................................................Failed 
- hook id: forbid-tabs
- exit code: 1

Tabs detected in file: .git/COMMIT_EDITMSG 

Example git message:

title

original message

# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
#
# Date:      Mon Apr 24 12:40:35 2023 +0200
#
# On branch xxxxxxxxxxxxxxx
#
# Changes to be committed:
#       modified:   ../some_file.hpp   <- This line has a tab in the terminal, but github helpfully replaces tabs with spaces....
#

The file should probably be ignored by the hook by default. However, I am not sure what the best approach for that would be here.
Options are to either ignoring the specific file (might not be enough in similar scenarios) OR a set of specific files (which ones) or everything in .git (might break some use-cases).

@Lucas-C
Copy link
Owner

Lucas-C commented May 17, 2023

Hi @xaver-k!

I agree that this would make sense.

You can already use the exclude entry in .pre-commit-config.yaml to ignore those files:
https://pre-commit.com/#regular-expressions

We could also probably add this entry in the https://github.com/Lucas-C/pre-commit-hooks/blob/master/.pre-commit-hooks.yaml file in this repository

@xaver-k
Copy link
Contributor Author

xaver-k commented May 20, 2023

Hi @Lucas-C
Interestingly I cannot reproduce the issue with a fresh git repository on my private machine, even though I could easily reproduce it at work on multiple machines. So I will need to investigate the root-cause a little further. Since I am on holiday right now and do not have access to my work-machine, that will first be in 2,5 weeks. I will keep you updated with what I find.

Regarding a fix: I would prefer to fix the root cause here via a PR and just bump the version in all of my repositories instead of maintaining a workaround through a custom exclude in each repo (and I am using your hooks in a lot of repos, thank you for providing them, by the way 😄 )

@Lucas-C
Copy link
Owner

Lucas-C commented May 21, 2023

Alright, no hurry!
Thank you for investigating this.

I wish nice holidays!

And sure, a PR would be welcome

@xaver-k
Copy link
Contributor Author

xaver-k commented Jun 6, 2023

So, I found the root-cause now and have a reproduce: https://github.com/xaver-k/no-tabs-pre-commit-hook-bug-reporduce

The issue is that the hook does not explicitly defines which stages to run at (see https://pre-commit.com/#hooks-stages), so it just runs for all stages. This means that in our case, it is also run at the commit-msg-stage and therefore checks the message.

If it works for you, I would make a PR that confines the hooks to only run at the following stages:

For the other stages, I do not see a usecase for checking tabs / line endings:

@Lucas-C
Copy link
Owner

Lucas-C commented Jun 6, 2023

Makes sense!

I'm not even sure the pre-push is required

@Lucas-C
Copy link
Owner

Lucas-C commented Jun 7, 2023

Closing as #75 was merged

Thank you @xaver-k!

@Lucas-C Lucas-C closed this as completed Jun 7, 2023
@xaver-k
Copy link
Contributor Author

xaver-k commented Jun 7, 2023

I'm not even sure the pre-push is required

Just for completeness, I found the hooks useful at the pre-push-stage when developers clone a repo, forget to set up pre-commit, make some dirty commits and then remember / are told and enable pre-commit.
Probably an edge-case, but nice to have, that's why I included that stage in the PR.

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

No branches or pull requests

2 participants