Improvements to update notifier

- Only report an update available if the version number of the release
  is greater than the current version

- Removes `command` dependency from `update` package; instead, pass
  current version as an argument

- Remove `brew upgrade` instructions since we can't be certain that gh
  was installed via Homebrew in the first place.

- Does not check for updates unless stderr is a tty

- Preserve stderr color output even if stdout is not a tty

- Fixes stderr color output on Windows

go.mod

@@ -4,6 +4,7 @@ go 1.13
 
 require (
 	github.com/AlecAivazis/survey/v2 v2.0.4
+	github.com/hashicorp/go-version v1.2.0
 	github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51
 	github.com/mattn/go-colorable v0.1.2
 	github.com/mattn/go-isatty v0.0.9

go.sum

@@ -13,6 +13,8 @@ github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSs
 github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
 github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/fsnotify/fsnotify v1.4.7/go.mod h1:jwhsz4b93w/PPRr/qN1Yymfu8t87LnFCMoQvtojpjFo=
+github.com/hashicorp/go-version v1.2.0 h1:3vNe/fWF5CBgRIguda1meWhsZHy3m8gCJ5wx+dIzX/E=
+github.com/hashicorp/go-version v1.2.0/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09ZGVZPK5anwXA=
 github.com/hashicorp/hcl v1.0.0/go.mod h1:E5yfLk+7swimpb2L/Alb/PJmXilQ/rhwaUYs4T20WEQ=
 github.com/hinshun/vt10x v0.0.0-20180616224451-1954e6464174 h1:WlZsjVhE8Af9IcZDGgJGQpNflI3+MJSBhsgT5PCtzBQ=
 github.com/hinshun/vt10x v0.0.0-20180616224451-1954e6464174/go.mod h1:DqJ97dSdRW1W22yXSB90986pcOyQ7r45iio1KN2ez1A=

main.go

@@ -7,13 +7,20 @@ import (
 
 	"github.com/github/gh-cli/command"
 	"github.com/github/gh-cli/update"
+	"github.com/github/gh-cli/utils"
+	"github.com/mattn/go-isatty"
+	"github.com/mgutz/ansi"
 )
 
 var updaterEnabled = "no"
 
 func main() {
-	updateMessageChan := make(chan *string)
-	go updateInBackground(updateMessageChan)
+	currentVersion := command.Version
+	updateMessageChan := make(chan *update.ReleaseInfo)
+	go func() {
+		rel, _ := checkForUpdate(currentVersion)
+		updateMessageChan <- rel
+	}()
 
 	if cmd, err := command.RootCmd.ExecuteC(); err != nil {
 		fmt.Fprintln(os.Stderr, err)
@@ -24,23 +31,29 @@ func main() {
 		os.Exit(1)
 	}
 
-	updateMessage := <-updateMessageChan
-	if updateMessage != nil {
-		fmt.Fprintf(os.Stderr, *updateMessage)
+	newRelease := <-updateMessageChan
+	if newRelease != nil {
+		msg := fmt.Sprintf(`A new release of gh is available: %s → %s
+Release notes: %s`, currentVersion, newRelease.Version, newRelease.URL)
+		stderr := utils.NewColorable(os.Stderr)
+		fmt.Fprintf(stderr, "\n\n%s\n\n", ansi.Color(msg, "cyan"))
 	}
 }
 
-func updateInBackground(updateMessageChan chan *string) {
-	if updaterEnabled != "yes" {
-		updateMessageChan <- nil
-		return
+func shouldCheckForUpdate() bool {
+	errFd := os.Stderr.Fd()
+	return updaterEnabled == "yes" && (isatty.IsTerminal(errFd) || isatty.IsCygwinTerminal(errFd))
+}
+
+func checkForUpdate(currentVersion string) (*update.ReleaseInfo, error) {
+	if !shouldCheckForUpdate() {
+		return nil, nil
 	}
 
 	client, err := command.BasicClient()
 	if err != nil {
-		updateMessageChan <- nil
-		return
+		return nil, err
 	}
 
-	updateMessageChan <- update.UpdateMessage(client)
+	return update.CheckForUpdate(client, "github/homebrew-gh", currentVersion)
 }

test/fixtures/latestRelease.json

@@ -1,4 +0,0 @@
-{
-  "tag_name": "v1.0.0",
-  "html_url": "https://www.spacejam.com/archive/spacejam/movie/jam.htm"
-}

update/update.go

@@ -2,42 +2,35 @@ package update
 
 import (
 	"fmt"
-	"os"
 
 	"github.com/github/gh-cli/api"
-	"github.com/github/gh-cli/command"
-	"github.com/github/gh-cli/utils"
+	"github.com/hashicorp/go-version"
 )
 
-const nwo = "github/homebrew-gh"
-
-type releaseInfo struct {
+// ReleaseInfo stores information about a release
+type ReleaseInfo struct {
 	Version string `json:"tag_name"`
 	URL     string `json:"html_url"`
 }
 
-func UpdateMessage(client *api.Client) *string {
-	latestRelease, err := getLatestRelease(client)
+// CheckForUpdate checks whether this software has had a newer relase on GitHub
+func CheckForUpdate(client *api.Client, repo, currentVersion string) (*ReleaseInfo, error) {
+	latestRelease := ReleaseInfo{}
+	err := client.REST("GET", fmt.Sprintf("repos/%s/releases/latest", repo), nil, &latestRelease)
 	if err != nil {
-		fmt.Fprintf(os.Stderr, "%+v", err)
-		return nil
+		return nil, err
 	}
 
-	updateAvailable := latestRelease.Version != command.Version
-	if updateAvailable {
-		alertMsg := fmt.Sprintf(utils.Cyan(`
-A new version of gh is available! %s → %s
-Changelog: %s
-Run 'brew upgrade gh' to upgrade to the latest version!`)+"\n\n", command.Version, latestRelease.Version, latestRelease.URL)
-		return &alertMsg
+	if versionGreaterThan(latestRelease.Version, currentVersion) {
+		return &latestRelease, nil
 	}
 
-	return nil
+	return nil, nil
 }
 
-func getLatestRelease(client *api.Client) (*releaseInfo, error) {
-	path := fmt.Sprintf("repos/%s/releases/latest", nwo)
-	var r releaseInfo
-	err := client.REST("GET", path, nil, &r)
-	return &r, err
+func versionGreaterThan(v, w string) bool {
+	vv, ve := version.NewVersion(v)
+	vw, we := version.NewVersion(w)
+
+	return ve == nil && we == nil && vv.GreaterThan(vw)
 }

update/update_test.go

@@ -1,34 +1,89 @@
 package update
 
 import (
-	"os"
-	"strings"
+	"bytes"
+	"fmt"
 	"testing"
 
 	"github.com/github/gh-cli/api"
-	"github.com/github/gh-cli/command"
 )
 
-func TestRunWhileCheckingForUpdate(t *testing.T) {
-	originalVersion := command.Version
-	command.Version = "v0.0.0"
-	defer func() {
-		command.Version = originalVersion
-	}()
-
-	http := &api.FakeHTTP{}
-	jsonFile, _ := os.Open("../test/fixtures/latestRelease.json")
-	defer jsonFile.Close()
-	http.StubResponse(200, jsonFile)
-
-	client := api.NewClient(api.ReplaceTripper(http))
-	alertMsg := *UpdateMessage(client)
-
-	if !strings.Contains(alertMsg, command.Version) {
-		t.Errorf("expected: \"%v\" to contain \"%v\"", alertMsg, command.Version)
+func TestCheckForUpdate(t *testing.T) {
+	scenarios := []struct {
+		Name           string
+		CurrentVersion string
+		LatestVersion  string
+		LatestURL      string
+		ExpectsResult  bool
+	}{
+		{
+			Name:           "latest is newer",
+			CurrentVersion: "v0.0.1",
+			LatestVersion:  "v1.0.0",
+			LatestURL:      "https://www.spacejam.com/archive/spacejam/movie/jam.htm",
+			ExpectsResult:  true,
+		},
+		{
+			Name:           "current is prerelease",
+			CurrentVersion: "v1.0.0-pre.1",
+			LatestVersion:  "v1.0.0",
+			LatestURL:      "https://www.spacejam.com/archive/spacejam/movie/jam.htm",
+			ExpectsResult:  true,
+		},
+		{
+			Name:           "latest is current",
+			CurrentVersion: "v1.0.0",
+			LatestVersion:  "v1.0.0",
+			LatestURL:      "https://www.spacejam.com/archive/spacejam/movie/jam.htm",
+			ExpectsResult:  false,
+		},
+		{
+			Name:           "latest is older",
+			CurrentVersion: "v0.10.0-pre.1",
+			LatestVersion:  "v0.9.0",
+			LatestURL:      "https://www.spacejam.com/archive/spacejam/movie/jam.htm",
+			ExpectsResult:  false,
+		},
 	}
 
-	if !strings.Contains(alertMsg, "v1.0.0") {
-		t.Errorf("expected: \"%v\" to contain \"%v\"", alertMsg, "v1.0.0")
+	for _, s := range scenarios {
+		t.Run(s.Name, func(t *testing.T) {
+			http := &api.FakeHTTP{}
+			client := api.NewClient(api.ReplaceTripper(http))
+			http.StubResponse(200, bytes.NewBufferString(fmt.Sprintf(`{
+				"tag_name": "%s",
+				"html_url": "%s"
+			}`, s.LatestVersion, s.LatestURL)))
+
+			rel, err := CheckForUpdate(client, "OWNER/REPO", s.CurrentVersion)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			if len(http.Requests) != 1 {
+				t.Fatalf("expected 1 HTTP request, got %d", len(http.Requests))
+			}
+			requestPath := http.Requests[0].URL.Path
+			if requestPath != "/repos/OWNER/REPO/releases/latest" {
+				t.Errorf("HTTP path: %q", requestPath)
+			}
+
+			if !s.ExpectsResult {
+				if rel != nil {
+					t.Fatal("expected no new release")
+				}
+				return
+			}
+			if rel == nil {
+				t.Fatal("expected to report new release")
+			}
+
+			if rel.Version != s.LatestVersion {
+				t.Errorf("Version: %q", rel.Version)
+			}
+			if rel.URL != s.LatestURL {
+				t.Errorf("URL: %q", rel.URL)
+			}
+		})
 	}
 }