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.

Sign up for GitHub

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

Jump to bottom

build: disallow squash commits and ensure fixup commits match an earlier commit #32023

Closed
wants to merge 4 commits into from
Closed

build: disallow squash commits and ensure fixup commits match an earlier commit #32023

wants to merge 4 commits into from

Conversation

gkalpak
Copy link
Member

@gkalpak gkalpak commented Aug 6, 2019 •
edited

This PR cleans up the validate-commit-message script and tests and introduces the following changes:

  • fix: do not allow squash! commits in validate-commit-message on CI
  • build: ensure fixup commits match an earlier, unmerged commit

This mainly ensures that we don't accidentally merge a fixup/squash commit into master.
See the individual commit messages for details.

@gkalpak gkalpak requested a review from a team as a code owner August 6, 2019 18:02
@gkalpak gkalpak added area: build & ci Related the build and CI infrastructure of the project action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release type: bug/fix labels Aug 6, 2019
@ngbot ngbot bot modified the milestone: needsTriage Aug 6, 2019
@gkalpak
test: clean up and re-organize validate-commit-message tests
547b6e3 
Mainly making the tests more closely follow the order of checks in the
function implementation, so that it is easier to follow.
@gkalpak
refactor: clean up validate-commit-message script
c803ed4 
This sets the ground for adding stricter rules for fixup commits in a
follow-up PR.
@JoostK
Copy link
Member

JoostK commented Aug 6, 2019

Wouldn't the postcommit run of check-commit-message consider a fixup commit invalid, before it got a chance to be rebased into the correct location?

Also, at least for me it's quite common to push fixup commits without rebasing at all. Although this may results in conflicts when reordering the commits during merge, quite often it's known that certain commits won't cause any conflicts when their changes are reordered.

@gkalpak gkalpak force-pushed the build-stricter-fixup-validation branch from 7b54b10 to 0edf4bd Compare August 6, 2019 18:23
@gkalpak
Copy link
Member Author

gkalpak commented Aug 6, 2019

Wouldn't the postcommit run of check-commit-message consider a fixup commit invalid, before it got a chance to be rebased into the correct location?

We don't have a post-commit hook. If you mean the commit-msg hook, then no. The commit-msg hook uses the validate-commit-message, which allows fixup commit. The behavior of the commit-msg hook is not affected by this PR (i.e. it allows fixup commits and checks that their format is valid (ignoring the fixup! prefix), but not whether they match a previous commit.

Also, at least for me it's quite common to push fixup commits without rebasing at all. Although this may results in conflicts when reordering the commits during merge, quite often it's known that certain commits won't cause any conflicts when their changes are reordered.

Me too 😁 This is still supported 😅
The word "preceding" might not be the right one here. I didn't mean the commit that comes immediately before the fixup commit, but anywhere between the fixup commit and the PR base. I'll find a more suitable word.

I also just realized that squash commits might be useful during local development, so I will change the script to allow them in the commit-msg validation and disallow them on CI.

Thx for the feedback 😃

@gkalpak gkalpak added state: WIP and removed action: merge The PR is ready for merge by the caretaker labels Aug 6, 2019
@gkalpak
fix: do not allow squash! commits when merging
7e27816 
While `fixup! ` is fine, `squash! ` means that the commit message needs
tweaking, which cannot be done automatically during merging (i.e. it
should be done by the PR author).

Previously, `validate-commit-message` would always allow
`squash! `-prefixed commits, which would cause problems during merging.

This commit changes `validate-commit-message` to make it configurable
whether such commits are allowed and configures the
`gulp validate-commit-message` task, which is run as part of the `lint`
job on CI, to not allow them.

NOTE: This new check is disabled in the pre-commit git hook that is used
      to validate commit messages, because these commits might still be
      useful during development.
@gkalpak gkalpak force-pushed the build-stricter-fixup-validation branch from 0edf4bd to d62851d Compare August 6, 2019 19:51
@gkalpak
build: ensure fixup commits match an earlier, unmerged commit
598ab1f 
Previously, `validate-commit-message` would treat `fixup! `-prefixed
commits like this:
- It would strip the `fixup! ` prefix.
- It would validate the rest of the commit message header as any other
  commit.

However, fixup commits are special in that they need to exactly match an
earlier commit message header (sans the `fixup! ` prefix) in order for
git to treat them correctly. Otherwise, they will not be squashed into
the original commits and will be merged as is. Fixup commits can end up
not matching their original commit for several reasons (e.g. accidental
typo, changing the original commit message, etc.).

This commit prevents invalid fixup commits to pass validation by
ensuring that they match an earlier (unmerged) commit (i.e. a commit
between the current HEAD and the BASE commit).

NOTE: This new behavior is currently not activated in the pre-commit git
      hook, that is used to validate commit messages (because the
      preceding, unmerged commits are not available there). It _is_
      activated in `gulp validate-commit-message`, which is run as part
      of the `lint` job on CI and thus will detect invalid commits,
      before their getting merged.
@gkalpak gkalpak force-pushed the build-stricter-fixup-validation branch from d62851d to 598ab1f Compare August 6, 2019 22:11
@gkalpak gkalpak added action: merge The PR is ready for merge by the caretaker and removed state: WIP labels Aug 7, 2019
@gkalpak gkalpak changed the title build: disallow squash commits and ensure fixup commits match a preceding commit build: disallow squash commits and ensure fixup commits match an earlier commit Aug 7, 2019
@kara kara added the action: review The PR is still awaiting reviews from at least one requested reviewer label Aug 9, 2019
@josephperrott josephperrott removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Aug 9, 2019
kara pushed a commit that referenced this pull request Aug 9, 2019
@gkalpak @kara
test: clean up and re-organize validate-commit-message tests (#32023)
d9d0689 
Mainly making the tests more closely follow the order of checks in the
function implementation, so that it is easier to follow.

PR Close #32023
kara pushed a commit that referenced this pull request Aug 9, 2019
@gkalpak @kara
refactor: clean up validate-commit-message script (#32023)
7b2617f 
This sets the ground for adding stricter rules for fixup commits in a
follow-up PR.

PR Close #32023
kara pushed a commit that referenced this pull request Aug 9, 2019
@gkalpak @kara
fix: do not allow squash! commits when merging (#32023)
edd6cb5 
While `fixup! ` is fine, `squash! ` means that the commit message needs
tweaking, which cannot be done automatically during merging (i.e. it
should be done by the PR author).

Previously, `validate-commit-message` would always allow
`squash! `-prefixed commits, which would cause problems during merging.

This commit changes `validate-commit-message` to make it configurable
whether such commits are allowed and configures the
`gulp validate-commit-message` task, which is run as part of the `lint`
job on CI, to not allow them.

NOTE: This new check is disabled in the pre-commit git hook that is used
      to validate commit messages, because these commits might still be
      useful during development.

PR Close #32023
kara pushed a commit that referenced this pull request Aug 9, 2019
@gkalpak @kara
build: ensure fixup commits match an earlier, unmerged commit (#32023)
c67f490 
Previously, `validate-commit-message` would treat `fixup! `-prefixed
commits like this:
- It would strip the `fixup! ` prefix.
- It would validate the rest of the commit message header as any other
  commit.

However, fixup commits are special in that they need to exactly match an
earlier commit message header (sans the `fixup! ` prefix) in order for
git to treat them correctly. Otherwise, they will not be squashed into
the original commits and will be merged as is. Fixup commits can end up
not matching their original commit for several reasons (e.g. accidental
typo, changing the original commit message, etc.).

This commit prevents invalid fixup commits to pass validation by
ensuring that they match an earlier (unmerged) commit (i.e. a commit
between the current HEAD and the BASE commit).

NOTE: This new behavior is currently not activated in the pre-commit git
      hook, that is used to validate commit messages (because the
      preceding, unmerged commits are not available there). It _is_
      activated in `gulp validate-commit-message`, which is run as part
      of the `lint` job on CI and thus will detect invalid commits,
      before their getting merged.

PR Close #32023
@kara kara closed this in ddd0204 Aug 9, 2019
kara pushed a commit that referenced this pull request Aug 9, 2019
@gkalpak @kara
refactor: clean up validate-commit-message script (#32023)
2b28925 
This sets the ground for adding stricter rules for fixup commits in a
follow-up PR.

PR Close #32023
kara pushed a commit that referenced this pull request Aug 9, 2019
@gkalpak @kara
fix: do not allow squash! commits when merging (#32023)
c0d5684 
While `fixup! ` is fine, `squash! ` means that the commit message needs
tweaking, which cannot be done automatically during merging (i.e. it
should be done by the PR author).

Previously, `validate-commit-message` would always allow
`squash! `-prefixed commits, which would cause problems during merging.

This commit changes `validate-commit-message` to make it configurable
whether such commits are allowed and configures the
`gulp validate-commit-message` task, which is run as part of the `lint`
job on CI, to not allow them.

NOTE: This new check is disabled in the pre-commit git hook that is used
      to validate commit messages, because these commits might still be
      useful during development.

PR Close #32023
kara pushed a commit that referenced this pull request Aug 9, 2019
@gkalpak @kara
build: ensure fixup commits match an earlier, unmerged commit (#32023)
3df54be 
Previously, `validate-commit-message` would treat `fixup! `-prefixed
commits like this:
- It would strip the `fixup! ` prefix.
- It would validate the rest of the commit message header as any other
  commit.

However, fixup commits are special in that they need to exactly match an
earlier commit message header (sans the `fixup! ` prefix) in order for
git to treat them correctly. Otherwise, they will not be squashed into
the original commits and will be merged as is. Fixup commits can end up
not matching their original commit for several reasons (e.g. accidental
typo, changing the original commit message, etc.).

This commit prevents invalid fixup commits to pass validation by
ensuring that they match an earlier (unmerged) commit (i.e. a commit
between the current HEAD and the BASE commit).

NOTE: This new behavior is currently not activated in the pre-commit git
      hook, that is used to validate commit messages (because the
      preceding, unmerged commits are not available there). It _is_
      activated in `gulp validate-commit-message`, which is run as part
      of the `lint` job on CI and thus will detect invalid commits,
      before their getting merged.

PR Close #32023
@gkalpak gkalpak deleted the build-stricter-fixup-validation branch August 12, 2019 07:59
pkozlowski-opensource pushed a commit to pkozlowski-opensource/angular that referenced this pull request Aug 12, 2019
@gkalpak @pkozlowski-opensource
test: clean up and re-organize validate-commit-message tests (angul…
35a47f2 
…ar#32023)

Mainly making the tests more closely follow the order of checks in the
function implementation, so that it is easier to follow.

PR Close angular#32023
pkozlowski-opensource pushed a commit to pkozlowski-opensource/angular that referenced this pull request Aug 12, 2019
@gkalpak @pkozlowski-opensource
refactor: clean up validate-commit-message script (angular#32023)
5925cf4 
This sets the ground for adding stricter rules for fixup commits in a
follow-up PR.

PR Close angular#32023
pkozlowski-opensource pushed a commit to pkozlowski-opensource/angular that referenced this pull request Aug 12, 2019
@gkalpak @pkozlowski-opensource
fix: do not allow squash! commits when merging (angular#32023)
38289be 
While `fixup! ` is fine, `squash! ` means that the commit message needs
tweaking, which cannot be done automatically during merging (i.e. it
should be done by the PR author).

Previously, `validate-commit-message` would always allow
`squash! `-prefixed commits, which would cause problems during merging.

This commit changes `validate-commit-message` to make it configurable
whether such commits are allowed and configures the
`gulp validate-commit-message` task, which is run as part of the `lint`
job on CI, to not allow them.

NOTE: This new check is disabled in the pre-commit git hook that is used
      to validate commit messages, because these commits might still be
      useful during development.

PR Close angular#32023
pkozlowski-opensource pushed a commit to pkozlowski-opensource/angular that referenced this pull request Aug 12, 2019
@gkalpak @pkozlowski-opensource
build: ensure fixup commits match an earlier, unmerged commit (angula…
cbb3a71 
…r#32023)

Previously, `validate-commit-message` would treat `fixup! `-prefixed
commits like this:
- It would strip the `fixup! ` prefix.
- It would validate the rest of the commit message header as any other
  commit.

However, fixup commits are special in that they need to exactly match an
earlier commit message header (sans the `fixup! ` prefix) in order for
git to treat them correctly. Otherwise, they will not be squashed into
the original commits and will be merged as is. Fixup commits can end up
not matching their original commit for several reasons (e.g. accidental
typo, changing the original commit message, etc.).

This commit prevents invalid fixup commits to pass validation by
ensuring that they match an earlier (unmerged) commit (i.e. a commit
between the current HEAD and the BASE commit).

NOTE: This new behavior is currently not activated in the pre-commit git
      hook, that is used to validate commit messages (because the
      preceding, unmerged commits are not available there). It _is_
      activated in `gulp validate-commit-message`, which is run as part
      of the `lint` job on CI and thus will detect invalid commits,
      before their getting merged.

PR Close angular#32023
sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
@gkalpak @sabeersulaiman
test: clean up and re-organize validate-commit-message tests (angul…
ca8d3d8 
…ar#32023)

Mainly making the tests more closely follow the order of checks in the
function implementation, so that it is easier to follow.

PR Close angular#32023
sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
@gkalpak @sabeersulaiman
refactor: clean up validate-commit-message script (angular#32023)
bdc6f6d 
This sets the ground for adding stricter rules for fixup commits in a
follow-up PR.

PR Close angular#32023
sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
@gkalpak @sabeersulaiman
fix: do not allow squash! commits when merging (angular#32023)
ae4e102 
While `fixup! ` is fine, `squash! ` means that the commit message needs
tweaking, which cannot be done automatically during merging (i.e. it
should be done by the PR author).

Previously, `validate-commit-message` would always allow
`squash! `-prefixed commits, which would cause problems during merging.

This commit changes `validate-commit-message` to make it configurable
whether such commits are allowed and configures the
`gulp validate-commit-message` task, which is run as part of the `lint`
job on CI, to not allow them.

NOTE: This new check is disabled in the pre-commit git hook that is used
      to validate commit messages, because these commits might still be
      useful during development.

PR Close angular#32023
sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
@gkalpak @sabeersulaiman
build: ensure fixup commits match an earlier, unmerged commit (angula…
8eb05ea 
…r#32023)

Previously, `validate-commit-message` would treat `fixup! `-prefixed
commits like this:
- It would strip the `fixup! ` prefix.
- It would validate the rest of the commit message header as any other
  commit.

However, fixup commits are special in that they need to exactly match an
earlier commit message header (sans the `fixup! ` prefix) in order for
git to treat them correctly. Otherwise, they will not be squashed into
the original commits and will be merged as is. Fixup commits can end up
not matching their original commit for several reasons (e.g. accidental
typo, changing the original commit message, etc.).

This commit prevents invalid fixup commits to pass validation by
ensuring that they match an earlier (unmerged) commit (i.e. a commit
between the current HEAD and the BASE commit).

NOTE: This new behavior is currently not activated in the pre-commit git
      hook, that is used to validate commit messages (because the
      preceding, unmerged commits are not available there). It _is_
      activated in `gulp validate-commit-message`, which is run as part
      of the `lint` job on CI and thus will detect invalid commits,
      before their getting merged.

PR Close angular#32023
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@josephperrott josephperrott josephperrott approved these changes

Assignees

@josephperrott josephperrott

Labels
action: merge The PR is ready for merge by the caretaker area: build & ci Related the build and CI infrastructure of the project cla: yes target: patch This PR is targeted for the next patch release type: bug/fix
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@gkalpak @JoostK @josephperrott @kara @googlebot