FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

lib: skip fixup! and squash! commits by richardlau · Pull Request #33 · nodejs/core-validate-commit · GitHub

/ core-validate-commit Public

lib: skip fixup! and squash! commits - #33

Closed
richardlau wants to merge 5 commits into
nodejs:masterfrom
richardlau:skip-fixup-squash
ConversationCommits (5)ChecksFiles changed
Closed

lib: skip fixup! and squash! commits#33
richardlau wants to merge 5 commits into
nodejs:masterfrom
richardlau:skip-fixup-squash

Conversation

richardlau commented Nov 9, 2018

Copy link
Copy Markdown
Member

joyeecheung reviewed Nov 9, 2018
View reviewed changes
Comment thread lib/index.js Outdated
this.disableRule('reviewers')
this.disableRule('metadata-end')
}
this.disableRule('skip-fixup-squash')

joyeecheung Nov 9, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Shouldn't this be disabled with a CLI option (see this.opts above) by default? I think only our CI will actually need this.

Trott Nov 9, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I agree that it skip-fixup-squash should be disabled by default. If node-core-utils detects a fixup or squash commit, I'd want it flagged. It's just CI where we want it disabled. I consider node-core-utils the primary use case.

Trott Nov 9, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

(Or check-fixup-squash is enabled by default and the --no-check-fixup-squash flag disables it.)

richardlau Nov 9, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Okay, I'll rework to --check-autosquash (less to type than --check-fixup-squash).

Comment thread lib/index.js Outdated
for (const rule of this.rules.values()) {
if (rule.disabled) continue
rule.validate(commit)
if (commit.title.startsWith('fixup!') ||

joyeecheung Nov 9, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The changes in this file seems unnecessary? I think the request was more like, if an option is on, then enable this rule, then check the fixup! and squash!. The rule disabling logic is in lib/index.js.

Trott Nov 9, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think the way it works is the options are positive (e.g., validate-metadata) and prepended with no at the command line to disable (e.g., --no-validate-metadata). So this might be better as a check-fixup-squash or check-autosquashable option and then our CI can send --no-lint-fixup-squash or --no-lint-autosquashable?

richardlau Nov 10, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The changes in this file are because, AFAICT, the rules as currently implemented are independent of each other and unordered. The new "rule" is there to give visual feedback when the commit is skipped, e.g.

-bash-4.2$ node bin/cmd.js --no-validate-metadata https://api.github.com/repos/nodejs/node/pulls/24254/commits
  ✔  1a79e37ebc0407581c093fac70a541d4d7b77ad5
     ✔  0:0      skipping fixes-url                        fixes-url
     ✔  0:0      blank line after title                    line-after-title
     ✔  0:0      line-lengths are valid                    line-length
     ✔  0:0      valid subsystems                          subsystem
     ✔  0:0      Title is formatted correctly.             title-format
     ✔  0:0      Title is <= 50 columns.                   title-length
  ✔  295c5cfef7a0dc36069d6e2ae523c2ed11ce3d1b
     ✔  0:0      Skipping fixup! commit.                   skip-fixup-squash
  ✔  5a01d5e5664889e8a8d1febf6b01b70379948c38
     ✔  0:0      Skipping fixup! commit.                   skip-fixup-squash
  ✔  2857dbb485c31df99792171ed5a788121dc011a6
     ✔  0:0      Skipping fixup! commit.                   skip-fixup-squash
-bash-4.2$

Trott commented Nov 9, 2018

Copy link
Copy Markdown
Member

Next-level feature: squash/fixup commits are ignored, but not if they are the first commit in the series being linted.

Trott commented Nov 9, 2018

Copy link
Copy Markdown
Member

Thanks for doing this. I look forward to Ci linting more than just the first commit! :-D

richardlau commented Nov 10, 2018

Copy link
Copy Markdown
Member Author

Added new option -a, --check-autosquashable:

-bash-4.2$ node bin/cmd.js --help
core-validate-commit - Validate the commit message for a particular commit in node core

  usage: core-validate-commit [options] [sha[, sha]]

  options:
    -h, --help                    show help and usage
    -v, --version                 show version
    -V, --validate-metadata       validate PR-URL and reviewers (on by default)
    -t, --tap                     output in tap format
    -l, --list                    list rules and their descriptions
    -a, --check-autosquashable    check autosquashable commits (on by default)
...

which is on by default:

-bash-4.2$ node bin/cmd.js --no-validate-metadata https://api.github.com/repos/nodejs/node/pulls/24220/commits
  ✔  ab62bc95d9103a8beebd0d3bd6fea81ed1771cca
     ✔  0:0      skipping fixes-url                        fixes-url
     ✔  0:0      blank line after title                    line-after-title
     ✔  0:0      line-lengths are valid                    line-length
     ✔  0:0      valid subsystems                          subsystem
     ✔  0:0      Title is formatted correctly.             title-format
     ⚠  0:50     Title should be <= 50 columns.            title-length
  ✖  11ddcdbbdb64286e6cf9024e3f9782deb89bb5da
     ✔  0:0      skipping fixes-url                        fixes-url
     ✔  0:0      blank line after title                    line-after-title
     ✔  0:0      line-lengths are valid                    line-length
     ✖  0:0      Invalid subsystem: "fixup! benchmark"     subsystem
     ✔  0:0      Title is formatted correctly.             title-format
     ⚠  0:50     Title should be <= 50 columns.            title-length
-bash-4.2$

and when toggled off:

-bash-4.2$ node bin/cmd.js --no-check-autosquashable --no-validate-metadata https://api.github.com/repos/nodejs/node/pulls/24220/commits
  ✔  ab62bc95d9103a8beebd0d3bd6fea81ed1771cca
     ✔  0:0      skipping fixes-url                        fixes-url
     ✔  0:0      blank line after title                    line-after-title
     ✔  0:0      line-lengths are valid                    line-length
     ✔  0:0      valid subsystems                          subsystem
     ✔  0:0      Title is formatted correctly.             title-format
     ⚠  0:50     Title should be <= 50 columns.            title-length
  ✔  11ddcdbbdb64286e6cf9024e3f9782deb89bb5da
     ✔  0:0      Skipping fixup! commit.                   skip-autosquashable
-bash-4.2$

richardlau commented Nov 10, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

Added more tests and changed the skipped message from a pass to a warning.

-bash-4.2$ node bin/cmd.js --no-validate-metadata https://api.github.com/repos/nodejs/node/pulls/24220/commits
  ✔  ab62bc95d9103a8beebd0d3bd6fea81ed1771cca
     ✔  0:0      skipping fixes-url                        fixes-url
     ✔  0:0      blank line after title                    line-after-title
     ✔  0:0      line-lengths are valid                    line-length
     ✔  0:0      valid subsystems                          subsystem
     ✔  0:0      Title is formatted correctly.             title-format
     ⚠  0:50     Title should be <= 50 columns.            title-length
  ✖  11ddcdbbdb64286e6cf9024e3f9782deb89bb5da
     ✔  0:0      skipping fixes-url                        fixes-url
     ✔  0:0      blank line after title                    line-after-title
     ✔  0:0      line-lengths are valid                    line-length
     ✖  0:0      Invalid subsystem: "fixup! benchmark"     subsystem
     ✔  0:0      Title is formatted correctly.             title-format
     ⚠  0:50     Title should be <= 50 columns.            title-length
-bash-4.2$ node bin/cmd.js --no-check-autosquashable --no-validate-metadata https://api.github.com/repos/nodejs/node/pulls/24220/commits
  ✔  ab62bc95d9103a8beebd0d3bd6fea81ed1771cca
     ✔  0:0      skipping fixes-url                        fixes-url
     ✔  0:0      blank line after title                    line-after-title
     ✔  0:0      line-lengths are valid                    line-length
     ✔  0:0      valid subsystems                          subsystem
     ✔  0:0      Title is formatted correctly.             title-format
     ⚠  0:50     Title should be <= 50 columns.            title-length
  ✔  11ddcdbbdb64286e6cf9024e3f9782deb89bb5da
     ⚠  0:0      Skipping fixup! commit.                   skip-autosquashable
-bash-4.2$

richardlau commented Nov 10, 2018

Copy link
Copy Markdown
Member Author

Next-level feature: squash/fixup commits are ignored, but not if they are the first commit in the series being linted.

Would require more thought. Currently each commit is being linted as an independent entity and each individual linting doesn't know if it's in a series or not.

joyeecheung reviewed Nov 19, 2018
View reviewed changes
Comment thread lib/index.js
for (const rule of this.rules.values()) {
if (rule.disabled) continue
rule.validate(commit)
if (!this.opts['check-autosquashable'] &&

joyeecheung Nov 19, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

So I am a bit confused..shouldn't it be named skip-autosquashable (off by default)? Because even when this is turned off via a --no switch we are still going to apply that rule to the commit for the warning

richardlau Nov 19, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I see the option as conceptual (i.e. a signal of intent) and the rule an implementation detail. I followed the existing validate-metadata option (which is also on by default and is not, for example, skip-metadata).

Initially I didn't have a rule at all and implemented the skip logic directly here. Maybe it shouldn't be a rule at all (for example it doesn't really have pass/fail states). It's sort of a meta "if the commit looks like this then don't apply the usual rules but apply this special one to emit a warning instead".

joyeecheung Nov 20, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@richardlau ACK. Somehow it feels more natural to me to say the intent is "skip autosquashability checks", and the rules are "check if the commits are autosqushable" i.e. I think it would be more intuitive if we switch the two names, the option being skip-autosquash-checks and the rule being check-autosquashable

joyeecheung Nov 20, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

..and, it may more natural if we split the "warn" checks and the "fail" checks into two rules, if the sole purpose of the warning rules is to get some warnings displayed..so we can keep the existing skip-autosquashable.js but strip the "fail" checks into a check-autosquashable.js

Comment thread lib/rules/skip-autosquashable.js
id: id
, meta: {
description: 'skip autosquashable fixup! and squash! commits'
, recommended: true

joyeecheung Nov 19, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

..so that the recommended and disabled here should be both false by default?

richardlau Nov 19, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I couldn't work out what recommended is for (or find code that does anything with it).

richardlau commented Nov 19, 2018

Copy link
Copy Markdown
Member Author

Just for the record, I'm not in a great rush to land this (and appreciate the reviews). I'm not even sure whether we would ultimately switch this on in the CI as anecdotally the only person I've noticed use autosquashable fixup! or squash! commits is @Trott.

Trott commented Nov 20, 2018

Copy link
Copy Markdown
Member

anecdotally the only person I've noticed use autosquashable fixup! or squash! commits is @Trott.

I wish more people would use them. They really help out when used in conjunction with node-core-utils.

richardlau commented Nov 20, 2018

Copy link
Copy Markdown
Member Author

anecdotally the only person I've noticed use autosquashable fixup! or squash! commits is @Trott.

I wish more people would use them. They really help out when used in conjunction with node-core-utils.

TBH I didn't even know they existed until I started looking into this from the various lint commit message Travis PR's. I've even myself previously used variants of fixup: ... commit messages (which are not autosquashable), copying the style from other PR's, but now I know git commit --fixup does this.

Of course, another argument could be made that if we start linting all commit messages it would raise awareness/force the adoption of autosquashable to make the commit linter job pass.

joyeecheung commented Nov 20, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

@richardlau Presumably if everybody uses these types of messages it would help implementing a CI job that merge the PRs without just squashing them into one commit but multiple commits as you wish...but yeah that's probably too far from now

joyeecheung commented Nov 20, 2018

Copy link
Copy Markdown
Member

Considering how limited the audience of this package is we can always come back tweaking the options..

joyeecheung commented Nov 20, 2018

Copy link
Copy Markdown
Member

@richardlau Do you think it's ready to merge this?

richardlau commented Nov 20, 2018

Copy link
Copy Markdown
Member Author

@richardlau Do you think it's ready to merge this?

Let me think about #33 (comment) and #33 (comment).

richardlau commented Nov 23, 2018

Copy link
Copy Markdown
Member Author

I'm going to close this for now. Given the direction of nodejs/node#24574 to avoid using the GitHub API, if we do want to filter out autosquashable commits we can do it in the shell script in the core repository before feeding the SHA hashes into core-validate-commit. Thanks anyway for the feedback/reviews.

richardlau closed this Nov 23, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

joyeecheung joyeecheung approved these changes
+1 more reviewer
Trott Trott approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL