lib: skip fixup! and squash! commits #33

richardlau · 2018-11-09T18:27:46Z

joyeecheung · 2018-11-09T21:29:24Z

lib/index.js

@@ -30,6 +30,7 @@ module.exports = class ValidateCommit extends EE {
      this.disableRule('reviewers')
      this.disableRule('metadata-end')
    }
+    this.disableRule('skip-fixup-squash')


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

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.

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

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

joyeecheung · 2018-11-09T21:31:28Z

lib/index.js

-      for (const rule of this.rules.values()) {
-        if (rule.disabled) continue
-        rule.validate(commit)
+      if (commit.title.startsWith('fixup!') ||


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.

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?

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 · 2018-11-09T21:50:52Z

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

Trott · 2018-11-09T22:04:20Z

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

richardlau · 2018-11-10T00:53:45Z

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 · 2018-11-10T01:55:20Z

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 · 2018-11-10T02:00:43Z

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 · 2018-11-19T10:50:48Z

lib/index.js

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


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

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

@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

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

joyeecheung · 2018-11-19T10:52:17Z

lib/rules/skip-autosquashable.js

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


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

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

richardlau · 2018-11-19T18:52:25Z

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 · 2018-11-20T00:07:10Z

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 · 2018-11-20T00:24:25Z

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 · 2018-11-20T13:47:03Z

@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 · 2018-11-20T13:48:25Z

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

joyeecheung · 2018-11-20T13:50:34Z

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

richardlau · 2018-11-20T14:27:00Z

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

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

richardlau · 2018-11-23T11:29:13Z

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.

lib: skip fixup! and squash! commits

79f946e

joyeecheung reviewed Nov 9, 2018

View reviewed changes

squash! lib: skip fixup! and squash! commits

d07d944

richardlau added 3 commits November 9, 2018 20:04

fixup! lib: skip fixup! and squash! commits

25e5398

fixup! lib: skip fixup! and squash! commits

47f98de

fixup! lib: skip fixup! and squash! commits

229ba0c

Trott approved these changes Nov 10, 2018

View reviewed changes

joyeecheung reviewed Nov 19, 2018

View reviewed changes

joyeecheung approved these changes Nov 20, 2018

View reviewed changes

richardlau closed this Nov 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: skip fixup! and squash! commits #33

lib: skip fixup! and squash! commits #33

richardlau commented Nov 9, 2018

joyeecheung Nov 9, 2018

Trott Nov 9, 2018

Trott Nov 9, 2018

richardlau Nov 9, 2018

joyeecheung Nov 9, 2018

Trott Nov 9, 2018

richardlau Nov 10, 2018 •

edited

Loading

Trott commented Nov 9, 2018

Trott commented Nov 9, 2018

richardlau commented Nov 10, 2018

richardlau commented Nov 10, 2018 •

edited

Loading

richardlau commented Nov 10, 2018

joyeecheung Nov 19, 2018 •

edited

Loading

richardlau Nov 19, 2018

joyeecheung Nov 20, 2018

joyeecheung Nov 20, 2018

joyeecheung Nov 19, 2018

richardlau Nov 19, 2018

richardlau commented Nov 19, 2018

Trott commented Nov 20, 2018

richardlau commented Nov 20, 2018

joyeecheung commented Nov 20, 2018 •

edited

Loading

joyeecheung commented Nov 20, 2018

joyeecheung commented Nov 20, 2018

richardlau commented Nov 20, 2018

richardlau commented Nov 23, 2018

lib: skip fixup! and squash! commits #33

lib: skip fixup! and squash! commits #33

Conversation

richardlau commented Nov 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardlau Nov 10, 2018 • edited Loading

Choose a reason for hiding this comment

Trott commented Nov 9, 2018

Trott commented Nov 9, 2018

richardlau commented Nov 10, 2018

richardlau commented Nov 10, 2018 • edited Loading

richardlau commented Nov 10, 2018

joyeecheung Nov 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardlau commented Nov 19, 2018

Trott commented Nov 20, 2018

richardlau commented Nov 20, 2018

joyeecheung commented Nov 20, 2018 • edited Loading

joyeecheung commented Nov 20, 2018

joyeecheung commented Nov 20, 2018

richardlau commented Nov 20, 2018

richardlau commented Nov 23, 2018

richardlau Nov 10, 2018 •

edited

Loading

richardlau commented Nov 10, 2018 •

edited

Loading

joyeecheung Nov 19, 2018 •

edited

Loading

joyeecheung commented Nov 20, 2018 •

edited

Loading