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

lib: skip fixup! and squash! commits #33

Closed
wants to merge 5 commits into from

Conversation

richardlau
Copy link
Member

lib/index.js Outdated
@@ -30,6 +30,7 @@ module.exports = class ValidateCommit extends EE {
this.disableRule('reviewers')
this.disableRule('metadata-end')
}
this.disableRule('skip-fixup-squash')
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@richardlau richardlau Nov 10, 2018

Choose a reason for hiding this comment

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

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
Copy link
Member

Trott commented Nov 9, 2018

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

@Trott
Copy link
Member

Trott commented Nov 9, 2018

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

@richardlau
Copy link
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
Copy link
Member Author

richardlau commented Nov 10, 2018

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
Copy link
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.

for (const rule of this.rules.values()) {
if (rule.disabled) continue
rule.validate(commit)
if (!this.opts['check-autosquashable'] &&
Copy link
Member

@joyeecheung joyeecheung Nov 19, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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".

Copy link
Member

Choose a reason for hiding this comment

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

@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

Copy link
Member

Choose a reason for hiding this comment

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

..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

id: id
, meta: {
description: 'skip autosquashable fixup! and squash! commits'
, recommended: true
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@richardlau
Copy link
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
Copy link
Member

Trott commented Nov 20, 2018

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
Copy link
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
Copy link
Member

joyeecheung commented Nov 20, 2018

@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
Copy link
Member

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

@joyeecheung
Copy link
Member

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

@richardlau
Copy link
Member Author

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

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

@richardlau
Copy link
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 richardlau closed this Nov 23, 2018
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 this pull request may close these issues.

3 participants