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

Fixup / Autosquash commits #4

Closed
DanielVoogsgerd opened this issue Oct 21, 2021 · 6 comments
Closed

Fixup / Autosquash commits #4

DanielVoogsgerd opened this issue Oct 21, 2021 · 6 comments

Comments

@DanielVoogsgerd
Copy link
Contributor

Hi, I love the pre-commit hook.

I have one small issue with it though. It rejects fixup / squash commits.

fixup / squash / amend commits are only intended for local development, but allow you to amend a commit further in the past than your current HEAD and can be trivially processed using. git rebase --autosquash. This is a really handy feature of git, but it will create commits of the following pattern:

fixup! Commit message of the old commit to apply the change to

squash! Commit message of the old commit to apply the change to

amend! Commit message of the old commit to apply the change to

Obviously, these are rejected by the pre-commit hook. Yet it would be nice if there would at least be an option to allow them until you push

@DanielVoogsgerd
Copy link
Contributor Author

As referenced above: I created a PR with a proposed solution.

@thekaveman
Copy link
Member

@DanielVoogsgerd thank you so much for a thoughtful issue write-up and proposed solution! We're glad the pre-commit hook has been useful.

Your use-case seems really valid, I too love a good git rebase. And I I have literally never heard of or seen usage of the
git commit --fixup-style commits, thank you for enlightening me!

As you noted though, this does take us away from strictly Conventional Commits formatting, something that I would like this hook to stay as close to as possible. So unfortunately I won't be able to accept #5 as-is (more below, please keep reading!)

I want to point out that a git-based workaround for #5 does exist, and that is the --no-verify flag:

Before

$ git commit --fixup HEAD
Conventional Commit......................................................Failed
- hook id: conventional-pre-commit
- exit code: 1

[Commit message] fixup! feat: previous commit message

Your commit message does not follow Conventional Commits formatting
https://www.conventionalcommits.org/

...

After

$ git commit --fixup HEAD --no-verify
[main 25fde30] fixup! feat: previous commit message
 1 file changed, 1 insertion(+), 1 deletion(-)

I tend to use this flag for exactly the use-case that you describe, revising a commit far in the history, purely in a local sense before I clean up the history for a push to remote. I was always doing it the git rebase -i way, and your approach seems like a great addition!

I'm going to leave some more comments directly on #5 because I do like part of that PR and would welcome the change - perhaps to setup your other proposal for #3.

Thank you again!

@DanielVoogsgerd
Copy link
Contributor Author

Thanks for the elaborate response!
I did not know about --no-verify. That solves my problem of not being able to use fixup commits. The only problem I might have with using it is the lack of granularity. It will skip all commit hooks which might not be a good idea as other suppressed commit hooks can then be triggered mid rebase. However a more elegant solution would probably be to make it optional to allow these types of commits. But implementing that might be a bit of a pain considering how the arguments are used for additional commit types.

Lets close this issue for now and see if problems arise from using --no-verify.

@bektaskemal
Copy link

For further reference, here is the solution I came up with. It uses no-verify but then runs pre-commit manually after rebase.
https://gist.github.com/bektaskemal/6d3144fc4a28daa0812162ab02f0b6f7

@JP-Ellis
Copy link

JP-Ellis commented Mar 28, 2023

I don't really want to revive an old thread and I understand @thekaveman's reasoning to not include this; however, I do find myself often doing a fixup! or squash! forgetting I have the pre-commit hook, and then having to rerun the command with --no-verify. I also think the use of fixup! and squash! should be made possible as they are a part of git's --autosquash feature.

As a compromise to a blanket allow, would there be a willingness to allow this behind an option? I'm thinking that the YAML definition could be extended as follows:

repos:

# - repo: ...

  - repo: https://github.com/compilerla/conventional-pre-commit
    rev: <git sha or tag>
    hooks:
      - id: conventional-pre-commit
        stages: [commit-msg]
        args: [--allow-autosquash]

and the function signature could be changed to

from conventional_pre_commit.format import is_conventional

# prints True
print(is_conventional("feat: this is a conventional commit"))

# prints False
print(is_conventional("fixup! feat: this is a conventional commit"))

# prints False
print(is_conventional("fixup! feat: this is a conventional commit", allow_autosquash=True))

The default would remain false in both cases so there would be no change in the behaviour of existing code, but should a team wish to allow this, it can be enabled without difficulty.

Let me know if this is something you would be open to and I can send through a PR.


Note that ideally, I would prefer it if the YAML definition became:

repos:

# - repo: ...

  - repo: https://github.com/compilerla/conventional-pre-commit
    rev: <git sha or tag>
    hooks:
      - id: conventional-pre-commit
        stages: [commit-msg]
        args: []
        allow-autosquash: true

but it would appear that pre-commit only allows for customisations to be passed through the args key unfortunately. In any case, I don't see a huge issue as someone could have:

repos:

# - repo: ...

  - repo: https://github.com/compilerla/conventional-pre-commit
    rev: <git sha or tag>
    hooks:
      - id: conventional-pre-commit
        stages: [commit-msg]
        args:
          - fix
          - feat
          - custom
          - --allow-autosquash
        allow-autosquash: true

and I doubt anyone would be confused, or anyone would be actually using --allow-autosquash as a commit type.


Another alternative I think is possible would be to hide the --allow-autosquash flag behind a new hook within this repository:

repos:

# - repo: ...

  - repo: https://github.com/compilerla/conventional-pre-commit
    rev: <git sha or tag>
    hooks:
      - id: conventional-pre-commit-autosquash
        stages: [commit-msg]
        args: []

This would functionally be the same as what I suggested above, but it at least avoids the ugliness of mixing the args.

@thekaveman
Copy link
Member

@DanielVoogsgerd @bektaskemal @JP-Ellis the latest version v2.4.0 now supports Fixup / Autosquash commits by default.

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

Successfully merging a pull request may close this issue.

4 participants