git-node: refuse to run without configurations/on wrong revs (nodejs#200)

joyeecheung · web-flow · commit 7a274ab0a55a · 2018-03-06T04:03:41.000+08:00
It now refuses to run if 1. User has not configured upstream or branch - no more defaults. 2. User is not on the branch configured 3. User is on detached HEAD Refs: nodejs/node#18914 Fixes: nodejs#198
diff --git a/components/git/epilogue.js b/components/git/epilogue.js
@@ -3,6 +3,12 @@
 module.exports = `Steps to land a pull request:
 ==============================================================================
 $ cd path/to/node/project
+
+# If you have not configured it before
+$ ncu-config set upstream <name-of-remote-to-nodejs/node>
+$ ncu-config set branch master   # Assuming you are landing commits on master
+
+$ git checkout master
 $ git node land --abort          # Abort a landing session, just in case
 $ git node land $PRID            # Start a new landing session
 
diff --git a/components/git/land.js b/components/git/land.js
@@ -103,6 +103,9 @@ module.exports = {
 
 async function main(state, argv, cli, req, dir) {
   let session = new LandingSession(cli, req, dir);
+  if (session.warnForMissing() || session.warnForWrongBranch()) {
+    return;
+  }
 
   try {
     session.restore();
diff --git a/docs/git-node.md b/docs/git-node.md
@@ -8,11 +8,7 @@ A custom Git command for managing pull requests. You can run it as
 1. See the readme on how to
   [set up credentials](../README.md#setting-up-credentials).
 1. It's a Git command, so make sure you have Git installed, of course.
-1. Configure your upstream remote and branch name. By default it assumes your
-   remote pointing to https://github.com/nodejs/node is called `upstream`, and
-   the branch that you are trying to land PRs on is `master`. If that's not the
-   case:
-
+1. Configure your upstream remote and branch name.
    ```
    $ cd path/to/node/project
    $ ncu-config set upstream your-remote-name
@@ -28,6 +24,12 @@ A custom Git command for managing pull requests. You can run it as
 Steps to land a pull request:
 ==============================================================================
 $ cd path/to/node/project
+
+# If you have not configured it before
+$ ncu-config set upstream <name-of-remote-to-nodejs/node>
+$ ncu-config set branch master   # Assuming you are landing commits on master
+
+$ git checkout master
 $ git node land --abort          # Abort a landing session, just in case
 $ git node land $PRID            # Start a new landing session
 
diff --git a/lib/landing_session.js b/lib/landing_session.js
@@ -138,14 +138,22 @@ class LandingSession extends Session {
     // TODO: do git rebase automatically?
   }
 
+  getCurrentRev() {
+    return runSync('git', ['rev-parse', 'HEAD']).trim();
+  }
+
+  getCurrentBranch() {
+    return runSync('git', ['rev-parse', '--abbrev-ref', 'HEAD']).trim();
+  }
+
   async amend() {
     const { cli } = this;
     if (!this.readyToAmend()) {
       cli.warn('Not yet ready to amend, run `git node land --abort`');
       return;
     }
 
-    const rev = runSync('git', ['rev-parse', 'HEAD']);
+    const rev = this.getCurrentRev();
     const original = runSync('git', [
       'show', 'HEAD', '-s', '--format=%B'
     ]).trim();
@@ -311,11 +319,60 @@ class LandingSession extends Session {
 
     const branchName = `${upstream}/${branch}`;
     const shouldResetHead = await cli.prompt(
-      `Do you want to try reset the branch to ${branchName}?`);
+      `Do you want to try reset the local ${branch} branch to ${branchName}?`);
     if (shouldResetHead) {
       await this.tryResetHead();
     }
   }
+
+  warnForMissing() {
+    const { upstream, branch, cli } = this;
+    const missing = !upstream || !branch;
+    if (!branch) {
+      cli.warn('You have not told git-node what branch you are trying' +
+               ' to land commits on.');
+      cli.separator();
+      cli.info(
+        'For example, if your want to land commits on the ' +
+        '`master` branch, you can run:\n\n' +
+        '  $ ncu-config set branch master');
+      cli.separator();
+    }
+    if (!upstream) {
+      cli.warn('You have not told git-node the remote you want to sync with.');
+      cli.separator();
+      cli.info(
+        'For example, if your remote pointing to nodejs/node is' +
+        ' `remote-upstream`, you can run:\n\n' +
+        '  $ ncu-config set upstream remote-upstream');
+      cli.separator();
+    }
+    return missing;
+  }
+
+  warnForWrongBranch() {
+    const { branch, cli } = this;
+    let rev = this.getCurrentBranch();
+    if (rev === 'HEAD') {
+      cli.warn(
+        'You are in detached HEAD state. Please run git-node on a valid ' +
+        'branch');
+      return true;
+    }
+    if (rev === branch) {
+      return false;
+    }
+    cli.warn(
+      `You are running git-node-land on \`${rev}\`,\n   but you have` +
+      ` configured \`${branch}\` to be the branch to land commits.`);
+    cli.separator();
+    cli.info(
+      `You can switch to \`${branch}\` with \`git checkout ${branch}\`, or\n` +
+      `   reconfigure the target branch with:\n\n` +
+      `  $ ncu-config set branch ${rev}`);
+    cli.separator();
+    return true;
+  }
 }
 
 module.exports = LandingSession;
diff --git a/lib/session.js b/lib/session.js
@@ -41,11 +41,11 @@ class Session {
   }
 
   get upstream() {
-    return this.config.upstream || 'upstream';
+    return this.config.upstream;
   }
 
   get branch() {
-    return this.config.branch || 'master';
+    return this.config.branch;
   }
 
   get pullName() {

Original file line number	Diff line number	Diff line change
`@@ -41,11 +41,11 @@ class Session {`
`41`	`41`	`}`
`42`	`42`
`43`	`43`	`get upstream() {`
`44`		`- return this.config.upstream \|\| 'upstream';`
	`44`	`+ return this.config.upstream;`
`45`	`45`	`}`
`46`	`46`
`47`	`47`	`get branch() {`
`48`		`- return this.config.branch \|\| 'master';`
	`48`	`+ return this.config.branch;`
`49`	`49`	`}`
`50`	`50`
`51`	`51`	`get pullName() {`