When squashing commits node-core-utils should fail if subsystem prefix is missing

[trivikr]

Hi,

I faced this issue while landing commit https://github.com/nodejs/node/commit/a82ac6eedf925b8922ca2e577d0364a62dea5ad7

Creating quick issue for follow-up, will add details later. Summary: When there are multiple commits to land and node-core-utils suggests squashing, it doesn't fail if subsystem prefix (and probably other requirements) is missing.

I'll reproduce the steps and add details later.

Regards, Trivikram

[joyeecheung]

It will fail when you run git node land --final though (that's when core-validate-commit gets invoked). Have you run that? Or just run git node land --continue until it invokes the validations?

Also, if I understand correctly, you are suggesting that git node should do the validation before the squashing happens? But then the tool cannot know which commit will stay and need to be validate at the point, can it?

[trivikr]

I don't think I ran git node land --final before landing. I'd just run the command which was suggested for squashing commits. I remember it as I didn't have to manually replace pick to f as I'd to do in the past.

The validation should be done after the squashing happens, either the user:

• doesn't have to run third command git node land --final
• or git node land --final runs automatically after squashing

This quick issue was created for logging purposes, I'll recreate a scenario to provide details.

[Trott]

If you don't use ncu every day, it's easy to forget to run git node land --continue (or --final which I didn't know was a thing!). I'm not sure what a good solution would be unless we think it's a good idea to have it launch git rebase -i for you rather than drop you into a shell. The idea is that if it launched it for you, it can automatically continue when the rebase is done. (I'd be for it running git rebase -i for you rather than dropping you into a shell, but I'm not sure it would work with everyone else's workflow.)

[Trott]

(Also: I'm glad to be having these kinds of issues. Like, this is way better than the issues we were dealing with in the pre-ncu days!)

[trivikr]

Tested with landing fake change https://github.com/nodejs/node/pull/23235, and noticed that git node land --final does catch issues with commit as @joyeecheung mentioned in her comment

Outputs while trying to land:

$ git node land 23235

``` ✔ Done loading data for nodejs/node/pull/23235 ----------------------------------- PR info ------------------------------------ Title [DO NOT LAND] Testing squashing while landing with git-node (#23235) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch trivikr:trivikr-patch-1 -> nodejs:master Labels doc Commits 4 - Testing squashing while landing with git-node - Another commit to add to the PR - One more commit! - Adding sample text Committers 2 - GitHub - Kamat, Trivikram <16024985+trivikr@users.noreply.github.com> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/23235 -------------------------------------------------------------------------------- ✔ Requested Changes: 0 ✖ Approvals: 0 ✖ No CI runs detected ℹ This PR was created on Tue Oct 02 2018 (weekday in UTC) ⚠ 47 hours left to land -------------------------------------------------------------------------------- ? This PR is not ready to land, do you want to continue? [Y/n] Y ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- ? Do you want to try reset the local master branch to upstream/master? [Y/n] Y ⠧ Bringing upstream/master up to date...From github.com:nodejs/node * branch master -> FETCH_HEAD ✔ upstream/master is now up-to-date ✔ Downloaded patch to /home/trivikr/workspace/node/.ncu/23235/patch -------------------------------------------------------------------------------- Applying: Testing squashing while landing with git-node .git/rebase-apply/patch:13: new blank line at EOF. + warning: 1 line adds whitespace errors. Applying: Another commit to add to the PR .git/rebase-apply/patch:13: new blank line at EOF. + warning: 1 line adds whitespace errors. Applying: One more commit! .git/rebase-apply/patch:13: new blank line at EOF. + warning: 1 line adds whitespace errors. Applying: Adding sample text ✔ Patches applied There are 4 commits in the PR Please run the following command to complete landing $ git rebase upstream/master -i -x "git node land --amend" ```

$ git rebase upstream/master -i -x "git node land --amend"

``` Executing: git node land --amend --------------------------------- New Message ---------------------------------- Testing squashing while landing with git-node commit for testing squashing with node-core-utils https://github.com/nodejs/node-core-utils/issues/283 PR-URL: https://github.com/nodejs/node/pull/23235 -------------------------------------------------------------------------------- ? Use this message? [Y/n] Y interactive rebase in progress; onto 37e8381150 Last commands done (2 commands done): pick 90d381469a Testing squashing while landing with git-node exec git node land --amend Next commands to do (6 remaining commands): fixup b6bf7f3daf Another commit to add to the PR exec git node land --amend You are currently editing a commit while rebasing branch 'master' on '37e8381150'. No changes You asked to amend the most recent commit, but doing so would make it empty. You can repeat your command with --allow-empty, or you can remove the commit entirely with "git reset HEAD^". warning: execution failed: git node land --amend You can fix the problem, and then run git rebase --continue ```

$ git rebase --continue

``` Executing: git node land --amend --------------------------------- New Message ---------------------------------- Testing squashing while landing with git-node commit for testing squashing with node-core-utils https://github.com/nodejs/node-core-utils/issues/283 PR-URL: https://github.com/nodejs/node/pull/23235 -------------------------------------------------------------------------------- ? Use this message? [Y/n] Y [detached HEAD ad4220cde3] Testing squashing while landing with git-node Author: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Tue Oct 2 22:30:06 2018 -0700 1 file changed, 1 insertion(+) Executing: git node land --amend ⚠ Found PR-URL: https://github.com/nodejs/node/pull/23235, skipping.. --------------------------------- New Message ---------------------------------- Testing squashing while landing with git-node commit for testing squashing with node-core-utils https://github.com/nodejs/node-core-utils/issues/283 PR-URL: https://github.com/nodejs/node/pull/23235 -------------------------------------------------------------------------------- ? Use this message? [Y/n] Y [detached HEAD 38de6289e4] Testing squashing while landing with git-node Author: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Tue Oct 2 22:30:06 2018 -0700 1 file changed, 2 insertions(+) Executing: git node land --amend ⚠ Found PR-URL: https://github.com/nodejs/node/pull/23235, skipping.. --------------------------------- New Message ---------------------------------- Testing squashing while landing with git-node commit for testing squashing with node-core-utils https://github.com/nodejs/node-core-utils/issues/283 PR-URL: https://github.com/nodejs/node/pull/23235 -------------------------------------------------------------------------------- ? Use this message? [Y/n] Y [detached HEAD 9be43a85e0] Testing squashing while landing with git-node Author: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Tue Oct 2 22:30:06 2018 -0700 1 file changed, 3 insertions(+) Successfully rebased and updated refs/heads/master. ```

$ git node land --final

``` ✖ 9be43a85e02d105dac07ed77c6e287162e25bacb ✔ 0:0 skipping fixes-url fixes-url ✔ 0:0 blank line after title line-after-title ✖ 1:72 Line should be <= 72 columns. line-length ✔ 0:0 metadata is at end of message metadata-end ✖ 0:0 Commit must have at least 1 reviewer. reviewers ✖ 0:0 Missing subsystem. subsystem ✔ 0:0 Title is formatted correctly. title-format ✔ 0:0 Title is <= 50 columns. title-length ```

[trivikr]

Is there a way to inform the remind the collaborator to run git node land --final after git rebase --continue is done?

If not, can we add the reminder at the end of the output of first command git node land <PR-ID> just after:

Please run the following command to complete landing

$ git rebase upstream/master -i -x "git node land --amend"

For example:

Once you're done rebasing, ensure to run the following command (and fix issues if any) before landing:

$ git node land --final

[joyeecheung]

Adding another hint is a good idea!

(I was thinking if there is a command that adds git node land --final to the end of the sequence but not sure if there is a flag to git rebase for that)

[joyeecheung]

(Also, probably a good time to record another demo again..)

[mmarchini]

Maybe we can use git hooks (i.e., the post-rewrite hook)? I never used them, so I'm not sure if that would work.

[mmarchini]

We have the following message today:

$ git rebase upstream/master -i -x "git node land --amend" --autosquash
$ git node land --continue

--continue will run --final, so I think we're good. Feel free to reopen if you believe we should do something else though (or feel free to open another issue)