gitlint
gitlint copied to clipboard
jorisroovers
Implement contrib rule to block fixup/squash commits
Found #139, figured that it is a low-hanging fruit and went and harvested that low-hanging fruit.
Closes #139
Added some fixups as well as tests and docs. :tada: I hope that lives up to your requirements (I think I did not miss anything).
I'm not sure whether I should also include a block for the the amend! keyword, that seems to be recognized by git as well. Tell me what you think...
I am wondering now whether this should really be a single rule, or 2 rules (one for fixup and one for squash). I can see someone might want to selectively enable them. I can similarly see someone might also want contribute similar disallow contrib rules for merge (commit.is_merge_commit) or revert commits (commit.is_revert_commit). I also think separate rules would be better than a rule option to toggle the behavior.
I thought about that as well, and I think no in case of "fixup" and "squash" and yes in case of "merge" and "revert".
Rationale being that "fixup" and "squash" are commits that are made because some previous commit in the PR needs cleanup. "fixup" is only a specialized "squash" here: "squash" gives the rebaser (person doing the rebase, normally the author of the PR) the option to rewrite the commit message. "fixup" is the same, but without the option to rewrite the commit message. Equally, "amend" is a "squash" but without a diff. I would group them, as I do not see why anyone (any maintainer) ever would want to keep a "squash" message, but not a "fixup" message (or the other way round). It just does not make any sense! Same for "amend" obviously.
Wrt !amend, is that something that can really be present in a commit message?
I didn't know about "amend!" commits either, but it was brought to my attention not a long time ago. If you have a look at the --autosquash option of git-rebase you'll see that "amend!" is actually something that git-rebase can work with (relevant git-commit doc).
I would expect any commit marked as !amend during rebase to just be amended to the previous commit. After the rebase, that commit no longer exists on its own and hence there's nothing gitlint can verify? Maybe I'm wrong here, would need to try.
Yes, all of these commits ("fixup!", "squash!", "amend!") should normally be rebased and squashed before merge, that's why I wrote this rule: so I can block PRs from being merged as long as there are fixup/squash/amend commits in the PR history.
Wrt is_merge_commit/is_revert_commit: Yes, I can add a rule for these (one for each) if you like!
Should I do that in another PR or in this one?
I would group them, as I do not see why anyone (any maintainer) ever would want to keep a "squash" message, but not a "fixup" message (or the other way round).
👍 , I can follow the reasoning. It's also a contrib rule, I'm less pendantic about those :)
I think we should be adding !amend then as well. However, I wonder whether we should change the name then. DisallowFixupSquashAmendCommit(Tests) doesn't exactly roll off the tongue. At the same time, it's probably more descriptive for users than something like DisallowSpecialCommits. Thoughts?
Wrt is_merge_commit/is_revert_commit: Yes, I can add a rule for these (one for each) if you like! Should I do that in another PR or in this one?
Great! Separate PRs for each rule please.
I'm out of time for today for further review/test/merge, so no rush :) I definitely would want to try the !amend commit case. We're close to getting this PR merged, should be before end of week 👍
However, I wonder whether we should change the name then. [...] Thoughts?
How about DisallowCleanupCommits?
Separate PRs for each rule please.
:+1:
I'm out of time for today for further review/test/merge, so no rush :) I definitely would want to try the !amend commit case. We're close to getting this PR merged, should be before end of week
I am in no hurry at all, we can take as much time as we need! :+1: :wink:
Sorry for the delay, I didn't press submit on my code review from last week apparently. And here I was thinking I was waiting for you!
Sure, no worries! I am in no rush! I will squash away these fixups once you approve! :+1:
FYI that you can easily run tests locally, that will allow you to fix these things up front. See here: https://jorisroovers.com/gitlint/contributing.
Note to self:
nix-shell -p python3Packages.flake8 python3Packages.coverage python3Packages.coveralls python3Packages.flake8-polyfill python3Packages.pytest python3Packages.pylint python3Packages.setuptools python3Packages.wheel python3 python3Packages.arrow python3Packages.sh python3Packages.click --run "bash ./run_tests.sh"
Okay, so after the latest patch I'm getting
│ # Assert violation when 'fixup!' in title │
│ violations = rule.validate(self.gitcommit("fixup! Föobar\n\nMy Body")) │
│ expected_violation = RuleViolation("CC1", "Fixup commits are not allowed", line_nr=1) │
│> self.assertListEqual(violations, [expected_violation]) │
│E AssertionError: Lists differ: [<gitlint.rules.RuleViolation object at 0x7f24b67c3fa0>] != [<gitlint.rules.RuleViolation object at 0x7f24b67c3e20>] │
│E │
│E First differing element 0: │
│E <gitlint.rules.RuleViolation object at 0x7f24b67c3fa0> │
│E <gitlint.rules.RuleViolation object at 0x7f24b67c3e20> │
│E │
│E - [<gitlint.rules.RuleViolation object at 0x7f24b67c3fa0>] │
│E ? ^^ │
│E │
│E + [<gitlint.rules.RuleViolation object at 0x7f24b67c3e20>] │
│E ? ^^ │
│ │
│gitlint-core/gitlint/tests/contrib/rules/test_disallow_cleanup_commits.py:27: AssertionError
Which I don't understand... the assertion should happen on the object contents, right? Not on the object address...
(I'm not that much a python guy as you probably noticed already)
The confusing error message is because we don’t have __repr__ implemented, I created #321 to fix that. The real error was that you were using rule-id CC1 instead of CC2 in your tests.
I found some other issues with the tests too (!squash should be squash!, same for amend! + code styling issues). I opted to quickly fix those rather than doing a few more back-and-forths, see https://github.com/jorisroovers/gitlint/pull/312/commits/1d9db6680035822f49c02c1244f694e7ca3efa14. With that, I think we’re all set here. Please give a quick acknowledgement and I’ll merge this!
This will go out with gitlint 0.18.0, currently no release date set (at least a few weeks, potentially 1-2 months).
Thanks for contributing to gitlint, really appreciated! 🎉
Sure! As this is quite a large PR, I will do a rebase and clean up the history and then we can merge this :tada:
Sure! As this is quite a large PR, I will do a rebase and clean up the history and then we can merge this 🎉
Not needed! I squash PRs on merge, and this is already based on latest main. Merging now :)