[PERFORMANCE] Use Update-Ref for branch create/delete and tag deletion (#930)

.local.env

@@ -1,4 +1,5 @@
 GITNESS_TRACE=true
-GITNESS_WEBHOOK_ALLOW_LOOPBACK=true
+GITNESS_GIT_TRACE=true
 GITNESS_PRINCIPAL_ADMIN_PASSWORD=changeit
+GITNESS_WEBHOOK_ALLOW_LOOPBACK=true
 GITNESS_METRIC_ENABLED=false

git/adapter.go

@@ -68,7 +68,7 @@ type Adapter interface {
 	GetCommitDivergences(ctx context.Context, repoPath string,
 		requests []types.CommitDivergenceRequest, max int32) ([]types.CommitDivergence, error)
 	GetRef(ctx context.Context, repoPath string, reference string) (string, error)
-	UpdateRef(ctx context.Context, repoPath, reference, newValue, oldValue string) error
+	UpdateRef(ctx context.Context, envVars map[string]string, repoPath, reference, newValue, oldValue string) error
 	CreateTemporaryRepoForPR(ctx context.Context, reposTempPath string, pr *types.PullRequest,
 		baseBranch, trackingBranch string) (types.TempRepository, error)
 	Merge(ctx context.Context, pr *types.PullRequest, mergeMethod enum.MergeMethod, baseBranch, trackingBranch string,

git/adapter/commit.go

@@ -357,7 +357,7 @@ func (a Adapter) GetCommitDivergences(
 	res := make([]types.CommitDivergence, len(requests))
 	for i, req := range requests {
 		res[i], err = a.getCommitDivergence(ctx, repoPath, req, max)
-		if errors.Is(err, &types.NotFoundError{}) {
+		if types.IsNotFoundError(err) {
 			res[i] = types.CommitDivergence{Ahead: -1, Behind: -1}
 			continue
 		}

git/adapter/ref.go

@@ -21,10 +21,12 @@ import (
 	"math"
 	"strings"
 
+	"github.com/harness/gitness/git/hook"
 	"github.com/harness/gitness/git/types"
 
 	gitea "code.gitea.io/gitea/modules/git"
 	gitearef "code.gitea.io/gitea/modules/git/foreachref"
+	"github.com/rs/zerolog/log"
 )
 
 func DefaultInstructor(
@@ -156,18 +158,18 @@ func walkGiteaReferenceParser(
 func (a Adapter) GetRef(
 	ctx context.Context,
 	repoPath string,
-	reference string,
+	ref string,
 ) (string, error) {
 	if repoPath == "" {
 		return "", ErrRepositoryPathEmpty
 	}
-	cmd := gitea.NewCommand(ctx, "show-ref", "--verify", "-s", "--", reference)
+	cmd := gitea.NewCommand(ctx, "show-ref", "--verify", "-s", "--", ref)
 	stdout, _, err := cmd.RunStdString(&gitea.RunOpts{
 		Dir: repoPath,
 	})
 	if err != nil {
 		if err.IsExitCode(128) && strings.Contains(err.Stderr(), "not a valid ref") {
-			return "", types.ErrNotFound("reference '%s' not found", reference)
+			return "", types.ErrNotFound("reference %q not found", ref)
 		}
 		return "", err
 	}
@@ -180,34 +182,147 @@ func (a Adapter) GetRef(
 // (e.g `refs/heads/main` instead of `main`).
 func (a Adapter) UpdateRef(
 	ctx context.Context,
+	envVars map[string]string,
 	repoPath string,
-	reference string,
+	ref string,
-	newValue string,
 	oldValue string,
+	newValue string,
 ) error {
 	if repoPath == "" {
 		return ErrRepositoryPathEmpty
 	}
-	args := make([]string, 0, 4)
+
-	args = append(args, "update-ref")
+	// don't break existing interface - user calls with empty value to delete the ref.
 	if newValue == "" {
-		// if newvalue is empty, delete ref
+		newValue = types.NilSHA
-		args = append(args, "-d", reference)
-	} else {
-		args = append(args, reference, newValue)
 	}
 
-	// if an old value was provided, verify it matches.
+	// if no old value was provided, use current value (as required for hooks)
-	if oldValue != "" {
+	// TODO: technically a delete could fail if someone updated the ref in the meanwhile.
-		args = append(args, oldValue)
+	//nolint:gocritic,nestif
+	if oldValue == "" {
+		val, err := a.GetRef(ctx, repoPath, ref)
+		if types.IsNotFoundError(err) {
+			// fail in case someone tries to delete a reference that doesn't exist.
+			if newValue == types.NilSHA {
+				return types.ErrNotFound("reference %q not found", ref)
+			}
+
+			oldValue = types.NilSHA
+		} else if err != nil {
+			return fmt.Errorf("failed to get current value of reference: %w", err)
+		} else {
+			oldValue = val
+		}
 	}
 
-	cmd := gitea.NewCommand(ctx, args...)
+	err := a.updateRefWithHooks(
-	_, _, err := cmd.RunStdString(&gitea.RunOpts{
+		ctx,
-		Dir: repoPath,
+		envVars,
-	})
+		repoPath,
+		ref,
+		oldValue,
+		newValue,
+	)
 	if err != nil {
-		return processGiteaErrorf(err, "update-ref failed")
+		return fmt.Errorf("failed to update reference with hooks: %w", err)
+	}
+
+	return nil
+}
+
+// updateRefWithHooks performs a git-ref update for the provided reference.
+// Requires both old and new value to be provided explcitly, or the call fails (ensures consistency across operation).
+// pre-receice will be called before the update, post-receive after.
+func (a Adapter) updateRefWithHooks(
+	ctx context.Context,
+	envVars map[string]string,
+	repoPath string,
+	ref string,
+	oldValue string,
+	newValue string,
+) error {
+	if repoPath == "" {
+		return ErrRepositoryPathEmpty
+	}
+
+	if oldValue == "" {
+		return fmt.Errorf("oldValue can't be empty")
+	}
+	if newValue == "" {
+		return fmt.Errorf("newValue can't be empty")
+	}
+	if oldValue == types.NilSHA && newValue == types.NilSHA {
+		return fmt.Errorf("provided values cannot be both empty")
+	}
+
+	githookClient, err := a.githookFactory.NewClient(ctx, envVars)
+	if err != nil {
+		return fmt.Errorf("failed to create githook client: %w", err)
+	}
+
+	// call pre-receive before updating the reference
+	out, err := githookClient.PreReceive(ctx, hook.PreReceiveInput{
+		RefUpdates: []hook.ReferenceUpdate{
+			{
+				Ref: ref,
+				Old: oldValue,
+				New: newValue,
+			},
+		},
+	})
+	if err != nil {
+		return fmt.Errorf("pre-receive call failed with: %w", err)
+	}
+	if out.Error != nil {
+		return fmt.Errorf("pre-receive call returned error: %q", *out.Error)
+	}
+
+	if a.traceGit {
+		log.Ctx(ctx).Trace().
+			Str("git", "pre-receive").
+			Msgf("pre-receive call succeeded with output:\n%s", strings.Join(out.Messages, "\n"))
+	}
+
+	args := make([]string, 0, 4)
+	args = append(args, "update-ref")
+	if newValue == types.NilSHA {
+		args = append(args, "-d", ref)
+	} else {
+		args = append(args, ref, newValue)
+	}
+
+	args = append(args, oldValue)
+
+	cmd := gitea.NewCommand(ctx, args...)
+	_, _, err = cmd.RunStdString(&gitea.RunOpts{
+		Dir: repoPath,
+	})
+	if err != nil {
+		return processGiteaErrorf(err, "update of ref %q from %q to %q failed", ref, oldValue, newValue)
+	}
+
+	// call post-receive after updating the reference
+	out, err = githookClient.PostReceive(ctx, hook.PostReceiveInput{
+		RefUpdates: []hook.ReferenceUpdate{
+			{
+				Ref: ref,
+				Old: oldValue,
+				New: newValue,
+			},
+		},
+	})
+	if err != nil {
+		return fmt.Errorf("post-receive call failed with: %w", err)
+	}
+	if out.Error != nil {
+		return fmt.Errorf("post-receive call returned error: %q", *out.Error)
+	}
+
+	if a.traceGit {
+		log.Ctx(ctx).Trace().
+			Str("git", "post-receive").
+			Msgf("post-receive call succeeded with output:\n%s", strings.Join(out.Messages, "\n"))
 	}
 
 	return nil

git/branch.go

@@ -24,7 +24,6 @@ import (
 	"github.com/harness/gitness/git/check"
 	"github.com/harness/gitness/git/types"
 
-	gitea "code.gitea.io/gitea/modules/git"
 	"github.com/rs/zerolog/log"
 )
 
@@ -93,80 +92,42 @@ func (s *Service) CreateBranch(ctx context.Context, params *CreateBranchParams)
 	if err := params.Validate(); err != nil {
 		return nil, err
 	}
-
-	repoPath := getFullPathForRepo(s.reposRoot, params.RepoUID)
-	env := CreateEnvironmentForPush(ctx, params.WriteParams)
-
 	if err := check.BranchName(params.BranchName); err != nil {
 		return nil, errors.InvalidArgument(err.Error())
 	}
 
-	repo, err := s.adapter.OpenRepository(ctx, repoPath)
+	repoPath := getFullPathForRepo(s.reposRoot, params.RepoUID)
+	targetCommit, err := s.adapter.GetCommit(ctx, repoPath, strings.TrimSpace(params.Target))
 	if err != nil {
-		return nil, fmt.Errorf("failed to open repo: %w", err)
+		return nil, fmt.Errorf("failed to get target commit: %w", err)
 	}
-
+	branchRef := adapter.GetReferenceFromBranchName(params.BranchName)
-	if ok, err := repo.IsEmpty(); ok {
+	err = s.adapter.UpdateRef(
-		if err != nil {
-			return nil, errors.Internal("failed to check if repository is empty: %v", err)
-		}
-		return nil, errors.InvalidArgument("branch cannot be created on empty repository")
-	}
-
-	sharedRepo, err := s.adapter.SharedRepository(s.tmpDir, params.RepoUID, repo.Path)
-	if err != nil {
-		return nil, errors.Internal("failed to create new shared repo", err)
-	}
-	defer sharedRepo.Close(ctx)
-
-	// clone repo (with HEAD branch - target might be anything)
-	err = sharedRepo.Clone(ctx, "")
-	if err != nil {
-		return nil, errors.Internal("failed to clone shared repo with branch '%s'", params.BranchName, err)
-	}
-
-	_, err = sharedRepo.GetBranchCommit(params.BranchName)
-	// return an error if branch alredy exists (push doesn't fail if it's a noop or fast forward push)
-	if err == nil {
-		return nil, errors.Conflict("branch '%s' already exists", params.BranchName)
-	}
-	if !gitea.IsErrNotExist(err) {
-		return nil, errors.Internal("branch creation of '%s' failed: %w", params.BranchName, err)
-	}
-
-	// get target commit (as target could be branch/tag/commit, and tag can't be pushed using source:destination syntax)
-	targetCommit, err := s.adapter.GetCommit(ctx, sharedRepo.Path(), strings.TrimSpace(params.Target))
-	if gitea.IsErrNotExist(err) {
-		return nil, errors.NotFound("target '%s' doesn't exist", params.Target)
-	}
-	if err != nil {
-		return nil, errors.Internal("failed to get commit id for target '%s'", params.Target, err)
-	}
-
-	// push to new branch (all changes should go through push flow for hooks and other safety meassures)
-	err = sharedRepo.PushCommitToBranch(ctx, targetCommit.SHA, params.BranchName, false, env...)
-	if err != nil {
-		return nil, err
-	}
-
-	// get branch
-	// TODO: get it from shared repo to avoid opening another gitea repo and having to strip here.
-	gitBranch, err := s.adapter.GetBranch(
 		ctx,
+		params.EnvVars,
 		repoPath,
-		strings.TrimPrefix(params.BranchName, gitReferenceNamePrefixBranch),
+		branchRef,
+		types.NilSHA, // we want to make sure we don't overwrite any parallel create
+		targetCommit.SHA,
 	)
+	if errors.IsConflict(err) {
+		return nil, errors.Conflict("branch %q already exists", params.BranchName, err)
+	}
 	if err != nil {
-		return nil, fmt.Errorf("failed to get git branch '%s': %w", params.BranchName, err)
+		return nil, fmt.Errorf("failed to update branch reference: %w", err)
 	}
 
-	branch, err := mapBranch(gitBranch)
+	commit, err := mapCommit(targetCommit)
 	if err != nil {
-		return nil, fmt.Errorf("failed to map rpc branch %v: %w", gitBranch.Name, err)
+		return nil, fmt.Errorf("failed to map git commit: %w", err)
 	}
 
 	return &CreateBranchOutput{
-		Branch: *branch,
+		Branch: Branch{
+			Name:   params.BranchName,
+			SHA:    commit.SHA,
+			Commit: commit,
+		},
 	}, nil
 }
 
@@ -199,37 +160,21 @@ func (s *Service) DeleteBranch(ctx context.Context, params *DeleteBranchParams)
 	}
 
 	repoPath := getFullPathForRepo(s.reposRoot, params.RepoUID)
-	env := CreateEnvironmentForPush(ctx, params.WriteParams)
+	branchRef := adapter.GetReferenceFromBranchName(params.BranchName)
 
-	repo, err := s.adapter.OpenRepository(ctx, repoPath)
+	err := s.adapter.UpdateRef(
-	if err != nil {
+		ctx,
-		return fmt.Errorf("failed to open repo: %w", err)
+		params.EnvVars,
+		repoPath,
+		branchRef,
+		"", // delete whatever is there
+		types.NilSHA,
+	)
+	if types.IsNotFoundError(err) {
+		return errors.NotFound("branch %q does not exist", params.BranchName, err)
 	}
-
-	sharedRepo, err := s.adapter.SharedRepository(s.tmpDir, params.RepoUID, repo.Path)
 	if err != nil {
-		return fmt.Errorf("failed to create new shared repo: %w", err)
+		return fmt.Errorf("failed to delete branch reference: %w", err)
-	}
-	defer sharedRepo.Close(ctx)
-
-	// clone repo (technically we don't care about which branch we clone)
-	err = sharedRepo.Clone(ctx, params.BranchName)
-	if err != nil {
-		return fmt.Errorf("failed to clone shared repo with branch '%s': %w", params.BranchName, err)
-	}
-
-	// get latest branch commit before we delete
-	_, err = sharedRepo.GetBranchCommit(params.BranchName)
-	if err != nil {
-		return fmt.Errorf("failed to get gitea commit for branch '%s': %w", params.BranchName, err)
-	}
-
-	// push to remote (all changes should go through push flow for hooks and other safety meassures)
-	// NOTE: setting sourceRef to empty will delete the remote branch when pushing:
-	// https://git-scm.com/docs/git-push#Documentation/git-push.txt-ltrefspecgt82308203
-	err = sharedRepo.PushDeleteBranch(ctx, params.BranchName, true, env...)
-	if err != nil {
-		return fmt.Errorf("failed to delete branch '%s' from remote repo: %w", params.BranchName, err)
 	}
 
 	return nil

git/ref.go

@@ -92,49 +92,16 @@ func (s *Service) UpdateRef(ctx context.Context, params UpdateRefParams) error {
 		return fmt.Errorf("UpdateRef: failed to fetch reference '%s': %w", params.Name, err)
 	}
 
-	repo, err := s.adapter.OpenRepository(ctx, repoPath)
+	err = s.adapter.UpdateRef(
+		ctx,
+		params.EnvVars,
+		repoPath,
+		reference,
+		params.OldValue,
+		params.NewValue,
+	)
 	if err != nil {
-		return fmt.Errorf("UpdateRef: failed to open repo: %w", err)
+		return fmt.Errorf("failed to update ref: %w", err)
-	}
-
-	if ok, _ := repo.IsEmpty(); ok {
-		return errors.InvalidArgument("branch cannot be created on empty repository")
-	}
-
-	sharedRepo, err := s.adapter.SharedRepository(s.tmpDir, params.RepoUID, repo.Path)
-	if err != nil {
-		return fmt.Errorf("UpdateRef: failed to create new shared repo: %w", err)
-	}
-	defer sharedRepo.Close(ctx)
-
-	// clone repo (with HEAD branch - target might be anything)
-	err = sharedRepo.Clone(ctx, "")
-	if err != nil {
-		return fmt.Errorf("UpdateRef: failed to clone shared repo: %w", err)
-	}
-
-	pushOpts := types.PushOptions{
-		Remote: sharedRepo.RemotePath(),
-		Env:    CreateEnvironmentForPush(ctx, params.WriteParams),
-	}
-
-	// handle deletion explicitly to avoid any unwanted side effects
-	if params.NewValue == "" {
-		pushOpts.Branch = ":" + reference
-	} else {
-		pushOpts.Branch = params.NewValue + ":" + reference
-	}
-
-	if params.OldValue == "" {
-		pushOpts.Force = true
-	} else {
-		pushOpts.ForceWithLease = reference + ":" + params.OldValue
-	}
-
-	// TODO: our shared repo has so much duplication, that should be changed IMHO.
-	err = s.adapter.Push(ctx, sharedRepo.Path(), pushOpts)
-	if err != nil {
-		return fmt.Errorf("UpdateRef: failed to push changes to original repo: %w", err)
 	}
 
 	return nil

git/tag.go

@@ -21,6 +21,7 @@ import (
 	"time"
 
 	"github.com/harness/gitness/errors"
+	"github.com/harness/gitness/git/adapter"
 	"github.com/harness/gitness/git/types"
 
 	"code.gitea.io/gitea/modules/git"
@@ -224,6 +225,7 @@ func (s *Service) ListCommitTags(
 		Tags: tags,
 	}, nil
 }
+
 func (s *Service) CreateCommitTag(ctx context.Context, params *CreateCommitTagParams) (*CreateCommitTagOutput, error) {
 	if err := params.Validate(); err != nil {
 		return nil, err
@@ -334,28 +336,21 @@ func (s *Service) DeleteTag(ctx context.Context, params *DeleteTagParams) error
 	}
 
 	repoPath := getFullPathForRepo(s.reposRoot, params.RepoUID)
+	tagRef := adapter.GetReferenceFromTagName(params.Name)
 
-	repo, err := s.adapter.OpenRepository(ctx, repoPath)
+	err := s.adapter.UpdateRef(
-	if err != nil {
+		ctx,
-		return fmt.Errorf("DeleteTag: failed to open repo: %w", err)
+		params.EnvVars,
+		repoPath,
+		tagRef,
+		"", // delete whatever is there
+		types.NilSHA,
+	)
+	if types.IsNotFoundError(err) {
+		return errors.NotFound("tag %q does not exist", params.Name, err)
 	}
-
-	sharedRepo, err := s.adapter.SharedRepository(s.tmpDir, params.RepoUID, repo.Path)
 	if err != nil {
-		return fmt.Errorf("DeleteTag: failed to create new shared repo: %w", err)
+		return fmt.Errorf("failed to delete tag reference: %w", err)
-	}
-
-	defer sharedRepo.Close(ctx)
-
-	// clone repo (with HEAD branch - tag target might be anything)
-	err = sharedRepo.Clone(ctx, "")
-	if err != nil {
-		return fmt.Errorf("DeleteTag: failed to clone shared repo with tag '%s': %w",
-			params.Name, err)
-	}
-	envs := CreateEnvironmentForPush(ctx, params.WriteParams)
-	if err = sharedRepo.PushDeleteTag(ctx, params.Name, true, envs...); err != nil {
-		return fmt.Errorf("DeleteTag: Failed to push the tag %s to remote: %w", params.Name, err)
 	}
 
 	return nil

git/types/errors.go

@@ -37,10 +37,20 @@ type NotFoundError struct {
 	Msg string
 }
 
+func IsNotFoundError(err error) bool {
+	return errors.Is(err, &NotFoundError{})
+}
+
 func (e *NotFoundError) Error() string {
 	return e.Msg
 }
 
+//nolint:errorlint // the purpose of this method is to check whether the target itself if of this type.
+func (e *NotFoundError) Is(target error) bool {
+	_, ok := target.(*NotFoundError)
+	return ok
+}
+
 func ErrNotFound(format string, args ...any) error {
 	return &NotFoundError{
 		Msg: fmt.Sprintf(format, args...),

git/types/types.go

@@ -22,6 +22,8 @@ import (
 	"github.com/harness/gitness/errors"
 )
 
+const NilSHA = "0000000000000000000000000000000000000000"
+
 type CloneRepoOptions struct {
 	Timeout       time.Duration
 	Mirror        bool