Use full output of git show-ref --tags to get tags for PushUpdateAddTag (#19235)

Strangely #19038 appears to relate to an issue whereby a tag appears to
be listed in `git show-ref --tags` but then does not appear when `git
show-ref --tags -- short_name` is called.

As a solution though I propose to stop the second call as it is
unnecessary and only likely to cause problems.

I've also noticed that the tags calls are wildly inefficient and aren't using the common cat-files - so these have been added.

I've also noticed that the git commit-graph is not being written on mirroring - so I've also added writing this to the migration which should improve mirror rendering somewhat. 

Fix #19038

Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: 6543 <6543@obermui.de>

modules/git/repo_branch_gogit.go

@@ -13,6 +13,7 @@ import (
 	"strings"
 
 	"github.com/go-git/go-git/v5/plumbing"
+	"github.com/go-git/go-git/v5/plumbing/storer"
 )
 
 // IsObjectExist returns true if given reference exists in the repository.
@@ -82,7 +83,8 @@ func (repo *Repository) GetBranchNames(skip, limit int) ([]string, int, error) {
 }
 
 // WalkReferences walks all the references from the repository
-func WalkReferences(ctx context.Context, repoPath string, walkfn func(string) error) (int, error) {
+// refType should be empty, ObjectTag or ObjectBranch. All other values are equivalent to empty.
+func WalkReferences(ctx context.Context, repoPath string, walkfn func(sha1, refname string) error) (int, error) {
 	repo := RepositoryFromContext(ctx, repoPath)
 	if repo == nil {
 		var err error
@@ -101,9 +103,45 @@ func WalkReferences(ctx context.Context, repoPath string, walkfn func(string) er
 	defer iter.Close()
 
 	err = iter.ForEach(func(ref *plumbing.Reference) error {
-		err := walkfn(string(ref.Name()))
+		err := walkfn(ref.Hash().String(), string(ref.Name()))
 		i++
 		return err
 	})
 	return i, err
 }
+
+// WalkReferences walks all the references from the repository
+func (repo *Repository) WalkReferences(arg ObjectType, skip, limit int, walkfn func(sha1, refname string) error) (int, error) {
+	i := 0
+	var iter storer.ReferenceIter
+	var err error
+	switch arg {
+	case ObjectTag:
+		iter, err = repo.gogitRepo.Tags()
+	case ObjectBranch:
+		iter, err = repo.gogitRepo.Branches()
+	default:
+		iter, err = repo.gogitRepo.References()
+	}
+	if err != nil {
+		return i, err
+	}
+	defer iter.Close()
+
+	err = iter.ForEach(func(ref *plumbing.Reference) error {
+		if i < skip {
+			i++
+			return nil
+		}
+		err := walkfn(ref.Hash().String(), string(ref.Name()))
+		i++
+		if err != nil {
+			return err
+		}
+		if limit != 0 && i >= skip+limit {
+			return storer.ErrStop
+		}
+		return nil
+	})
+	return i, err
+}

modules/git/repo_branch_nogogit.go

@@ -68,13 +68,29 @@ func (repo *Repository) GetBranchNames(skip, limit int) ([]string, int, error) {
 }
 
 // WalkReferences walks all the references from the repository
-func WalkReferences(ctx context.Context, repoPath string, walkfn func(string) error) (int, error) {
+func WalkReferences(ctx context.Context, repoPath string, walkfn func(sha1, refname string) error) (int, error) {
 	return walkShowRef(ctx, repoPath, "", 0, 0, walkfn)
 }
 
+// WalkReferences walks all the references from the repository
+// refType should be empty, ObjectTag or ObjectBranch. All other values are equivalent to empty.
+func (repo *Repository) WalkReferences(refType ObjectType, skip, limit int, walkfn func(sha1, refname string) error) (int, error) {
+	var arg string
+	switch refType {
+	case ObjectTag:
+		arg = "--tags"
+	case ObjectBranch:
+		arg = "--heads"
+	default:
+		arg = ""
+	}
+
+	return walkShowRef(repo.Ctx, repo.Path, arg, skip, limit, walkfn)
+}
+
 // callShowRef return refs, if limit = 0 it will not limit
 func callShowRef(ctx context.Context, repoPath, prefix, arg string, skip, limit int) (branchNames []string, countAll int, err error) {
-	countAll, err = walkShowRef(ctx, repoPath, arg, skip, limit, func(branchName string) error {
+	countAll, err = walkShowRef(ctx, repoPath, arg, skip, limit, func(_, branchName string) error {
 		branchName = strings.TrimPrefix(branchName, prefix)
 		branchNames = append(branchNames, branchName)
 
@@ -83,7 +99,7 @@ func callShowRef(ctx context.Context, repoPath, prefix, arg string, skip, limit
 	return
 }
 
-func walkShowRef(ctx context.Context, repoPath, arg string, skip, limit int, walkfn func(string) error) (countAll int, err error) {
+func walkShowRef(ctx context.Context, repoPath, arg string, skip, limit int, walkfn func(sha1, refname string) error) (countAll int, err error) {
 	stdoutReader, stdoutWriter := io.Pipe()
 	defer func() {
 		_ = stdoutReader.Close()
@@ -130,11 +146,7 @@ func walkShowRef(ctx context.Context, repoPath, arg string, skip, limit int, wal
 	for limit == 0 || i < skip+limit {
 		// The output of show-ref is simply a list:
 		// <sha> SP <ref> LF
-		_, err := bufReader.ReadSlice(' ')
+		sha, err := bufReader.ReadString(' ')
-		for err == bufio.ErrBufferFull {
-			// This shouldn't happen but we'll tolerate it for the sake of peace
-			_, err = bufReader.ReadSlice(' ')
-		}
 		if err == io.EOF {
 			return i, nil
 		}
@@ -154,7 +166,12 @@ func walkShowRef(ctx context.Context, repoPath, arg string, skip, limit int, wal
 		if len(branchName) > 0 {
 			branchName = branchName[:len(branchName)-1]
 		}
-		err = walkfn(branchName)
+
+		if len(sha) > 0 {
+			sha = sha[:len(sha)-1]
+		}
+
+		err = walkfn(sha, branchName)
 		if err != nil {
 			return i, err
 		}

modules/git/repo_commitgraph.go

@@ -0,0 +1,21 @@
+// Copyright 2022 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package git
+
+import (
+	"context"
+	"fmt"
+)
+
+// WriteCommitGraph write commit graph to speed up repo access
+// this requires git v2.18 to be installed
+func WriteCommitGraph(ctx context.Context, repoPath string) error {
+	if CheckGitVersionAtLeast("2.18") == nil {
+		if _, err := NewCommand(ctx, "commit-graph", "write").RunInDir(repoPath); err != nil {
+			return fmt.Errorf("unable to write commit-graph for '%s' : %w", repoPath, err)
+		}
+	}
+	return nil
+}

modules/git/repo_tag.go

@@ -10,7 +10,6 @@ import (
 	"fmt"
 	"strings"
 
-	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/util"
 )
 
@@ -34,69 +33,6 @@ func (repo *Repository) CreateAnnotatedTag(name, message, revision string) error
 	return err
 }
 
-func (repo *Repository) getTag(tagID SHA1, name string) (*Tag, error) {
-	t, ok := repo.tagCache.Get(tagID.String())
-	if ok {
-		log.Debug("Hit cache: %s", tagID)
-		tagClone := *t.(*Tag)
-		tagClone.Name = name // This is necessary because lightweight tags may have same id
-		return &tagClone, nil
-	}
-
-	tp, err := repo.GetTagType(tagID)
-	if err != nil {
-		return nil, err
-	}
-
-	// Get the commit ID and tag ID (may be different for annotated tag) for the returned tag object
-	commitIDStr, err := repo.GetTagCommitID(name)
-	if err != nil {
-		// every tag should have a commit ID so return all errors
-		return nil, err
-	}
-	commitID, err := NewIDFromString(commitIDStr)
-	if err != nil {
-		return nil, err
-	}
-
-	// If type is "commit, the tag is a lightweight tag
-	if ObjectType(tp) == ObjectCommit {
-		commit, err := repo.GetCommit(commitIDStr)
-		if err != nil {
-			return nil, err
-		}
-		tag := &Tag{
-			Name:    name,
-			ID:      tagID,
-			Object:  commitID,
-			Type:    tp,
-			Tagger:  commit.Committer,
-			Message: commit.Message(),
-		}
-
-		repo.tagCache.Set(tagID.String(), tag)
-		return tag, nil
-	}
-
-	// The tag is an annotated tag with a message.
-	data, err := NewCommand(repo.Ctx, "cat-file", "-p", tagID.String()).RunInDirBytes(repo.Path)
-	if err != nil {
-		return nil, err
-	}
-
-	tag, err := parseTagData(data)
-	if err != nil {
-		return nil, err
-	}
-
-	tag.Name = name
-	tag.ID = tagID
-	tag.Type = tp
-
-	repo.tagCache.Set(tagID.String(), tag)
-	return tag, nil
-}
-
 // GetTagNameBySHA returns the name of a tag from its tag object SHA or commit SHA
 func (repo *Repository) GetTagNameBySHA(sha string) (string, error) {
 	if len(sha) < 5 {
@@ -159,6 +95,20 @@ func (repo *Repository) GetTag(name string) (*Tag, error) {
 	return tag, nil
 }
 
+// GetTagWithID returns a Git tag by given name and ID
+func (repo *Repository) GetTagWithID(idStr, name string) (*Tag, error) {
+	id, err := NewIDFromString(idStr)
+	if err != nil {
+		return nil, err
+	}
+
+	tag, err := repo.getTag(id, name)
+	if err != nil {
+		return nil, err
+	}
+	return tag, nil
+}
+
 // GetTagInfos returns all tag infos of the repository.
 func (repo *Repository) GetTagInfos(page, pageSize int) ([]*Tag, int, error) {
 	// TODO this a slow implementation, makes one git command per tag
@@ -192,19 +142,6 @@ func (repo *Repository) GetTagInfos(page, pageSize int) ([]*Tag, int, error) {
 	return tags, tagsTotal, nil
 }
 
-// GetTagType gets the type of the tag, either commit (simple) or tag (annotated)
-func (repo *Repository) GetTagType(id SHA1) (string, error) {
-	// Get tag type
-	stdout, err := NewCommand(repo.Ctx, "cat-file", "-t", id.String()).RunInDir(repo.Path)
-	if err != nil {
-		return "", err
-	}
-	if len(stdout) == 0 {
-		return "", ErrNotExist{ID: id.String()}
-	}
-	return strings.TrimSpace(stdout), nil
-}
-
 // GetAnnotatedTag returns a Git tag by its SHA, must be an annotated tag
 func (repo *Repository) GetAnnotatedTag(sha string) (*Tag, error) {
 	id, err := NewIDFromString(sha)

modules/git/repo_tag_gogit.go

@@ -11,6 +11,8 @@ package git
 import (
 	"strings"
 
+	"code.gitea.io/gitea/modules/log"
+
 	"github.com/go-git/go-git/v5/plumbing"
 )
 
@@ -53,3 +55,83 @@ func (repo *Repository) GetTags(skip, limit int) ([]string, error) {
 
 	return tagNames, nil
 }
+
+// GetTagType gets the type of the tag, either commit (simple) or tag (annotated)
+func (repo *Repository) GetTagType(id SHA1) (string, error) {
+	// Get tag type
+	obj, err := repo.gogitRepo.Object(plumbing.AnyObject, id)
+	if err != nil {
+		if err == plumbing.ErrReferenceNotFound {
+			return "", &ErrNotExist{ID: id.String()}
+		}
+		return "", err
+	}
+
+	return obj.Type().String(), nil
+}
+
+func (repo *Repository) getTag(tagID SHA1, name string) (*Tag, error) {
+	t, ok := repo.tagCache.Get(tagID.String())
+	if ok {
+		log.Debug("Hit cache: %s", tagID)
+		tagClone := *t.(*Tag)
+		tagClone.Name = name // This is necessary because lightweight tags may have same id
+		return &tagClone, nil
+	}
+
+	tp, err := repo.GetTagType(tagID)
+	if err != nil {
+		return nil, err
+	}
+
+	// Get the commit ID and tag ID (may be different for annotated tag) for the returned tag object
+	commitIDStr, err := repo.GetTagCommitID(name)
+	if err != nil {
+		// every tag should have a commit ID so return all errors
+		return nil, err
+	}
+	commitID, err := NewIDFromString(commitIDStr)
+	if err != nil {
+		return nil, err
+	}
+
+	// If type is "commit, the tag is a lightweight tag
+	if ObjectType(tp) == ObjectCommit {
+		commit, err := repo.GetCommit(commitIDStr)
+		if err != nil {
+			return nil, err
+		}
+		tag := &Tag{
+			Name:    name,
+			ID:      tagID,
+			Object:  commitID,
+			Type:    tp,
+			Tagger:  commit.Committer,
+			Message: commit.Message(),
+		}
+
+		repo.tagCache.Set(tagID.String(), tag)
+		return tag, nil
+	}
+
+	gogitTag, err := repo.gogitRepo.TagObject(tagID)
+	if err != nil {
+		if err == plumbing.ErrReferenceNotFound {
+			return nil, &ErrNotExist{ID: tagID.String()}
+		}
+
+		return nil, err
+	}
+
+	tag := &Tag{
+		Name:    name,
+		ID:      tagID,
+		Object:  gogitTag.Target,
+		Type:    tp,
+		Tagger:  &gogitTag.Tagger,
+		Message: gogitTag.Message,
+	}
+
+	repo.tagCache.Set(tagID.String(), tag)
+	return tag, nil
+}

modules/git/repo_tag_nogogit.go

@@ -8,6 +8,13 @@
 
 package git
 
+import (
+	"errors"
+	"io"
+
+	"code.gitea.io/gitea/modules/log"
+)
+
 // IsTagExist returns true if given tag exists in the repository.
 func (repo *Repository) IsTagExist(name string) bool {
 	if name == "" {
@@ -23,3 +30,104 @@ func (repo *Repository) GetTags(skip, limit int) (tags []string, err error) {
 	tags, _, err = callShowRef(repo.Ctx, repo.Path, TagPrefix, "--tags", skip, limit)
 	return
 }
+
+// GetTagType gets the type of the tag, either commit (simple) or tag (annotated)
+func (repo *Repository) GetTagType(id SHA1) (string, error) {
+	wr, rd, cancel := repo.CatFileBatchCheck(repo.Ctx)
+	defer cancel()
+	_, err := wr.Write([]byte(id.String() + "\n"))
+	if err != nil {
+		return "", err
+	}
+	_, typ, _, err := ReadBatchLine(rd)
+	if IsErrNotExist(err) {
+		return "", ErrNotExist{ID: id.String()}
+	}
+	return typ, nil
+}
+
+func (repo *Repository) getTag(tagID SHA1, name string) (*Tag, error) {
+	t, ok := repo.tagCache.Get(tagID.String())
+	if ok {
+		log.Debug("Hit cache: %s", tagID)
+		tagClone := *t.(*Tag)
+		tagClone.Name = name // This is necessary because lightweight tags may have same id
+		return &tagClone, nil
+	}
+
+	tp, err := repo.GetTagType(tagID)
+	if err != nil {
+		return nil, err
+	}
+
+	// Get the commit ID and tag ID (may be different for annotated tag) for the returned tag object
+	commitIDStr, err := repo.GetTagCommitID(name)
+	if err != nil {
+		// every tag should have a commit ID so return all errors
+		return nil, err
+	}
+	commitID, err := NewIDFromString(commitIDStr)
+	if err != nil {
+		return nil, err
+	}
+
+	// If type is "commit, the tag is a lightweight tag
+	if ObjectType(tp) == ObjectCommit {
+		commit, err := repo.GetCommit(commitIDStr)
+		if err != nil {
+			return nil, err
+		}
+		tag := &Tag{
+			Name:    name,
+			ID:      tagID,
+			Object:  commitID,
+			Type:    tp,
+			Tagger:  commit.Committer,
+			Message: commit.Message(),
+		}
+
+		repo.tagCache.Set(tagID.String(), tag)
+		return tag, nil
+	}
+
+	// The tag is an annotated tag with a message.
+	wr, rd, cancel := repo.CatFileBatch(repo.Ctx)
+	defer cancel()
+
+	if _, err := wr.Write([]byte(tagID.String() + "\n")); err != nil {
+		return nil, err
+	}
+	_, typ, size, err := ReadBatchLine(rd)
+	if err != nil {
+		if errors.Is(err, io.EOF) || IsErrNotExist(err) {
+			return nil, ErrNotExist{ID: tagID.String()}
+		}
+		return nil, err
+	}
+	if typ != "tag" {
+		return nil, ErrNotExist{ID: tagID.String()}
+	}
+
+	// then we need to parse the tag
+	// and load the commit
+	data, err := io.ReadAll(io.LimitReader(rd, size))
+	if err != nil {
+		return nil, err
+	}
+	_, err = rd.Discard(1)
+	if err != nil {
+		return nil, err
+	}
+
+	tag, err := parseTagData(data)
+	if err != nil {
+		return nil, err
+	}
+
+	tag.Name = name
+	tag.ID = tagID
+	tag.Type = tp
+
+	repo.tagCache.Set(tagID.String(), tag)
+	return tag, nil
+}

modules/repository/repo.go

@@ -81,6 +81,10 @@ func MigrateRepositoryGitData(ctx context.Context, u *user_model.User,
 		return repo, fmt.Errorf("Clone: %v", err)
 	}
 
+	if err := git.WriteCommitGraph(ctx, repoPath); err != nil {
+		return repo, err
+	}
+
 	if opts.Wiki {
 		wikiPath := repo_model.WikiPath(u.Name, opts.RepoName)
 		wikiRemotePath := WikiRemoteURL(ctx, opts.CloneAddr)
@@ -102,6 +106,9 @@ func MigrateRepositoryGitData(ctx context.Context, u *user_model.User,
 				}
 			}
 		}
+		if err := git.WriteCommitGraph(ctx, wikiPath); err != nil {
+			return repo, err
+		}
 	}
 
 	if repo.OwnerID == u.ID {
@@ -279,23 +286,25 @@ func SyncReleasesWithTags(repo *repo_model.Repository, gitRepo *git.Repository)
 			}
 		}
 	}
-	tags, err := gitRepo.GetTags(0, 0)
+
-	if err != nil {
+	_, err := gitRepo.WalkReferences(git.ObjectTag, 0, 0, func(sha1, refname string) error {
-		return fmt.Errorf("unable to GetTags in Repo[%d:%s/%s]: %w", repo.ID, repo.OwnerName, repo.Name, err)
+		tagName := strings.TrimPrefix(refname, git.TagPrefix)
+		if _, ok := existingRelTags[strings.ToLower(tagName)]; ok {
+			return nil
 		}
-	for _, tagName := range tags {
+
-		if _, ok := existingRelTags[strings.ToLower(tagName)]; !ok {
+		if err := PushUpdateAddTag(repo, gitRepo, tagName, sha1, refname); err != nil {
-			if err := PushUpdateAddTag(repo, gitRepo, tagName); err != nil {
 			return fmt.Errorf("unable to PushUpdateAddTag: %q to Repo[%d:%s/%s]: %w", tagName, repo.ID, repo.OwnerName, repo.Name, err)
 		}
-		}
+
-	}
 		return nil
+	})
+	return err
 }
 
 // PushUpdateAddTag must be called for any push actions to add tag
-func PushUpdateAddTag(repo *repo_model.Repository, gitRepo *git.Repository, tagName string) error {
+func PushUpdateAddTag(repo *repo_model.Repository, gitRepo *git.Repository, tagName, sha1, refname string) error {
-	tag, err := gitRepo.GetTag(tagName)
+	tag, err := gitRepo.GetTagWithID(sha1, tagName)
 	if err != nil {
 		return fmt.Errorf("unable to GetTag: %w", err)
 	}

services/mirror/mirror_pull.go

@@ -204,6 +204,7 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo
 	timeout := time.Duration(setting.Git.Timeout.Mirror) * time.Second
 
 	log.Trace("SyncMirrors [repo: %-v]: running git remote update...", m.Repo)
+
 	gitArgs := []string{"remote", "update"}
 	if m.EnablePrune {
 		gitArgs = append(gitArgs, "--prune")
@@ -276,6 +277,10 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo
 	}
 	output := stderrBuilder.String()
 
+	if err := git.WriteCommitGraph(ctx, repoPath); err != nil {
+		log.Error("SyncMirrors [repo: %-v]: %v", m.Repo, err)
+	}
+
 	gitRepo, err := git.OpenRepositoryCtx(ctx, repoPath)
 	if err != nil {
 		log.Error("SyncMirrors [repo: %-v]: failed to OpenRepository: %v", m.Repo, err)
@@ -368,6 +373,10 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo
 				}
 				return nil, false
 			}
+
+			if err := git.WriteCommitGraph(ctx, wikiPath); err != nil {
+				log.Error("SyncMirrors [repo: %-v]: %v", m.Repo, err)
+			}
 		}
 		log.Trace("SyncMirrors [repo: %-v Wiki]: git remote update complete", m.Repo)
 	}

services/repository/branch.go

@@ -55,7 +55,7 @@ func GetBranches(ctx context.Context, repo *repo_model.Repository, skip, limit i
 
 // checkBranchName validates branch name with existing repository branches
 func checkBranchName(ctx context.Context, repo *repo_model.Repository, name string) error {
-	_, err := git.WalkReferences(ctx, repo.RepoPath(), func(refName string) error {
+	_, err := git.WalkReferences(ctx, repo.RepoPath(), func(_, refName string) error {
 		branchRefName := strings.TrimPrefix(refName, git.BranchPrefix)
 		switch {
 		case branchRefName == name: