[vergo:minor-release] Look for head commit at any point of versioned branch (#14)

* Look for head commit at any point of versioned branch

- This is an attempt to fix #13, using code shamelessly stolen from src-d/go-git#611 (comment)
- Working on this in the background so opening a draft PR for now, with plans to extract this code, test it more thorougly and add error handling

Required by #13

* Renames and remove unneeded map

* Handle errors, log and default to false

* Add changelog entry

* Fix func tests

* Naming improvements

* Add memorization to speed up execution

* Avoid recursion by iterating over the git log

* Improve error messages

Author: adamdougal

CHANGELOG.md

@@ -1,5 +1,9 @@
 # Changelog
 
+## [0.27.0] - 08-02-2023
+Fixed bug introduced in version `0.21.0` where `vergo bump` and `vergo check` would fail if the current commit is not 
+the latest on a versioned branch e.g. `master` or `main`.
+
 ## [0.26.0] - 15-11-2022
 Fixed bug in tag prefix trimming.
 

bump/bump_test.go

@@ -1,6 +1,7 @@
 package bump_test
 
 import (
+	"fmt"
 	"github.com/Masterminds/semver/v3"
 	"github.com/go-git/go-billy/v5/memfs"
 	gogit "github.com/go-git/go-git/v5"
@@ -115,7 +116,7 @@ func TestBumpShouldFailWhenNotOnMainBranch(t *testing.T) {
 				r.BranchExists(branchName)
 				assert.Equal(t, branchName, r.Head().Name().Short())
 				_, err = Bump(r.Repo, increment, Options{TagPrefix: prefix, VersionedBranches: mainBranch})
-				assert.Regexp(t, "branch apple is not in main branches list: master, main", err)
+				assert.Regexp(t, "branch apple is not in versioned branches list: master, main", err)
 			})
 		}
 	}
@@ -161,7 +162,7 @@ func TestBumpShouldNOTWorkWhenHeadlessCheckoutOfOtherBranch(t *testing.T) {
 				assert.Equal(t, plumbing.HEAD.String(), r.Head().Name().Short())
 
 				_, err = Bump(r.Repo, increment, Options{TagPrefix: prefix, VersionedBranches: mainBranch})
-				assert.Regexp(t, "invalid headless checkout", err)
+				assert.Equal(t, fmt.Sprintf("commit %s is not on a versioned branch: master, main", latestHashOnApple.String()), err.Error())
 			})
 		}
 	}

cmd/cmd_check_test.go

@@ -1,6 +1,7 @@
 package cmd_test
 
 import (
+	"fmt"
 	. "github.com/sky-uk/vergo/internal-test"
 	"github.com/sky-uk/vergo/release"
 	"github.com/stretchr/testify/assert"
@@ -27,7 +28,8 @@ func TestCheckIncrementHintSuccess(t *testing.T) {
 
 func TestCheckIncrementHintSkipHint(t *testing.T) {
 	_, tempDir := PersistentRepository(t)
-	cmd, buffer := makeCheckFail(t, release.ErrSkipRelease, release.ErrInvalidHeadless, release.ErrNoIncrement)
+	expectedError := fmt.Errorf("commit %s is not on a versioned branch: %s", "blah", "blah")
+	cmd, buffer := makeCheckFail(t, release.ErrSkipRelease, expectedError, release.ErrNoIncrement)
 	cmd.SetArgs([]string{"check", "increment-hint", "--repository-location", tempDir, "-t", "some-prefix", "--log-level", "error"})
 	err := cmd.Execute()
 	assert.Nil(t, err)
@@ -54,20 +56,22 @@ func TestCheckIncrementHintFailure(t *testing.T) {
 
 func TestCheckReleaseManyFailures(t *testing.T) {
 	_, tempDir := PersistentRepository(t)
-	cmd, buffer := makeCheckFail(t, release.ErrSkipRelease, release.ErrInvalidHeadless, success)
+	expectedError := fmt.Errorf("commit %s is not on a versioned branch: %s", "blah", "blah")
+	cmd, buffer := makeCheckFail(t, release.ErrSkipRelease, expectedError, success)
 	cmd.SetArgs([]string{"check", "release", "--repository-location", tempDir, "-t", "some-prefix", "--log-level", "error"})
 	err := cmd.Execute()
 	assert.NotNil(t, err)
 	assert.Equal(t, `Error: skip release hint present
-HEAD validation: invalid headless checkout
+commit blah is not on a versioned branch: blah
 `, readBuffer(t, buffer))
 }
 
 func TestCheckReleaseInvalidHeadless(t *testing.T) {
 	_, tempDir := PersistentRepository(t)
-	cmd, buffer := makeCheckFail(t, success, release.ErrInvalidHeadless, success)
+	expectedError := fmt.Errorf("commit %s is not on a versioned branch: %s", "blah", "blah")
+	cmd, buffer := makeCheckFail(t, success, expectedError, success)
 	cmd.SetArgs([]string{"check", "release", "--repository-location", tempDir, "-t", "some-prefix", "--log-level", "error"})
 	err := cmd.Execute()
 	assert.NotNil(t, err)
-	assert.Equal(t, "Error: HEAD validation: invalid headless checkout\n", readBuffer(t, buffer))
+	assert.Equal(t, "Error: commit blah is not on a versioned branch: blah\n", readBuffer(t, buffer))
 }

fun-tests/test.sh

@@ -174,15 +174,15 @@ remote_tags=$(git ls-remote --tags origin apple-0.2.0 app-0.1.1 banana-2.0.0 ora
 setUpLikeCI
 
 [[ "$(git branch -l | grep -v HEAD)" == "" ]] #make sure no local branch
-git checkout 117443bb
-[[ "$(vergo check release --tag-prefix=apple 2>&1)" == *'invalid headless checkout'* ]]
-[[ "$(vergo bump minor --tag-prefix=apple 2>&1)" == *'invalid headless checkout'* ]]
+git checkout d13ae40 #commit on the `test-branch` branch
+[[ "$(vergo check release --tag-prefix=apple 2>&1)" == *' is not on a versioned branch'* ]]
+[[ "$(vergo bump minor --tag-prefix=apple 2>&1)" == *' is not on a versioned branch'* ]]
 
 [[ "$(git branch -l | grep -v HEAD)" == "" ]] #make sure no local branch
 git checkout a54f1f7
 vergo check release --tag-prefix=apple
 [[ "$(vergo bump minor --tag-prefix=apple --log-level=trace 2>&1)" == *'Set tag apple-0.2.0'* ]]
 
 git checkout origin/master -b master #create local branch
-[[ "$(vergo check release --tag-prefix=apple --log-level=trace --versioned-branch-names main 2>&1)" == *'branch master is not in main branches list: main'* ]]
-[[ "$(vergo bump minor --tag-prefix=apple --log-level=trace --versioned-branch-names main 2>&1)" == *'branch master is not in main branches list: main'* ]]
+[[ "$(vergo check release --tag-prefix=apple --log-level=trace --versioned-branch-names main 2>&1)" == *'branch master is not in versioned branches list: main'* ]]
+[[ "$(vergo bump minor --tag-prefix=apple --log-level=trace --versioned-branch-names main 2>&1)" == *'branch master is not in versioned branches list: main'* ]]

release/release.go

@@ -6,17 +6,17 @@ import (
 	"github.com/Masterminds/semver/v3"
 	gogit "github.com/go-git/go-git/v5"
 	"github.com/go-git/go-git/v5/plumbing"
+	"github.com/go-git/go-git/v5/plumbing/object"
+	"github.com/go-git/go-git/v5/plumbing/storer"
 	log "github.com/sirupsen/logrus"
 	"github.com/thoas/go-funk"
 	"regexp"
 	"strings"
 )
 
 var (
-	ErrNoIncrement     = errors.New("increment hint not present")
-	ErrSkipRelease     = errors.New("skip release hint present")
-	ErrHEADValidation  = errors.New("HEAD validation")
-	ErrInvalidHeadless = fmt.Errorf("%w: %s", ErrHEADValidation, "invalid headless checkout")
+	ErrNoIncrement = errors.New("increment hint not present")
+	ErrSkipRelease = errors.New("skip release hint present")
 )
 
 func checkSkipHint(aString, tagPrefix string) bool {
@@ -85,7 +85,8 @@ func ValidateHEAD(repo *gogit.Repository, remoteName string, versionedBranches [
 		return err
 	}
 	log.Debugf("Current branch:%v, short: %v", head.Name(), head.Name().Short())
-	if head.Name() == plumbing.HEAD {
+	isHeadlessCheckout := head.Name() == plumbing.HEAD
+	if isHeadlessCheckout {
 		validRef := false
 		for _, mainBranchName := range versionedBranches {
 			remote, err := repo.Remote(remoteName)
@@ -102,20 +103,27 @@ func ValidateHEAD(repo *gogit.Repository, remoteName string, versionedBranches [
 			if err != nil {
 				log.WithError(err).Debugf("branchRef could not be resolved: %s\n", branchRef.String())
 			} else {
-				if revision.String() == head.Hash().String() {
+				commitOnVersionedBranch, err := isCommitOnBranch(repo, head.Hash(), branchRef)
+				if err != nil {
+					log.WithError(err).Errorf("Failed to check if commit %s is on branch %s\n",
+						head.Hash().String(), branchRef.String())
+				}
+
+				if commitOnVersionedBranch {
 					validRef = true
 					break
 				} else {
-					log.Tracef("Invalid ref [branch: %s, head: %s, ref: %s]\n",
+					log.Warnf("Commit not found on branch [branch: %s, head: %s, ref: %s]\n",
 						branchRef.String(), head.Hash().String(), revision.String())
 				}
 			}
 		}
 		if !validRef {
-			return ErrInvalidHeadless
+			return fmt.Errorf("commit %s is not on a versioned branch: %s",
+				head.Hash(), strings.Join(versionedBranches, ", "))
 		}
 	} else if !funk.ContainsString(versionedBranches, head.Name().Short()) {
-		return fmt.Errorf("%w: branch %s is not in main branches list: %s", ErrHEADValidation,
+		return fmt.Errorf("branch %s is not in versioned branches list: %s",
 			head.Name().Short(), strings.Join(versionedBranches, ", "))
 	}
 	return nil
@@ -142,3 +150,33 @@ func PreRelease(repo *gogit.Repository, options PreReleaseOptions) PreReleaseFun
 		return pre, nil
 	}
 }
+
+func isCommitOnBranch(repo *gogit.Repository, commit plumbing.Hash, branch plumbing.ReferenceName) (bool, error) {
+	branchRef, err := repo.Reference(branch, true)
+	if err != nil {
+		return false, err
+	}
+
+	branchGitLog, err := repo.Log(&gogit.LogOptions{
+		From: branchRef.Hash(),
+	})
+
+	if err != nil {
+		return false, err
+	}
+
+	reaches := false
+	err = branchGitLog.ForEach(func(branchCommit *object.Commit) error {
+		if branchCommit.Hash == commit {
+			reaches = true
+			return storer.ErrStop
+		}
+		return nil
+	})
+
+	if err != nil {
+		return false, err
+	}
+
+	return reaches, nil
+}

release/release_test.go

@@ -1,6 +1,7 @@
 package release_test
 
 import (
+	"fmt"
 	gogit "github.com/go-git/go-git/v5"
 	"github.com/go-git/go-git/v5/plumbing"
 	. "github.com/sky-uk/vergo/internal-test"
@@ -157,7 +158,7 @@ func TestShouldFailWhenNotOnMainBranch(t *testing.T) {
 				r.BranchExists(branchName)
 				assert.Equal(t, branchName, r.Head().Name().Short())
 				err = release.ValidateHEAD(r.Repo, remoteName, mainBranch)
-				assert.Regexp(t, "branch apple is not in main branches list: master, main", err)
+				assert.Regexp(t, "branch apple is not in versioned branches list: master, main", err)
 			})
 		}
 	}
@@ -180,6 +181,25 @@ func TestShouldWorkWhenHeadlessCheckoutOfMainBranch(t *testing.T) {
 	}
 }
 
+//nolint:scopelint,paralleltest
+func TestShouldWorkWhenOnHeadlessCheckoutOfOldCommitOnMainBranch(t *testing.T) {
+	for _, prefix := range prefixes {
+		for _, increment := range increments {
+			t.Run(prefix+"-"+increment, func(t *testing.T) {
+				r := NewTestRepo(t)
+				oldCommit := r.Head().Hash()
+				r.DoCommit("foo")
+				err := r.Worktree().Checkout(&gogit.CheckoutOptions{Hash: oldCommit})
+				assert.Nil(t, err)
+				assert.Equal(t, plumbing.HEAD.String(), r.Head().Name().Short())
+
+				err = release.ValidateHEAD(r.Repo, remoteName, mainBranch)
+				assert.Nil(t, err)
+			})
+		}
+	}
+}
+
 //nolint:scopelint,paralleltest
 func TestShouldNOTWorkWhenHeadlessCheckoutOfOtherBranch(t *testing.T) {
 	for _, prefix := range prefixes {
@@ -203,7 +223,7 @@ func TestShouldNOTWorkWhenHeadlessCheckoutOfOtherBranch(t *testing.T) {
 				assert.Equal(t, plumbing.HEAD.String(), r.Head().Name().Short())
 
 				err = release.ValidateHEAD(r.Repo, remoteName, mainBranch)
-				assert.Regexp(t, "invalid headless checkout", err)
+				assert.Equal(t, fmt.Sprintf("commit %s is not on a versioned branch: master, main", latestHashOnApple.String()), err.Error())
 			})
 		}
 	}