Fix a bug which prevented force pushing to a previous commit in a branch

---
type: change
description: |-
  Fix a bug which prevented force pushing to a previous commit in a branch

  This bug was caused because VerifyCanSetBranchHEADTo did not properly handle the
  case; what would happen is that the merge-base of the old HEAD and the new one
  would be taken, and then the range of that merge-base result to the new commit
  would be retrieved. But since the merge-base of the old HEAD and the new would
  just be the new HEAD, it'd be taking the range from new HEAD to new HEAD; and
  empty range.

  This commit adds an ancestry check prior to the merge-base, to manually account
  for this case, and calls verifyCommit directly if it comes about. It's not very
  pretty, but it works.
fingerprint: AEYAvGa1GIfbmjh/gqu2l7qdhQtlJIGHhJd1GaZ5D87h
credentials:
- type: pgp_signature
  pub_key_id: 95C46FA6A41148AC
  body: iQIzBAABAgAdFiEEJ6tQKp6olvZKJ0lwlcRvpqQRSKwFAl6t5d0ACgkQlcRvpqQRSKyjuw//fpTDF/PSBHlmZUOV6k9mffdOdSR5cwGZ1K2b8Nn5astdNsnIW3F+4u2M5X98xFDTbmsB2Dt0jhRX00Gx9rLC5hO50GCE/z2VkEpHNgZbn4sv1SEGkdSAbo/zogB0+bDTVwH4HxKgQwm2R/u/4OGT2i0skP2xhRaXa7HQZuPtzAajc+JVkoX8ZJAbJX2xXoo9vmmgXvxI6lBP+xJMVQCDrLTHKuAeN9ouJM4AmDgyDKGok8o+N5i4QW6axWjrbEpPtnzcR/SOBO4lXnhJUTACYOt4c3GlOVcb3zc8QRCJpUe6YC4nwIi59oyEuu+IIwjgu5SUsKgqNNAzKTJUn7aRlPeqncDw00OGO93u2Y6A8/E5yGnQZ6wFlZuR4NO3t4lydhYOGY4hDs+34OvhDzFCLOWvxkjuG8ArTSwp45zavM2fPWyt+2c0g7CDdHDr6xQspcvL2tn1Ot1gXZQIMmUvM3SLqPmcAegihiFSCzcuQX+61WMyYbmaHy3/bFr5jZYF7wOsj2AgMWkFmACbfT0TlMt5E3BDB7bED1jVq4sXnT+v2VaYD0ASTg3bhmjJ7T298P0QCRbL2QtnPT1qeLsCqaEzA8INl8s5Yp5rt6yde0nVyRzHQkyQyAoDzKLQ1OSWmYt29WN4cGQh0UhpCnltCGVQpD36BkcvHwc7W5Vy190=
  account: mediocregopher

payload.go

@@ -416,9 +416,11 @@ func (proj *Project) verifyCommit(
 	parentTree *object.Tree,
 	isNonFF bool,
 ) error {
-	parentTree, err := proj.parentTree(commit.Object)
+	if parentTree == nil {
-	if err != nil {
+		var err error
-		return fmt.Errorf("retrieving parent tree of commit: %w", err)
+		if parentTree, err = proj.parentTree(commit.Object); err != nil {
+			return fmt.Errorf("retrieving parent tree of commit: %w", err)
+		}
 	}
 
 	var sigFS fs.FS
@@ -573,21 +575,31 @@ func (proj *Project) VerifyCanSetBranchHEADTo(branchName plumbing.ReferenceName,
 		return fmt.Errorf("retrieving commit object %q: %w", oldCommitRef.Hash(), err)
 	}
 
-	newCommitObj, err := proj.GitRepo.CommitObject(hash)
+	newCommit, err := proj.GetCommit(hash)
 	if err != nil {
-		return fmt.Errorf("retrieving commit object %q: %w", hash, err)
+		return fmt.Errorf("retrieving commit %q: %w", hash, err)
 	}
 
-	mbCommits, err := oldCommitObj.MergeBase(newCommitObj)
+	if isAncestor, err := newCommit.Object.IsAncestor(oldCommitObj); err != nil {
+		return fmt.Errorf("determining if %q is an ancestor of %q: %w",
+			newCommit.Hash, oldCommitObj.Hash, err)
+	} else if isAncestor {
+		// if the new commit is an ancestor of the old one then the branch is
+		// being force-pushed to a previous commit. This is weird to handle
+		// using VerifyCommits, so just call verifyCommit directly.
+		return proj.verifyCommit(branchName, newCommit, nil, true)
+	}
+
+	mbCommits, err := oldCommitObj.MergeBase(newCommit.Object)
 	if err != nil {
 		return fmt.Errorf("determining merge-base between %q and %q: %w",
-			oldCommitObj.Hash, newCommitObj.Hash, err)
+			oldCommitObj.Hash, newCommit.Hash, err)
 	} else if len(mbCommits) == 0 {
 		return fmt.Errorf("%q and %q have no ancestors in common",
-			oldCommitObj.Hash, newCommitObj.Hash)
+			oldCommitObj.Hash, newCommit.Hash)
 	} else if len(mbCommits) == 2 {
 		return fmt.Errorf("%q and %q have more than one ancestor in common",
-			oldCommitObj.Hash, newCommitObj.Hash)
+			oldCommitObj.Hash, newCommit.Hash)
 	}
 
 	commits, err := proj.GetCommitRange(mbCommits[0].Hash, hash)

payload_test.go

@@ -147,7 +147,7 @@ func TestNonFastForwardCommits(t *testing.T) {
 	}
 }
 
-func TestCanSetBranchHEADTo(t *testing.T) {
+func TestVerifyCanSetBranchHEADTo(t *testing.T) {
 	type toTest struct {
 		// branchName and hash are the arguments passed into
 		// VerifyCanSetBranchHEADTo.
@@ -391,6 +391,24 @@ func TestCanSetBranchHEADTo(t *testing.T) {
 				}
 			},
 		},
+		{
+			descr: "branch nonff to previous commit",
+			init: func(h *harness, rootSig sigcred.Signifier) toTest {
+				h.assertCommitChange(verifySkip, "init", rootSig)
+
+				other := plumbing.NewBranchReferenceName("other")
+				h.checkout(other)
+				h.stage(map[string]string{"foo": "foo"})
+				fooCommit := h.assertCommitChange(verifySkip, "foo", rootSig)
+				h.stage(map[string]string{"bar": "bar"})
+				h.assertCommitChange(verifySkip, "bar", rootSig)
+
+				return toTest{
+					branchName: other,
+					hash:       fooCommit.Hash,
+				}
+			},
+		},
 	}
 
 	for _, test := range tests {