diff --git a/pkg/credhelper/cmd_update.go b/pkg/credhelper/cmd_update.go index 695107f..0744f61 100644 --- a/pkg/credhelper/cmd_update.go +++ b/pkg/credhelper/cmd_update.go @@ -8,6 +8,7 @@ import ( "os/exec" "path" "path/filepath" + "regexp" "runtime" "strconv" "strings" @@ -76,36 +77,79 @@ func fetchLatestVersion() (string, error) { } // isNewerVersion compares two version strings (simple semver comparison) +// describeSuffix matches the "--g" tail `git describe` appends to +// the most recent tag, as in v0.1.4-18-g8f70cce. +var describeSuffix = regexp.MustCompile(`^(.*)-(\d+)-g[0-9a-f]+$`) + +// parsedVersion is a version split into its numeric components plus, for a +// git-describe build, how many commits it sits past its tag. +type parsedVersion struct { + nums []int + ahead int +} + +// parseVersion reads "v0.1.4" and "v0.1.4-18-g8f70cce". It reports ok=false for +// anything it cannot read in full, so an unparseable version is never mistaken +// for an upgrade — the previous code swallowed strconv errors and substituted +// 0, which made every release look newer than any git-describe build. +func parseVersion(s string) (parsedVersion, bool) { + s = strings.TrimPrefix(strings.TrimSpace(s), "v") + if s == "" { + return parsedVersion{}, false + } + + var ahead int + if m := describeSuffix.FindStringSubmatch(s); m != nil { + n, err := strconv.Atoi(m[2]) + if err != nil { + return parsedVersion{}, false + } + s, ahead = m[1], n + } + + parts := strings.Split(s, ".") + nums := make([]int, 0, len(parts)) + for _, p := range parts { + n, err := strconv.Atoi(p) + if err != nil { + return parsedVersion{}, false + } + nums = append(nums, n) + } + return parsedVersion{nums: nums, ahead: ahead}, true +} + +// versionComponent reads the i'th numeric component, treating absent trailing +// components as 0 so "0.1" and "0.1.0" compare equal. +func versionComponent(nums []int, i int) int { + if i < len(nums) { + return nums[i] + } + return 0 +} + func isNewerVersion(newVersion, currentVersion string) bool { if currentVersion == "dev" { return true } - newV := strings.TrimPrefix(newVersion, "v") - curV := strings.TrimPrefix(currentVersion, "v") + newV, okNew := parseVersion(newVersion) + curV, okCur := parseVersion(currentVersion) + if !okNew || !okCur { + return false + } - newParts := strings.Split(newV, ".") - curParts := strings.Split(curV, ".") - - for i := range min(len(newParts), len(curParts)) { - newNum := 0 - if parsed, err := strconv.Atoi(newParts[i]); err == nil { - newNum = parsed - } - curNum := 0 - if parsed, err := strconv.Atoi(curParts[i]); err == nil { - curNum = parsed - } - - if newNum > curNum { - return true - } - if newNum < curNum { - return false + for i := range max(len(newV.nums), len(curV.nums)) { + a, b := versionComponent(newV.nums, i), versionComponent(curV.nums, i) + if a != b { + return a > b } } - return len(newParts) > len(curParts) + // Same tag: whichever build sits further past it is the newer one. This is + // deliberately not semver ordering, where a prerelease sorts below its + // release — a git-describe build is commits AHEAD of its tag, not behind. + return newV.ahead > curV.ahead } // goreleaserArchiveName returns the archive filename goreleaser publishes for diff --git a/pkg/credhelper/version_compare_test.go b/pkg/credhelper/version_compare_test.go new file mode 100644 index 0000000..f58b538 --- /dev/null +++ b/pkg/credhelper/version_compare_test.go @@ -0,0 +1,50 @@ +package credhelper + +import "testing" + +// isNewerVersion had no tests, and a git-describe build ("v0.1.4-18-g8f70cce") +// nagged about an update on every single invocation. The cause was a silent +// default: strconv.Atoi("4-18-g8f70cce") fails, the failure was swallowed, the +// component became 0, and so every published release compared as newer than any +// dev build of the same tag. +func TestIsNewerVersion(t *testing.T) { + tests := []struct { + name string + latest string + current string + want bool + }{ + // The reported case: a dev build 18 commits past v0.1.4 is AHEAD of + // v0.1.4, so there is nothing to update to. + {"git describe build is ahead of its own tag", "v0.1.4", "v0.1.4-18-g8f70cce", false}, + {"git describe build vs a real later release", "v0.2.0", "v0.1.4-18-g8f70cce", true}, + {"git describe build vs a later patch", "v0.1.5", "v0.1.4-18-g8f70cce", true}, + {"further ahead of the same tag", "v0.1.4-2-gabc1234", "v0.1.4-18-g8f70cce", false}, + {"behind another describe build", "v0.1.4-20-gabc1234", "v0.1.4-18-g8f70cce", true}, + + // Plain releases. + {"newer patch", "v0.1.5", "v0.1.4", true}, + {"newer minor", "v0.2.0", "v0.1.9", true}, + {"newer major", "v1.0.0", "v0.9.9", true}, + {"same version", "v0.1.4", "v0.1.4", false}, + {"older patch", "v0.1.3", "v0.1.4", false}, + {"older major", "v0.9.9", "v1.0.0", false}, + {"missing v prefix still compares", "0.1.5", "0.1.4", true}, + + // An untagged build always updates. + {"dev always updates", "v0.1.4", "dev", true}, + + // Nothing parseable means no nagging. + {"garbage latest does not nag", "not-a-version", "v0.1.4", false}, + {"empty latest does not nag", "", "v0.1.4", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isNewerVersion(tt.latest, tt.current); got != tt.want { + t.Errorf("isNewerVersion(%q, %q) = %v, want %v", + tt.latest, tt.current, got, tt.want) + } + }) + } +}