Support non-fastforward commits

---
type: change
message: |-
  Support non-fastforward commits

  This includes:

  * adding a filter to accessctl which can be used to allow non-ff commits, and
    augmenting the default access controls to deny non-ff commits for master.
    VerifyCommits was then modified to use that new functionality, and then tests
    were added to cover that.

  * adding a `VerifyBranchCanSetHEADTo` method, and using that in the pre-receive
    hook rather than putting all the logic in the hook itself.
    `VerifyBranchCanSetHEADTo` is thoroughly tested, and the tests for it ended up
    uncovering some broken aspects of `VerifyCommits` as well, so those are fixed
    too.
change_hash: ACTyCsTFBnAjGAek355IU3I6MioLIx5mb1mS4YjMUrF5
credentials:
- type: pgp_signature
  pub_key_id: 95C46FA6A41148AC
  body: iQIzBAABAgAdFiEEJ6tQKp6olvZKJ0lwlcRvpqQRSKwFAl6jPvsACgkQlcRvpqQRSKxfag/+JD8bs7zbFZc3XzLWz3vOhPl3OaxdXbQoqlCVywBSZ1dHrJ7BtbTltQpRgNRv+Khs/ibQAUphDFKsAauF7IKZu2fcluMYH1kulEZsYzHFZUz3zDNcPtZhD/KdPgBRSa4tv76iaeCvGGv7Eb9zHxzYiXofkf8Bkn7n63D3aE1N3MhceSPAU07johiZnjXpb2UGonLq1kQlCcEAy57H82iv0N21QjJmZ/bSNgT9d6c9kEb4lmOCs1ZWvW7kzqVLXkhgZ2/77nLKTaFvsTjA6MOodD2vrLQ4KmHmWLjYA2PmqMLkSKoMIUQhatIZiBiJNvF0HztPiIhCJLVwu5eGnVGQwMR74IOBoATlb8R7FuqOhX70b4B0W8O7ovIDWM5dNatKyrzJkJ9lWPX61dP6cx7cshM3dQAr+Xmjvu2CTllIFg01b0j3Ec0epbbXbb5QsuWleaEbsqatktRMiISC/6ix2ijH/n5vYq9GsDS9VhpsXLHdBVIiveorAXr92BR0wrHF2p7sSy7sptcmNLXe4SlJVHi4AHw7qbixoZKo4mPQepsxaIbeBNG74X0Wg4MGKDBUfQ2kX8JpU4jq/ZVDBGAY6CfH9s1Zns4BVQBokBeCUgh3Iik6NzeKAiPTNnD20JfXxaX1OfJIwP8yopUnqJQXdjqV0KFPRym0VNZyCXQEHFU=
  account: mediocregopher

Dockerfile.dehub-remote

@@ -36,9 +36,6 @@ COPY --from=0 /usr/bin/git-http-server /usr/bin/git-http-server
 # Create config files for container startup and nginx
 COPY cmd/dehub-remote/nginx.conf /etc/nginx/nginx.conf
 
-# Create pre-receive
-COPY cmd/dehub-remote/pre-receive /pre-receive
-
 # Create start.sh
 COPY cmd/dehub-remote/start.sh /start.sh
 RUN chmod +x /start.sh

ROADMAP.md

@@ -9,7 +9,6 @@ set, only a sequence of milestones and the requirements to hit them.
 Must be able to feel good about showing the project publicly, as well as be able
 to accept help from people asking to help.
 
-* Fast-forward perms on branches (so they can be deleted)
 * Figure out commit range syntax, use that everywhere.
 * Create a branch which is just a public "welcome thread", which can be part of
   the tutorials.

accessctl/access_control.go

@@ -28,6 +28,11 @@ var DefaultAccessControlsStr = `
       any_account: true
       count: 1
   
+  - action: deny
+    filters:
+    - type: commit_attributes
+      non_fast_forward: true
+
   - action: allow
     filters:
     - type: branch
@@ -67,6 +72,11 @@ type CommitRequest struct {
 	// FilesChanged is the set of file paths (relative to the repo root) which
 	// have been modified in some way.
 	FilesChanged []string
+
+	// NonFastForward should be set to true if the branch HEAD and this commit
+	// are not directly related (i.e. neither is a direct ancestor of the
+	// other).
+	NonFastForward bool
 }
 
 // Action describes what action an AccessControl should perform

accessctl/filter.go

@@ -1,9 +1,10 @@
 package accessctl
 
 import (
-	"dehub.dev/src/dehub.git/typeobj"
 	"errors"
 	"fmt"
+
+	"dehub.dev/src/dehub.git/typeobj"
 )
 
 // ErrFilterNoMatch is returned from a FilterInterface's Match method when the
@@ -28,11 +29,12 @@ type FilterInterface interface {
 // Filter represents an access control filter being defined in the Config. Only
 // one of its fields may be filled at a time.
 type Filter struct {
-	Signature    *FilterSignature    `type:"signature"`
+	Signature        *FilterSignature        `type:"signature"`
-	Branch       *FilterBranch       `type:"branch"`
+	Branch           *FilterBranch           `type:"branch"`
-	FilesChanged *FilterFilesChanged `type:"files_changed"`
+	FilesChanged     *FilterFilesChanged     `type:"files_changed"`
-	CommitType   *FilterCommitType   `type:"commit_type"`
+	CommitType       *FilterCommitType       `type:"commit_type"`
-	Not          *FilterNot          `type:"not"`
+	CommitAttributes *FilterCommitAttributes `type:"commit_attributes"`
+	Not              *FilterNot              `type:"not"`
 }
 
 // MarshalYAML implements the yaml.Marshaler interface.
@@ -100,3 +102,20 @@ func (f FilterCommitType) MatchCommit(req CommitRequest) error {
 		return errors.New(`one of the following fields must be set: "commit_type", "commit_types"`)
 	}
 }
+
+// FilterCommitAttributes filters by one more attributes a commit can have. If
+// more than one field is filled in then all relevant attributes must be present
+// on the commit for this filter to match.
+type FilterCommitAttributes struct {
+	NonFastForward bool `yaml:"non_fast_forward"`
+}
+
+var _ FilterInterface = FilterCommitAttributes{}
+
+// MatchCommit implements the method for FilterInterface.
+func (f FilterCommitAttributes) MatchCommit(req CommitRequest) error {
+	if f.NonFastForward && !req.NonFastForward {
+		return ErrFilterNoMatch{Err: errors.New("commit is a fast-forward")}
+	}
+	return nil
+}

accessctl/filter_test.go

@@ -86,3 +86,52 @@ func TestFilterCommitType(t *testing.T) {
 		},
 	})
 }
+
+func TestFilterCommitAttributes(t *testing.T) {
+	mkReq := func(nonFF bool) CommitRequest {
+		return CommitRequest{NonFastForward: nonFF}
+	}
+
+	runCommitMatchTests(t, []filterCommitMatchTest{
+		{
+			descr:  "ff with empty filter",
+			filter: FilterCommitAttributes{},
+			req:    mkReq(false),
+			match:  true,
+		},
+		{
+			descr:  "non-ff with empty filter",
+			filter: FilterCommitAttributes{},
+			req:    mkReq(true),
+			match:  true,
+		},
+		{
+			descr:  "ff with non-ff filter",
+			filter: FilterCommitAttributes{NonFastForward: true},
+			req:    mkReq(false),
+			match:  false,
+		},
+		{
+			descr:  "non-ff with non-ff filter",
+			filter: FilterCommitAttributes{NonFastForward: true},
+			req:    mkReq(true),
+			match:  true,
+		},
+		{
+			descr: "ff with inverted non-ff filter",
+			filter: FilterNot{Filter: Filter{
+				CommitAttributes: &FilterCommitAttributes{NonFastForward: true},
+			}},
+			req:   mkReq(false),
+			match: true,
+		},
+		{
+			descr: "non-ff with inverted non-ff filter",
+			filter: FilterNot{Filter: Filter{
+				CommitAttributes: &FilterCommitAttributes{NonFastForward: true},
+			}},
+			req:   mkReq(true),
+			match: false,
+		},
+	})
+}

cmd/dehub/cmd_hook.go

@@ -45,23 +45,16 @@ func cmdHook(ctx context.Context, cmd *dcmd.Cmd) {
 				return nil, fmt.Errorf("malformed pre-receive hook stdin line %q", line)
 			}
 
-			startRev := plumbing.Revision(lineParts[0])
+			endHash := plumbing.NewHash(lineParts[1])
-			endRev := plumbing.Revision(lineParts[1])
 			branchName := plumbing.ReferenceName(lineParts[2])
 
 			if !branchName.IsBranch() {
 				return nil, fmt.Errorf("reference %q is not a branch, can't push to it", branchName)
+			} else if endHash == plumbing.ZeroHash {
+				return nil, errors.New("deleting remote branches is not currently supported")
 			}
 
-			gitCommits, err := repo.GetGitRevisionRange(startRev, endRev)
+			return nil, repo.VerifyCanSetBranchHEADTo(branchName, endHash)
-			if err != nil {
-				return nil, fmt.Errorf("getting commits from %q to %q: %w",
-					startRev, endRev, err)
-
-			} else if err := repo.VerifyCommits(branchName, gitCommits); err != nil {
-				return nil, fmt.Errorf("verifying commits from %q to %q: %w",
-					startRev, endRev, err)
-			}
 		}
 
 		fmt.Println("All pushed commits have been verified, well done.")

commit.go

@@ -294,50 +294,102 @@ func (r *Repo) HasStagedChanges() (bool, error) {
 
 // VerifyCommits verifies that the given commits, which are presumably on the
 // given branch, are gucci.
-func (r *Repo) VerifyCommits(branch plumbing.ReferenceName, gitCommits []GitCommit) error {
+func (r *Repo) VerifyCommits(branchName plumbing.ReferenceName, gitCommits []GitCommit) error {
-
+	// this isn't strictly necessary for this method, but it helps discover bugs
-	// First determine the root of the main branch. All commits need to be an
+	// in other parts of the code.
-	// ancestor of it.
+	if len(gitCommits) == 0 {
-	var root plumbing.Hash
+		return errors.New("cannot call VerifyCommits with empty commit slice")
-	mainGitCommit, err := r.GetGitRevision(plumbing.Revision(MainRefName))
-	if err != nil {
-		return fmt.Errorf("retrieving commit at HEAD of main: %w", err)
 	}
 
-	rootCommit := mainGitCommit.GitCommit
+	// First determine the root of the main branch. All commits need to be an
-	for {
+	// ancestor of it. If the main branch has not been created yet then there
-		if rootCommit.NumParents() == 0 {
+	// might not be a root commit yet.
-			break
+	var rootCommit *object.Commit
-		} else if rootCommit.NumParents() > 1 {
+	mainGitCommit, err := r.GetGitRevision(plumbing.Revision(MainRefName))
-			return fmt.Errorf("commit %q in main branch has more than one parent", root)
+	if errors.Is(err, plumbing.ErrReferenceNotFound) {
-		} else if rootCommit, err = rootCommit.Parent(0); err != nil {
+
-			return fmt.Errorf("retrieving parent commit of %q: %w", root, err)
+		// main branch hasn't been created yet. The commits can only be verified
+		// if they are for the main branch and they include the root commit.
+		if branchName != MainRefName {
+			return fmt.Errorf("cannot verify commits in branch %q when no main branch exists", branchName)
 		}
+
+		for _, gitCommit := range gitCommits {
+			if gitCommit.GitCommit.NumParents() == 0 {
+				rootCommit = gitCommit.GitCommit
+				break
+			}
+		}
+		if rootCommit == nil {
+			return errors.New("root commit of main branch cannot be determined")
+		}
+
+	} else if err != nil {
+		return fmt.Errorf("retrieving commit at HEAD of %q: %w", MainRefName.Short(), err)
+
+	} else {
+		rootCommit = mainGitCommit.GitCommit
+		for {
+			if rootCommit.NumParents() == 0 {
+				break
+			} else if rootCommit.NumParents() > 1 {
+				return fmt.Errorf("commit %q in main branch has more than one parent", rootCommit.Hash)
+			} else if rootCommit, err = rootCommit.Parent(0); err != nil {
+				return fmt.Errorf("retrieving parent commit of %q: %w", rootCommit.Hash, err)
+			}
+		}
+	}
+
+	// We also need the HEAD of the given branch, if it exists.
+	branchGitCommit, err := r.GetGitRevision(plumbing.Revision(branchName))
+	if err != nil && !errors.Is(err, plumbing.ErrReferenceNotFound) {
+		return fmt.Errorf("retrieving commit at HEAD of %q: %w", branchName.Short(), err)
 	}
 
 	for i, gitCommit := range gitCommits {
 		// It's not a requirement that the given GitCommits are in ancestral
-		// order, but usually they are, so we can help verifyCommit not have to
+		// order, but usually they are; if the previous commit is the parent of
-		// calculate the parentTree if the previous commit is the parent of this
+		// this one we can skip a bunch of work.
-		// one, and not have to determine that each commit is an ancestor of
-		// main manually.
 		var parentTree *object.Tree
+		var isNonFF bool
 		if i > 0 && gitCommits[i-1].GitCommit.Hash == gitCommit.GitCommit.ParentHashes[0] {
 			parentTree = gitCommits[i-1].GitTree
 
 		} else if gitCommit.GitCommit.Hash == rootCommit.Hash {
-			// looking at the root commit itself, assume it's ok
+			// looking at the root commit, assume it's ok
 
-		} else if isAncestor, err := rootCommit.IsAncestor(gitCommit.GitCommit); err != nil {
+		} else {
-			return fmt.Errorf("determining if %q is an ancestor of %q (root of main): %w",
+			var err error
-				gitCommit.GitCommit.Hash, rootCommit.Hash, err)
+			isAncestor := func(older, younger *object.Commit) bool {
+				var isAncestor bool
+				if err != nil {
+					return false
+				} else if isAncestor, err = older.IsAncestor(younger); err != nil {
+					err = fmt.Errorf("determining if %q is an ancestor of %q: %w",
+						younger.Hash, older.Hash, err)
+					return false
 
-		} else if !isAncestor {
+				}
-			return fmt.Errorf("%q is not an ancestor of %q (root of main)",
+				return isAncestor
-				gitCommit.GitCommit.Hash, rootCommit.Hash)
+			}
+
+			ancestorOfRoot := isAncestor(rootCommit, gitCommit.GitCommit)
+			if branchGitCommit.GitCommit != nil {
+				// if the branch doesn't actually exist then this couldn't
+				// possibly be a nonFF
+				isNonFF = !isAncestor(branchGitCommit.GitCommit, gitCommit.GitCommit)
+			}
+
+			if err != nil {
+				return err
+			} else if !ancestorOfRoot {
+				return fmt.Errorf("commit %q must be direct descendant of root commit of %q (%q)",
+					gitCommit.GitCommit.Hash, MainRefName.Short(), rootCommit.Hash,
+				)
+			}
 		}
 
-		if err := r.verifyCommit(branch, gitCommit, parentTree); err != nil {
+		if err := r.verifyCommit(branchName, gitCommit, parentTree, isNonFF); err != nil {
 			return fmt.Errorf("verifying commit %q: %w",
 				gitCommit.GitCommit.Hash, err)
 		}
@@ -367,7 +419,12 @@ func (r *Repo) parentTree(commitObj *object.Commit) (*object.Tree, error) {
 }
 
 // if parentTree is nil then it will be inferred.
-func (r *Repo) verifyCommit(branch plumbing.ReferenceName, gitCommit GitCommit, parentTree *object.Tree) error {
+func (r *Repo) verifyCommit(
+	branchName plumbing.ReferenceName,
+	gitCommit GitCommit,
+	parentTree *object.Tree,
+	isNonFF bool,
+) error {
 	parentTree, err := r.parentTree(gitCommit.GitCommit)
 	if err != nil {
 		return fmt.Errorf("retrieving parent tree of commit: %w", err)
@@ -407,10 +464,11 @@ func (r *Repo) verifyCommit(branch plumbing.ReferenceName, gitCommit GitCommit,
 	}
 
 	err = accessctl.AssertCanCommit(cfg.AccessControls, accessctl.CommitRequest{
-		Type:         commitType,
+		Type:           commitType,
-		Branch:       branch.Short(),
+		Branch:         branchName.Short(),
-		Credentials:  gitCommit.Commit.Common.Credentials,
+		Credentials:    gitCommit.Commit.Common.Credentials,
-		FilesChanged: pathsChanged,
+		FilesChanged:   pathsChanged,
+		NonFastForward: isNonFF,
 	})
 	if err != nil {
 		return fmt.Errorf("asserting access controls: %w", err)
@@ -494,3 +552,59 @@ func (r *Repo) changeRangeInfo(commits []GitCommit) (changeRangeInfo, error) {
 	info.changeHash = genChangeHash(nil, info.msg, changedFiles)
 	return info, nil
 }
+
+// VerifyCanSetBranchHEADTo is used to verify that a branch's HEAD can be set to
+// the given hash. It verifies any new commits which are being added, and
+// handles verifying non-fast-forward commits as well.
+//
+// If the given hash matches the current HEAD of the branch then this performs
+// no further checks and returns nil.
+func (r *Repo) VerifyCanSetBranchHEADTo(branchName plumbing.ReferenceName, hash plumbing.Hash) error {
+	oldCommitRef, err := r.GitRepo.Reference(branchName, true)
+	if errors.Is(err, plumbing.ErrReferenceNotFound) {
+		// if the branch is being created then just pull all of its commits and
+		// verify them.
+		// TODO optimize this so that it tries to use the merge-base with main,
+		// so we're not re-verifying a ton of commits unecessarily
+		commits, err := r.GetGitCommitRange(plumbing.ZeroHash, hash)
+		if err != nil {
+			return fmt.Errorf("retrieving %q and all its ancestors: %w", hash, err)
+		}
+		return r.VerifyCommits(branchName, commits)
+
+	} else if err != nil {
+		return fmt.Errorf("resolving branch reference to a hash: %w", err)
+
+	} else if oldCommitRef.Hash() == hash {
+		// if the HEAD is already at the given hash then it must be fine.
+		return nil
+	}
+
+	oldCommitObj, err := r.GitRepo.CommitObject(oldCommitRef.Hash())
+	if err != nil {
+		return fmt.Errorf("retrieving commit object %q: %w", oldCommitRef.Hash(), err)
+	}
+
+	newCommitObj, err := r.GitRepo.CommitObject(hash)
+	if err != nil {
+		return fmt.Errorf("retrieving commit object %q: %w", hash, err)
+	}
+
+	mbCommits, err := oldCommitObj.MergeBase(newCommitObj)
+	if err != nil {
+		return fmt.Errorf("determining merge-base between %q and %q: %w",
+			oldCommitObj.Hash, newCommitObj.Hash, err)
+	} else if len(mbCommits) == 0 {
+		return fmt.Errorf("%q and %q have no ancestors in common",
+			oldCommitObj.Hash, newCommitObj.Hash)
+	} else if len(mbCommits) == 2 {
+		return fmt.Errorf("%q and %q have more than one ancestor in common",
+			oldCommitObj.Hash, newCommitObj.Hash)
+	}
+
+	commits, err := r.GetGitCommitRange(mbCommits[0].Hash, hash)
+	if err != nil {
+		return fmt.Errorf("retrieving commits %q to %q: %w", mbCommits[0].Hash, hash, err)
+	}
+	return r.VerifyCommits(branchName, commits)
+}

commit_change_test.go

@@ -78,7 +78,7 @@ func TestChangeCommitVerify(t *testing.T) {
 			for _, step := range test.steps {
 				h.stage(step.tree)
 
-				gitCommit := h.assertCommitChange(true, step.msg, rootSig)
+				gitCommit := h.assertCommitChange(verifyShouldSucceed, step.msg, rootSig)
 				if step.msgHead == "" {
 					step.msgHead = strings.TrimSpace(step.msg) + "\n\n"
 				}
@@ -105,7 +105,7 @@ func TestCombineCommitChanges(t *testing.T) {
 
 	// commit initial config, so the root user can modify it in the next commit
 	rootSig := h.stageNewAccount("root", false)
-	h.assertCommitChange(true, "initial commit", rootSig)
+	h.assertCommitChange(verifyShouldSucceed, "initial commit", rootSig)
 
 	// add a toot user and modify the access controls such that both accounts
 	// are required for the main branch
@@ -131,21 +131,21 @@ func TestCombineCommitChanges(t *testing.T) {
     any_account: true
     count: 1
 `)
-	tootCommit := h.assertCommitChange(true, "add toot", rootSig)
+	tootCommit := h.assertCommitChange(verifyShouldSucceed, "add toot", rootSig)
 
 	// make a single change commit in another branch using root. Then add a
 	// credential using toot, and combine them onto main.
 	otherBranch := plumbing.NewBranchReferenceName("other")
 	h.checkout(otherBranch)
 	h.stage(map[string]string{"foo": "bar"})
-	fooCommit := h.assertCommitChange(true, "add foo file", rootSig)
+	fooCommit := h.assertCommitChange(verifyShouldSucceed, "add foo file", rootSig)
 
 	// now adding a credential commit from toot should work
 	credCommitObj, err := h.repo.NewCommitCredential(fooCommit.Interface.StoredHash())
 	if err != nil {
 		t.Fatal(err)
 	}
-	credCommit := h.tryCommit(true, credCommitObj, tootSig)
+	credCommit := h.tryCommit(verifyShouldSucceed, credCommitObj, tootSig)
 
 	allCommits, err := h.repo.GetGitCommitRange(
 		tootCommit.GitCommit.Hash,

commit_credential_test.go

@@ -32,7 +32,7 @@ func TestCredentialCommitVerify(t *testing.T) {
     account_ids:
     - root
 `)
-	rootGitCommit := h.assertCommitChange(true, "initial commit", rootSig)
+	rootGitCommit := h.assertCommitChange(verifyShouldSucceed, "initial commit", rootSig)
 
 	// toot user wants to create a credential commit for the root commit, for
 	// whatever reason.
@@ -42,9 +42,9 @@ func TestCredentialCommitVerify(t *testing.T) {
 		t.Fatalf("creating credential commit for hash %x: %v", rootChangeHash, err)
 
 	}
-	h.tryCommit(false, credCommit, tootSig)
+	h.tryCommit(verifyShouldFail, credCommit, tootSig)
 
 	// toot tries again in their own branch, and should be allowed.
 	h.checkout(tootBranch)
-	h.tryCommit(true, credCommit, tootSig)
+	h.tryCommit(verifyShouldSucceed, credCommit, tootSig)
 }

commit_test.go

@@ -1,8 +1,13 @@
 package dehub
 
 import (
+	"errors"
+	"regexp"
 	"testing"
 
+	"dehub.dev/src/dehub.git/accessctl"
+	"dehub.dev/src/dehub.git/sigcred"
+	"gopkg.in/src-d/go-git.v4"
 	"gopkg.in/src-d/go-git.v4/plumbing"
 )
 
@@ -14,23 +19,23 @@ func TestConfigChange(t *testing.T) {
 
 	// commit the initial staged changes, which merely include the config and
 	// public key
-	gitCommit := h.assertCommitChange(true, "commit configuration", rootSig)
+	gitCommit := h.assertCommitChange(verifyShouldSucceed, "commit configuration", rootSig)
 	gitCommits = append(gitCommits, gitCommit)
 
 	// create a new account and add it to the configuration. That commit should
 	// not be verifiable, though
 	tootSig := h.stageNewAccount("toot", false)
 	h.stageCfg()
-	h.assertCommitChange(false, "add toot user", tootSig)
+	h.assertCommitChange(verifyShouldFail, "add toot user", tootSig)
 
 	// now add with the root user, this should work.
 	h.stageCfg()
-	gitCommit = h.assertCommitChange(true, "add toot user", rootSig)
+	gitCommit = h.assertCommitChange(verifyShouldSucceed, "add toot user", rootSig)
 	gitCommits = append(gitCommits, gitCommit)
 
 	// _now_ the toot user should be able to do things.
 	h.stage(map[string]string{"foo/bar": "what a cool file"})
-	gitCommit = h.assertCommitChange(true, "add a cool file", tootSig)
+	gitCommit = h.assertCommitChange(verifyShouldSucceed, "add a cool file", tootSig)
 	gitCommits = append(gitCommits, gitCommit)
 
 	if err := h.repo.VerifyCommits(MainRefName, gitCommits); err != nil {
@@ -47,14 +52,13 @@ func TestMainAncestryRequirement(t *testing.T) {
 
 		// stage and try to add to the "other" branch, it shouldn't work though
 		h.stageCfg()
-		h.assertCommitChange(false, "starting new branch at other", rootSig)
+		h.assertCommitChange(verifyShouldFail, "starting new branch at other", rootSig)
 	})
 
 	t.Run("new branch, single commit", func(t *testing.T) {
 		h := newHarness(t)
 		rootSig := h.stageNewAccount("root", false)
-		h.stageCfg()
+		h.assertCommitChange(verifyShouldSucceed, "add cfg", rootSig)
-		h.assertCommitChange(true, "add cfg", rootSig)
 
 		// set HEAD to this other branch which doesn't really exist
 		ref := plumbing.NewSymbolicReference(plumbing.HEAD, otherBranch)
@@ -63,7 +67,7 @@ func TestMainAncestryRequirement(t *testing.T) {
 		}
 
 		h.stageCfg()
-		h.assertCommitChange(false, "starting new branch at other", rootSig)
+		h.assertCommitChange(verifyShouldFail, "starting new branch at other", rootSig)
 	})
 }
 
@@ -77,5 +81,354 @@ func TestAnonymousCommits(t *testing.T) {
     - type: signature
       any: true
 `)
-	h.assertCommitChange(true, "this will work", anonSig)
+	h.assertCommitChange(verifyShouldSucceed, "this will work", anonSig)
+}
+
+func TestNonFastForwardCommits(t *testing.T) {
+	h := newHarness(t)
+	rootSig := h.stageNewAccount("root", false)
+	initCommit := h.assertCommitChange(verifyShouldSucceed, "init", rootSig)
+
+	// add another commit
+	h.stage(map[string]string{"foo": "foo"})
+	fooCommit := h.assertCommitChange(verifyShouldSucceed, "foo", rootSig)
+
+	commitOn := func(hash plumbing.Hash, msg string) GitCommit {
+		ref := plumbing.NewHashReference(plumbing.HEAD, hash)
+		if err := h.repo.GitRepo.Storer.SetReference(ref); err != nil {
+			h.t.Fatal(err)
+		} else if commitChange, err := h.repo.NewCommitChange("bar"); err != nil {
+			h.t.Fatal(err)
+		} else if commitChange, err = h.repo.AccreditCommit(commitChange, rootSig); err != nil {
+			h.t.Fatal(err)
+		} else if gitCommit, err := h.repo.Commit(commitChange); err != nil {
+			h.t.Fatal(err)
+		} else {
+			return gitCommit
+		}
+		panic("can't get here")
+	}
+
+	// checkout initCommit directly, make a new commit on top of it, and try to
+	// verify that (this is too fancy for the harness, must be done manually).
+	h.stage(map[string]string{"bar": "bar"})
+	barCommit := commitOn(initCommit.GitCommit.Hash, "bar")
+	err := h.repo.VerifyCommits(MainRefName, []GitCommit{barCommit})
+	if !errors.As(err, new(accessctl.ErrCommitRequestDenied)) {
+		h.t.Fatalf("expected ErrCommitRequestDenied, got: %v", err)
+	}
+
+	// check main back out (fooCommit should be checked out), and modify the
+	// config to allow nonFF commits, and add another bogus commit on top.
+	h.checkout(MainRefName)
+	h.stageAccessControls(`
+      - action: allow
+        filters:
+        - type: commit_attributes
+          non_fast_forward: true`)
+	h.stageCfg()
+	allowNonFFCommit := h.assertCommitChange(verifyShouldSucceed, "allow non-ff", rootSig)
+
+	h.stage(map[string]string{"foo": "foo foo"})
+	h.assertCommitChange(verifyShouldSucceed, "foo foo", rootSig)
+
+	// checking out allowNonFFCommit directly and performing a nonFF commit
+	// should work now.
+	h.stage(map[string]string{"baz": "baz"})
+	bazCommit := commitOn(allowNonFFCommit.GitCommit.Hash, "baz")
+	if err = h.repo.VerifyCommits(MainRefName, []GitCommit{bazCommit}); err != nil {
+		h.t.Fatal(err)
+	}
+
+	// verifying the full history should also work
+	gitCommits := []GitCommit{initCommit, fooCommit, allowNonFFCommit, bazCommit}
+	if err = h.repo.VerifyCommits(MainRefName, gitCommits); err != nil {
+		h.t.Fatal(err)
+	}
+}
+
+func TestCanSetBranchHEADTo(t *testing.T) {
+	type toTest struct {
+		// branchName and hash are the arguments passed into
+		// VerifyCanSetBranchHEADTo.
+		branchName plumbing.ReferenceName
+		hash       plumbing.Hash
+
+		// if set then the branch will have its HEAD reset to this hash prior to
+		// calling VerifyCanSetBranchHEADTo.
+		resetTo plumbing.Hash
+	}
+
+	type test struct {
+		descr string
+		init  func(h *harness, rootSig sigcred.SignifierInterface) toTest
+
+		// If true then the verify call is expected to fail. The string is a
+		// regex which should match the unwrapped error returned.
+		expErr string
+	}
+
+	tests := []test{
+		{
+			descr: "creation of main",
+			init: func(h *harness, rootSig sigcred.SignifierInterface) toTest {
+				// checkout other and build on top of that, so that when
+				// VerifyCanSetBranchHEADTo is called main won't exist.
+				other := plumbing.NewBranchReferenceName("other")
+				h.checkout(other)
+
+				initCommit := h.assertCommitChange(verifySkip, "init", rootSig)
+				return toTest{
+					branchName: MainRefName,
+					hash:       initCommit.GitCommit.Hash,
+				}
+			},
+		},
+		{
+			descr: "main ff",
+			init: func(h *harness, rootSig sigcred.SignifierInterface) toTest {
+				initCommit := h.assertCommitChange(verifySkip, "init", rootSig)
+				h.stage(map[string]string{"foo": "foo"})
+				nextCommit := h.assertCommitChange(verifySkip, "next", rootSig)
+				return toTest{
+					branchName: MainRefName,
+					hash:       nextCommit.GitCommit.Hash,
+					resetTo:    initCommit.GitCommit.Hash,
+				}
+			},
+		},
+		{
+			descr: "new branch, no main",
+			init: func(h *harness, rootSig sigcred.SignifierInterface) toTest {
+				// checkout other and build on top of that, so that when
+				// VerifyCanSetBranchHEADTo is called main won't exist.
+				other := plumbing.NewBranchReferenceName("other")
+				h.checkout(other)
+
+				initCommit := h.assertCommitChange(verifySkip, "init", rootSig)
+				return toTest{
+					branchName: plumbing.NewBranchReferenceName("other2"),
+					hash:       initCommit.GitCommit.Hash,
+				}
+			},
+			expErr: `^cannot verify commits in branch "refs/heads/other2" when no main branch exists$`,
+		},
+		{
+			// this case isn't generally possible, unless someone manually
+			// creates a branch in an empty repo on the remote
+			descr: "existing branch, no main",
+			init: func(h *harness, rootSig sigcred.SignifierInterface) toTest {
+				// checkout other and build on top of that, so that when
+				// VerifyCanSetBranchHEADTo is called main won't exist.
+				other := plumbing.NewBranchReferenceName("other")
+				h.checkout(other)
+
+				initCommit := h.assertCommitChange(verifySkip, "init", rootSig)
+				h.stage(map[string]string{"foo": "foo"})
+				fooCommit := h.assertCommitChange(verifySkip, "foo", rootSig)
+
+				return toTest{
+					branchName: other,
+					hash:       fooCommit.GitCommit.Hash,
+					resetTo:    initCommit.GitCommit.Hash,
+				}
+			},
+			expErr: `^cannot verify commits in branch "refs/heads/other" when no main branch exists$`,
+		},
+		{
+			descr: "new branch, not ancestor of main",
+			init: func(h *harness, rootSig sigcred.SignifierInterface) toTest {
+				h.assertCommitChange(verifySkip, "init", rootSig)
+
+				// create new branch with no HEAD, and commit on that.
+				other := plumbing.NewBranchReferenceName("other")
+				ref := plumbing.NewSymbolicReference(plumbing.HEAD, other)
+				if err := h.repo.GitRepo.Storer.SetReference(ref); err != nil {
+					t.Fatal(err)
+				}
+
+				h.stageCfg()
+				h.stage(map[string]string{"foo": "foo"})
+				badInitCommit := h.assertCommitChange(verifySkip, "a different init", rootSig)
+				return toTest{
+					branchName: plumbing.NewBranchReferenceName("other2"),
+					hash:       badInitCommit.GitCommit.Hash,
+				}
+			},
+			expErr: `^commit "[0-9a-f]+" must be direct descendant of root commit of "main" \("[0-9a-f]+"\)$`,
+		},
+		{
+			// this case isn't generally possible, unless someone manually
+			// creates a branch in an empty repo on the remote
+			descr: "existing branch, not ancestor of main",
+			init: func(h *harness, rootSig sigcred.SignifierInterface) toTest {
+				h.assertCommitChange(verifySkip, "init", rootSig)
+
+				// create new branch with no HEAD, and commit on that.
+				other := plumbing.NewBranchReferenceName("other")
+				ref := plumbing.NewSymbolicReference(plumbing.HEAD, other)
+				if err := h.repo.GitRepo.Storer.SetReference(ref); err != nil {
+					t.Fatal(err)
+				}
+
+				h.stageCfg()
+				h.stage(map[string]string{"foo": "foo"})
+				badInitCommit := h.assertCommitChange(verifySkip, "a different init", rootSig)
+
+				h.stage(map[string]string{"bar": "bar"})
+				barCommit := h.assertCommitChange(verifySkip, "bar", rootSig)
+
+				return toTest{
+					branchName: other,
+					hash:       barCommit.GitCommit.Hash,
+					resetTo:    badInitCommit.GitCommit.Hash,
+				}
+			},
+			expErr: `^commit "[0-9a-f]+" must be direct descendant of root commit of "main" \("[0-9a-f]+"\)$`,
+		},
+		{
+			descr: "new branch off of main",
+			init: func(h *harness, rootSig sigcred.SignifierInterface) toTest {
+				initCommit := 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)
+
+				return toTest{
+					branchName: other,
+					hash:       fooCommit.GitCommit.Hash,
+					resetTo:    initCommit.GitCommit.Hash,
+				}
+			},
+		},
+		{
+			descr: "new branch off of older main commit",
+			init: func(h *harness, rootSig sigcred.SignifierInterface) toTest {
+				initCommit := h.assertCommitChange(verifySkip, "init", rootSig)
+
+				h.stage(map[string]string{"foo": "foo"})
+				h.assertCommitChange(verifySkip, "foo", rootSig)
+
+				other := plumbing.NewBranchReferenceName("other")
+				h.checkout(other)
+				h.reset(initCommit.GitCommit.Hash, git.HardReset)
+				h.stage(map[string]string{"bar": "bar"})
+				barCommit := h.assertCommitChange(verifySkip, "bar", rootSig)
+
+				return toTest{
+					branchName: other,
+					hash:       barCommit.GitCommit.Hash,
+					resetTo:    initCommit.GitCommit.Hash,
+				}
+			},
+		},
+		{
+			descr: "branch ff",
+			init: func(h *harness, rootSig sigcred.SignifierInterface) toTest {
+				h.assertCommitChange(verifySkip, "init", rootSig)
+
+				other := plumbing.NewBranchReferenceName("other")
+				h.checkout(other)
+
+				var commits []GitCommit
+				for _, str := range []string{"foo", "bar", "baz", "biz", "buz"} {
+					h.stage(map[string]string{str: str})
+					commit := h.assertCommitChange(verifySkip, str, rootSig)
+					commits = append(commits, commit)
+				}
+
+				return toTest{
+					branchName: other,
+					hash:       commits[len(commits)-1].GitCommit.Hash,
+					resetTo:    commits[0].GitCommit.Hash,
+				}
+			},
+		},
+		{
+			descr: "main nonff",
+			init: func(h *harness, rootSig sigcred.SignifierInterface) toTest {
+				initCommit := h.assertCommitChange(verifySkip, "init", rootSig)
+				h.stage(map[string]string{"foo": "foo"})
+				h.assertCommitChange(verifySkip, "foo", rootSig)
+
+				// start another branch back at init and make a new commit on it
+				other := plumbing.NewBranchReferenceName("other")
+				h.checkout(other)
+				h.reset(initCommit.GitCommit.Hash, git.HardReset)
+				h.stage(map[string]string{"bar": "bar"})
+				barCommit := h.assertCommitChange(verifySkip, "bar", rootSig)
+
+				return toTest{
+					branchName: MainRefName,
+					hash:       barCommit.GitCommit.Hash,
+				}
+			},
+			expErr: `^commit matched and denied by this access control:`,
+		},
+		{
+			descr: "branch nonff",
+			init: func(h *harness, rootSig sigcred.SignifierInterface) 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)
+
+				other2 := plumbing.NewBranchReferenceName("other2")
+				h.checkout(other2)
+				h.reset(fooCommit.GitCommit.Hash, git.HardReset)
+				h.stage(map[string]string{"baz": "baz"})
+				bazCommit := h.assertCommitChange(verifySkip, "baz", rootSig)
+
+				return toTest{
+					branchName: other,
+					hash:       bazCommit.GitCommit.Hash,
+				}
+			},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.descr, func(t *testing.T) {
+			h := newHarness(t)
+			rootSig := h.stageNewAccount("root", false)
+			toTest := test.init(h, rootSig)
+
+			if toTest.resetTo != plumbing.ZeroHash {
+				ref := plumbing.NewHashReference(toTest.branchName, toTest.resetTo)
+				if err := h.repo.GitRepo.Storer.SetReference(ref); err != nil {
+					t.Fatal(err)
+				}
+			}
+
+			err := h.repo.VerifyCanSetBranchHEADTo(toTest.branchName, toTest.hash)
+			if test.expErr == "" {
+				if err != nil {
+					t.Fatalf("unexpected error: %v", err)
+				}
+				return
+			} else if err == nil {
+				t.Fatal("expected verification to fail")
+			}
+
+			ogErr := err
+			for {
+				if unwrappedErr := errors.Unwrap(err); unwrappedErr != nil {
+					err = unwrappedErr
+				} else {
+					break
+				}
+			}
+
+			errRegex := regexp.MustCompile(test.expErr)
+			if !errRegex.MatchString(err.Error()) {
+				t.Fatalf("\nexpected error of form %q\nbut got: %v", test.expErr, ogErr)
+			}
+		})
+	}
 }

repo.go

@@ -396,6 +396,10 @@ func (r *Repo) GetGitCommitRange(start, end plumbing.Hash) ([]GitCommit, error)
 	var commits []GitCommit
 	var found bool
 	for {
+		if found = start != plumbing.ZeroHash && curr.GitCommit.Hash == start; found {
+			break
+		}
+
 		commits = append(commits, curr)
 		numParents := curr.GitCommit.NumParents()
 		if numParents == 0 {
@@ -409,9 +413,6 @@ func (r *Repo) GetGitCommitRange(start, end plumbing.Hash) ([]GitCommit, error)
 		parent, err := r.GetGitCommit(parentHash)
 		if err != nil {
 			return nil, fmt.Errorf("retrieving commit %q: %w", parentHash, err)
-		} else if start != plumbing.ZeroHash && parentHash == start {
-			found = true
-			break
 		}
 		curr = parent
 	}

repo_test.go

@@ -97,7 +97,6 @@ func (h *harness) stageAccessControls(aclYAML string) {
 }
 
 func (h *harness) checkout(branch plumbing.ReferenceName) {
-
 	w, err := h.repo.GitRepo.Worktree()
 	if err != nil {
 		h.t.Fatal(err)
@@ -105,8 +104,8 @@ func (h *harness) checkout(branch plumbing.ReferenceName) {
 
 	head, err := h.repo.GetGitHead()
 	if errors.Is(err, ErrHeadIsZero) {
-		// if HEAD is resolvable to any hash than the Checkout method doesn't
+		// if HEAD is not resolvable to any hash than the Checkout method
-		// work, just set HEAD manually.
+		// doesn't work, just set HEAD manually.
 		ref := plumbing.NewSymbolicReference(plumbing.HEAD, branch)
 		if err := h.repo.GitRepo.Storer.SetReference(ref); err != nil {
 			h.t.Fatal(err)
@@ -151,8 +150,16 @@ func (h *harness) reset(to plumbing.Hash, mode git.ResetMode) {
 	}
 }
 
+type verifyExpectation int
+
+const (
+	verifyShouldSucceed verifyExpectation = 1
+	verifyShouldFail    verifyExpectation = 0
+	verifySkip          verifyExpectation = -1
+)
+
 func (h *harness) tryCommit(
-	shouldSucceed bool,
+	verifyExp verifyExpectation,
 	commit Commit,
 	accountSig sigcred.SignifierInterface,
 ) GitCommit {
@@ -166,6 +173,8 @@ func (h *harness) tryCommit(
 	gitCommit, err := h.repo.Commit(commit)
 	if err != nil {
 		h.t.Fatalf("failed to commit ChangeCommit: %v", err)
+	} else if verifyExp == verifySkip {
+		return gitCommit
 	}
 
 	branch, err := h.repo.ReferenceToBranchName(plumbing.HEAD)
@@ -173,6 +182,8 @@ func (h *harness) tryCommit(
 		h.t.Fatalf("determining checked out branch: %v", err)
 	}
 
+	shouldSucceed := verifyExp > 0
+
 	err = h.repo.VerifyCommits(branch, []GitCommit{gitCommit})
 	if shouldSucceed && err != nil {
 		h.t.Fatalf("verifying commit %q: %v", gitCommit.GitCommit.Hash, err)
@@ -192,7 +203,7 @@ func (h *harness) tryCommit(
 }
 
 func (h *harness) assertCommitChange(
-	shouldSucceed bool,
+	verifyExp verifyExpectation,
 	msg string,
 	sig sigcred.SignifierInterface,
 ) GitCommit {
@@ -200,7 +211,7 @@ func (h *harness) assertCommitChange(
 	if err != nil {
 		h.t.Fatalf("creating ChangeCommit: %v", err)
 	}
-	return h.tryCommit(shouldSucceed, commit, sig)
+	return h.tryCommit(verifyExp, commit, sig)
 }
 
 func TestHasStagedChanges(t *testing.T) {
@@ -220,12 +231,12 @@ func TestHasStagedChanges(t *testing.T) {
 
 	h.stage(map[string]string{"foo": "bar"})
 	assertHasStaged(true)
-	h.assertCommitChange(true, "first commit", rootSig)
+	h.assertCommitChange(verifyShouldSucceed, "first commit", rootSig)
 	assertHasStaged(false)
 
 	h.stage(map[string]string{"foo": ""}) // delete foo
 	assertHasStaged(true)
-	h.assertCommitChange(true, "second commit", rootSig)
+	h.assertCommitChange(verifyShouldSucceed, "second commit", rootSig)
 	assertHasStaged(false)
 }
 
@@ -263,7 +274,7 @@ func TestShortHashResolving(t *testing.T) {
 	// but that's hard...
 	h := newHarness(t)
 	rootSig := h.stageNewAccount("root", false)
-	hash := h.assertCommitChange(true, "first commit", rootSig).GitCommit.Hash
+	hash := h.assertCommitChange(verifyShouldSucceed, "first commit", rootSig).GitCommit.Hash
 	hashStr := hash.String()
 	t.Log(hashStr)