From c3467fbadb7e2f28c3f786dfabdc13c173e62055 Mon Sep 17 00:00:00 2001
From: David Anderson <danderson@tailscale.com>
Date: Mon, 3 Aug 2020 17:53:08 +0000
Subject: [PATCH] version: adjust to a pure semver version number, per
 bradfitz's proposal.

Signed-off-by: David Anderson <danderson@tailscale.com>
---
 version/mkversion.sh      | 184 +++++++++++++++++++++-----------------
 version/mkversion_test.go |  48 ++++++----
 2 files changed, 134 insertions(+), 98 deletions(-)

diff --git a/version/mkversion.sh b/version/mkversion.sh
index f9bc79b84..41c214850 100755
--- a/version/mkversion.sh
+++ b/version/mkversion.sh
@@ -5,89 +5,107 @@ set -eu
 mode=$1
 describe=$2
 
-long() {
-    ver="${describe#v}"
-    stem="${ver%%-*}"
-    case "$stem" in
-        *.*.*)
-            # Full SemVer, nothing to do.
-            semver="${stem}"
+# Git describe output overall looks like
+# MAJOR.MINOR.PATCH-NUMCOMMITS-GITHASH. Depending on the tag being
+# described and the state of the repo, ver can be missing PATCH,
+# NUMCOMMITS, or both.
+#
+# Valid values look like: 1.2.3-1234-abcdef, 0.98-1234-abcdef,
+# 1.0.0-abcdef, 0.99-abcdef.
+ver="${describe#v}"
+stem="${ver%%-*}" # Just the semver-ish bit e.g. 1.2.3, 0.98
+suffix="${ver#$stem}" # The rest e.g. -23-abcdef, -abcdef
+
+# Normalize the stem into a full major.minor.patch semver. We might
+# not use all those pieces depending on what kind of version we're
+# making, but it's good to have them all on hand.
+case "$stem" in
+    *.*.*)
+        # Full SemVer, nothing to do
+        stem="$stem"
         ;;
-        *.*)
-            # Old style major.minor, add a .0
-            semver="${stem}.0"
-            ;;
-        *)
-            echo "Unparseable version $stem" >&2
-            exit 1
-            ;;
-    esac
-    suffix="${ver#$stem}"
-    case "$suffix" in
-        -*-*)
-            # Has a change count in addition to the commit hash.
+    *.*)
+        # Old style major.minor, add a .0
+        stem="${stem}.0"
+        ;;
+    *)
+        echo "Unparseable version $stem" >&2
+        exit 1
+        ;;
+esac
+major=$(echo "$stem" | cut -f1 -d.)
+minor=$(echo "$stem" | cut -f2 -d.)
+patch=$(echo "$stem" | cut -f3 -d.)
+
+# Extract the change count and git ID from the suffix.
+case "$suffix" in
+    -*-*)
+        # Has both a change count and a commit hash.
+        changecount=$(echo "$suffix" | cut -f2 -d-)
+        githash=$(echo "$suffix" | cut -f3 -d-)
+        ;;
+    -*)
+        # Git hash only, change count is zero.
+        changecount="0"
+        githash=$(echo "$suffix" | cut -f2 -d-)
+        ;;
+    *)
+        echo "Unparseable version suffix $suffix" >&2
+        exit 1
+        ;;
+esac
+
+# Validate that the version data makes sense. Rules:
+#  - Odd number minors are unstable. Patch must be 0, and gets
+#    replaced by changecount.
+#  - Even number minors are stable. Changecount must be 0, and
+#    gets removed.
+#
+# After this section, we only use major/minor/patch, which have been
+# tweaked as needed.
+if expr "$minor" : "[0-9]*[13579]$" >/dev/null; then
+    # Unstable
+    if [ "$patch" != "0" ]; then
+        # This is a fatal error, because a non-zero patch number
+        # indicates that we created an unstable git tag in violation
+        # of our versioning policy, and we want to blow up loudly to
+        # get that fixed.
+        echo "Unstable release $describe has a non-zero patch number, which is not allowed" >&2
+        exit 1
+    fi
+    patch="$changecount"
+else
+    # Stable
+    if [ "$changecount" != "0" ]; then
+        # This is a commit that's sitting between two stable
+        # releases. We never want to release such a commit to the
+        # pbulic, but it's useful to be able to build it for
+        # debugging. Just force the version to 0.0.0, so that we're
+        # forced to rely on the git commit hash.
+        major=0
+        minor=0
+        patch=0
+    fi
+fi
+
+case "$1" in
+    long)
+        echo "${major}.${minor}.${patch}-${githash}"
+        ;;
+    short)
+        echo "${major}.${minor}.${patch}"
+        ;;
+    xcode)
+        # CFBundleShortVersionString: the "short name" used in the App
+        # Store.  eg. 0.92.98
+        echo "VERSION_NAME = ${major}.${minor}.${patch}"
+        # CFBundleVersion: the build number. Needs to be 3 numeric
+        # sections that increment for each release according to SemVer
+        # rules.
+        #
+        # We start counting at 100 because we submitted using raw
+        # build numbers before, and Apple doesn't let you start over.
+        # e.g. 0.98.3 -> 100.98.3
+        echo "VERSION_ID = $((major + 100)).${minor}.${patch}"        
         ;;
-        -*)
-            # Missing change count, add one.
-            suffix="-0${suffix}"
-            ;;
-        *)
-            echo "Unexpected version suffix" >&2
-            exit 1
-    esac
-    echo "${semver}${suffix}"
-}
-
-short() {
-    ver="$(long)"
-    case "$ver" in
-        *-*-*)
-            echo "${ver%-*}"
-            ;;
-        *-*)
-            echo "$ver"
-            ;;
-        *)
-            echo "Long version in invalid format" >&2
-            exit 1
-            ;;
-    esac
-}
-
-xcode() {
-    ver=$(short | sed -e 's/-/./')
-    major=$(echo "$ver" | cut -f1 -d.)
-    minor=$(echo "$ver" | cut -f2 -d.)
-    patch=$(echo "$ver" | cut -f3 -d.)
-    changecount=$(echo "$ver" | cut -f4 -d.)
-
-    # Apple version numbers must be major.minor.patch. We have 4 fields
-    # because we need major.minor.patch for go module compatibility, and
-    # changecount for automatic version numbering of unstable builds. To
-    # resolve this, for Apple builds, we combine changecount into patch:
-    patch=$((patch*10000 + changecount))
-
-    # CFBundleShortVersionString: the "short name" used in the App Store.
-    # eg. 0.92.98
-    echo "VERSION_NAME = $major.$minor.$patch"
-    # CFBundleVersion: the build number. Needs to be 3 numeric sections
-    # that increment for each release according to SemVer rules.
-    #
-    # We start counting at 100 because we submitted using raw build
-    # numbers before, and Apple doesn't let you start over.
-    # e.g. 0.98.3-123 -> 100.98.3123
-    major=$((major + 100))
-    echo "VERSION_ID = $major.$minor.$patch"
-}
-
-case "$mode" in
-    long)
-        long
-    ;;
-    short)
-        short
-    ;;
-    xcode)
-        xcode
-    ;;
 esac
diff --git a/version/mkversion_test.go b/version/mkversion_test.go
index c69a0a167..8e2384f55 100644
--- a/version/mkversion_test.go
+++ b/version/mkversion_test.go
@@ -15,42 +15,60 @@ func xcode(short, long string) string {
 	return fmt.Sprintf("VERSION_NAME = %s\nVERSION_ID = %s", short, long)
 }
 
-func mkversion(t *testing.T, mode, in string) string {
+func mkversion(t *testing.T, mode, in string) (string, bool) {
 	t.Helper()
 	bs, err := exec.Command("./mkversion.sh", mode, in).CombinedOutput()
 	if err != nil {
 		t.Logf("mkversion.sh output: %s", string(bs))
-		t.Fatalf("mkversion.sh %s %s: %v", mode, in, err)
+		return "", false
 	}
-	return strings.TrimSpace(string(bs))
+	return strings.TrimSpace(string(bs)), true
 }
 
 func TestMkversion(t *testing.T) {
 	tests := []struct {
 		in    string
+		ok    bool
 		long  string
 		short string
 		xcode string
 	}{
-		{"v0.98-abcdef", "0.98.0-0-abcdef", "0.98.0-0", xcode("0.98.0", "100.98.0")},
-		{"v0.98-123-abcdef", "0.98.0-123-abcdef", "0.98.0-123", xcode("0.98.123", "100.98.123")},
-		{"v0.99.5-123-abcdef", "0.99.5-123-abcdef", "0.99.5-123", xcode("0.99.50123", "100.99.50123")},
-		{"v0.99.5-123-abcdef", "0.99.5-123-abcdef", "0.99.5-123", xcode("0.99.50123", "100.99.50123")},
-		{"v2.3-0-abcdef", "2.3.0-0-abcdef", "2.3.0-0", xcode("2.3.0", "102.3.0")},
-		{"1.2.3-4-abcdef", "1.2.3-4-abcdef", "1.2.3-4", xcode("1.2.30004", "101.2.30004")},
+		{"v0.98-abcdef", true, "0.98.0-abcdef", "0.98.0", xcode("0.98.0", "100.98.0")},
+		{"v0.98.1-abcdef", true, "0.98.1-abcdef", "0.98.1", xcode("0.98.1", "100.98.1")},
+		{"v1.1.0-37-abcdef", true, "1.1.37-abcdef", "1.1.37", xcode("1.1.37", "101.1.37")},
+		{"v1.2.9-abcdef", true, "1.2.9-abcdef", "1.2.9", xcode("1.2.9", "101.2.9")},
+		{"v1.2.9-0-abcdef", true, "1.2.9-abcdef", "1.2.9", xcode("1.2.9", "101.2.9")},
+		{"v1.15.0-129-abcdef", true, "1.15.129-abcdef", "1.15.129", xcode("1.15.129", "101.15.129")},
+
+		{"v0.98-123-abcdef", true, "0.0.0-abcdef", "0.0.0", xcode("0.0.0", "100.0.0")},
+		{"v1.0.0-37-abcdef", true, "0.0.0-abcdef", "0.0.0", xcode("0.0.0", "100.0.0")},
+
+		{"v0.99.5-0-abcdef", false, "", "", ""},   // unstable, patch not allowed
+		{"v0.99.5-123-abcdef", false, "", "", ""}, // unstable, patch not allowed
+		{"v1-abcdef", false, "", "", ""},          // bad semver
+		{"v1.0", false, "", "", ""},               // missing suffix
 	}
 
 	for _, test := range tests {
-		gotlong := mkversion(t, "long", test.in)
-		gotshort := mkversion(t, "short", test.in)
-		gotxcode := mkversion(t, "xcode", test.in)
-		if gotlong != test.long {
+		gotlong, longOK := mkversion(t, "long", test.in)
+		if longOK != test.ok {
+			t.Errorf("mkversion.sh long %q ok=%v, want %v", test.in, longOK, test.ok)
+		}
+		gotshort, shortOK := mkversion(t, "short", test.in)
+		if shortOK != test.ok {
+			t.Errorf("mkversion.sh short %q ok=%v, want %v", test.in, shortOK, test.ok)
+		}
+		gotxcode, xcodeOK := mkversion(t, "xcode", test.in)
+		if xcodeOK != test.ok {
+			t.Errorf("mkversion.sh xcode %q ok=%v, want %v", test.in, xcodeOK, test.ok)
+		}
+		if longOK && gotlong != test.long {
 			t.Errorf("mkversion.sh long %q: got %q, want %q", test.in, gotlong, test.long)
 		}
-		if gotshort != test.short {
+		if shortOK && gotshort != test.short {
 			t.Errorf("mkversion.sh short %q: got %q, want %q", test.in, gotshort, test.short)
 		}
-		if gotxcode != test.xcode {
+		if xcodeOK && gotxcode != test.xcode {
 			t.Errorf("mkversion.sh xcode %q: got %q, want %q", test.in, gotxcode, test.xcode)
 		}
 	}