Implement contrib rule to block fixup/squash commits

[matthiasbeyer]

Found #139, figured that it is a low-hanging fruit and went and harvested that low-hanging fruit.

Closes #139

[jorisroovers]

Left comments inline.

Needs docs and unit test, please see:
https://jorisroovers.com/gitlint/contributing/#contrib-rules

[matthiasbeyer]

Added some fixups as well as tests and docs. 🎉 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...

[jorisroovers]

Thanks for taking the time to follow-up!

Suggested changes wrt following testing naming conventions inline. Looks pretty good otherwise. I haven't manually tested it yet, will probably do that before merging.

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.

Wrt !amend, is that something that can really be present in a commit message? 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.

[matthiasbeyer]

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.

[matthiasbeyer]

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?

[jorisroovers]

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 👍

[matthiasbeyer]

However, I wonder whether we should change the name then. [...] Thoughts?

How about DisallowCleanupCommits?

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

I am in no hurry at all, we can take as much time as we need! 👍 😉

[jorisroovers]

I like DisallowCleanupCommits - let's do it!

Also experimented a bit with amend! commits.

For (my own) future reference:

# To create amend! commit
git commit --fixup=amend:<sha>

# You can also use 'reword', this will also create an `amend!` commit, but will only reword the commit message, not adding changes to it
git commit --fixup=reword:<sha>

# amend! messages get autosquashed like regular fixup! and squash! commits
git rebase -i <sha> --autosquash

I agree it makes sense to add this to this contrib rule.

gitlint does however not yet have a commit.is_fixup_amend_commit flag. I will create a separate issue for this (edit: see #318). The detection itself is 1 line of code, but there’s a bit more work in adding/updating tests and other plumbing (e.g. commit __eq__, __str__ methods, etc).

Side-note: I personally prefer commit.is_fixup_amend_commit over commit.is_amend_commit as amend ! in this case is special form of fixup.

I suggest we don’t wait for that here and already implement amend! detection in this PR using commit.message.title.startswith("amend!"), so you can already do all the other related work (docs, tests, etc). We’ll do a quick follow-up commit to replace that with commit.is_fixup_amend_commit once it exists.

[jorisroovers]

The filename should use underscores, not dashes: disallow_cleanup_commits.py
This is what is causing CI to fail: https://github.com/jorisroovers/gitlint/runs/7674953115?check_suite_focus=true

[jorisroovers]

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!

[matthiasbeyer]

Sure, no worries! I am in no rush! I will squash away these fixups once you approve! 👍

[jorisroovers]

Unit test is failing, see https://github.com/jorisroovers/gitlint/runs/7801218972?check_suite_focus=true

=================================== FAILURES ===================================
_____ ContribDisallowCleanupCommitsTest.test_disallow_fixup_squash_commit ______

self = <gitlint.tests.contrib.rules.test_disallow_cleanup_commits.ContribDisallowCleanupCommitsTest testMethod=test_disallow_fixup_squash_commit>

    def test_disallow_fixup_squash_commit(self):
        # No violations when no 'fixup!' line and no 'squash!' line is present
        rule = DisallowCleanupCommits()
        violations = rule.validate(self.gitcommit("Föobar\n\nMy Body"))
>       self.assertListEqual([], violations)
E       AssertionError: Second sequence is not a list: None

gitlint-core/gitlint/tests/contrib/rules/test_disallow_cleanup_commits.py:22: AssertionError

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.

I've also just merged #320 which adds support for fixup=amend commit detection, see inline comment.

[jorisroovers]

I just merged #320, if you rebase to latest main branch, you'll be able to do this:

Suggested change

	if commit.message.title.startswith("amend!"):
	if commit.is_fixup_amend_commit:

[matthiasbeyer]

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"

[matthiasbeyer]

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)

[jorisroovers]

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 1d9db66. 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! 🎉

[matthiasbeyer]

Sure! As this is quite a large PR, I will do a rebase and clean up the history and then we can merge this 🎉

[jorisroovers]

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 :)