Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor "refs/*" string usage by using constants #17784

Merged
merged 2 commits into from
Dec 2, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion contrib/pr/checkout.go
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ func main() {
//Use git cli command for windows
runCmd("git", "fetch", remoteUpstream, fmt.Sprintf("pull/%s/head:%s", pr, branch))
} else {
ref := fmt.Sprintf("refs/pull/%s/head:%s", pr, branchRef)
ref := fmt.Sprintf(gitea_git.PullPrefix+"%s/head:%s", pr, branchRef)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why this isn't

Suggested change
ref := fmt.Sprintf(gitea_git.PullPrefix+"%s/head:%s", pr, branchRef)
ref := fmt.Sprintf("%s%s/head:%s", gitea_git.PullPrefix, pr, branchRef)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No specific reason other than I didn't thought of it. Your proposal is indeed more elegant.

err = repo.Fetch(&git.FetchOptions{
RemoteName: remoteUpstream,
RefSpecs: []config.RefSpec{
Expand Down
5 changes: 3 additions & 2 deletions models/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"code.gitea.io/gitea/models/unit"
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/base"
"code.gitea.io/gitea/modules/git"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/references"
api "code.gitea.io/gitea/modules/structs"
Expand Down Expand Up @@ -761,8 +762,8 @@ func (issue *Issue) ChangeRef(doer *user_model.User, oldRef string) (err error)
if err = issue.loadRepo(db.GetEngine(ctx)); err != nil {
return fmt.Errorf("loadRepo: %v", err)
}
oldRefFriendly := strings.TrimPrefix(oldRef, "refs/heads/")
newRefFriendly := strings.TrimPrefix(issue.Ref, "refs/heads/")
oldRefFriendly := strings.TrimPrefix(oldRef, git.BranchPrefix)
newRefFriendly := strings.TrimPrefix(issue.Ref, git.BranchPrefix)

opts := &CreateCommentOptions{
Type: CommentTypeChangeIssueRef,
Expand Down
3 changes: 2 additions & 1 deletion models/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"code.gitea.io/gitea/models/db"
"code.gitea.io/gitea/models/unit"
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/git"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/timeutil"
Expand Down Expand Up @@ -349,7 +350,7 @@ func (pr *PullRequest) GetDefaultSquashMessage() string {

// GetGitRefName returns git ref for hidden pull request branch
func (pr *PullRequest) GetGitRefName() string {
return fmt.Sprintf("refs/pull/%d/head", pr.Index)
return fmt.Sprintf(git.PullPrefix+"%d/head", pr.Index)
Copy link
Member

@delvh delvh Dec 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why this isn't

Suggested change
return fmt.Sprintf(git.PullPrefix+"%d/head", pr.Index)
return fmt.Sprintf("%s%d/head", git.PullPrefix, pr.Index)

}

// IsChecking returns true if this pull request is still checking conflict.
Expand Down
2 changes: 1 addition & 1 deletion modules/convert/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ func ToAPIPullRequest(pr *models.PullRequest, doer *user_model.User) *api.PullRe
},
Head: &api.PRBranchInfo{
Name: pr.HeadBranch,
Ref: fmt.Sprintf("refs/pull/%d/head", pr.Index),
Ref: fmt.Sprintf(git.PullPrefix+"%d/head", pr.Index),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue:

Suggested change
Ref: fmt.Sprintf(git.PullPrefix+"%d/head", pr.Index),
Ref: fmt.Sprintf("%s%d/head", git.PullPrefix, pr.Index),

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you can send another PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RepoID: -1,
},
}
Expand Down
2 changes: 1 addition & 1 deletion modules/git/commit.go
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ func CommitChangesWithArgs(repoPath string, args []string, opts CommitChangesOpt
func AllCommitsCount(repoPath string, hidePRRefs bool, files ...string) (int64, error) {
args := []string{"--all", "--count"}
if hidePRRefs {
args = append([]string{"--exclude=refs/pull/*"}, args...)
args = append([]string{"--exclude=" + PullPrefix + "*"}, args...)
}
cmd := NewCommand("rev-list")
cmd.AddArguments(args...)
Expand Down
33 changes: 21 additions & 12 deletions modules/git/ref.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,15 @@ package git

import "strings"

const (
// RemotePrefix is the base directory of the remotes information of git.
RemotePrefix = "refs/remotes/"
// PullPrefix is the base directory of the pull information of git.
PullPrefix = "refs/pull/"

pullLen = len(PullPrefix)
)

// Reference represents a Git ref.
type Reference struct {
Name string
Expand All @@ -24,17 +33,17 @@ func (ref *Reference) ShortName() string {
if ref == nil {
return ""
}
if strings.HasPrefix(ref.Name, "refs/heads/") {
return ref.Name[11:]
if strings.HasPrefix(ref.Name, BranchPrefix) {
return strings.TrimPrefix(ref.Name, BranchPrefix)
}
if strings.HasPrefix(ref.Name, "refs/tags/") {
return ref.Name[10:]
if strings.HasPrefix(ref.Name, TagPrefix) {
return strings.TrimPrefix(ref.Name, TagPrefix)
}
if strings.HasPrefix(ref.Name, "refs/remotes/") {
return ref.Name[13:]
if strings.HasPrefix(ref.Name, RemotePrefix) {
return strings.TrimPrefix(ref.Name, RemotePrefix)
}
if strings.HasPrefix(ref.Name, "refs/pull/") && strings.IndexByte(ref.Name[10:], '/') > -1 {
return ref.Name[10 : strings.IndexByte(ref.Name[10:], '/')+10]
if strings.HasPrefix(ref.Name, PullPrefix) && strings.IndexByte(ref.Name[pullLen:], '/') > -1 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bit unfair that only PullPrefix gets a xxxLen constant ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I felt this would be overhead for the others. But consistency is important too.

return ref.Name[pullLen : strings.IndexByte(ref.Name[pullLen:], '/')+pullLen]
}

return ref.Name
Expand All @@ -45,16 +54,16 @@ func (ref *Reference) RefGroup() string {
if ref == nil {
return ""
}
if strings.HasPrefix(ref.Name, "refs/heads/") {
if strings.HasPrefix(ref.Name, BranchPrefix) {
return "heads"
}
if strings.HasPrefix(ref.Name, "refs/tags/") {
if strings.HasPrefix(ref.Name, TagPrefix) {
return "tags"
}
if strings.HasPrefix(ref.Name, "refs/remotes/") {
if strings.HasPrefix(ref.Name, RemotePrefix) {
return "remotes"
}
if strings.HasPrefix(ref.Name, "refs/pull/") && strings.IndexByte(ref.Name[10:], '/') > -1 {
if strings.HasPrefix(ref.Name, PullPrefix) && strings.IndexByte(ref.Name[pullLen:], '/') > -1 {
return "pull"
}
return ""
Expand Down
2 changes: 1 addition & 1 deletion modules/git/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ func parseSize(objects string) *CountObject {

// GetLatestCommitTime returns time for latest commit in repository (across all branches)
func GetLatestCommitTime(repoPath string) (time.Time, error) {
cmd := NewCommand("for-each-ref", "--sort=-committerdate", "refs/heads/", "--count", "1", "--format=%(committerdate)")
cmd := NewCommand("for-each-ref", "--sort=-committerdate", BranchPrefix, "--count", "1", "--format=%(committerdate)")
stdout, err := cmd.RunInDir(repoPath)
if err != nil {
return time.Time{}, err
Expand Down
2 changes: 1 addition & 1 deletion modules/git/repo_compare.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ func (repo *Repository) GetMergeBase(tmpRemote string, base, head string) (strin
}

if tmpRemote != "origin" {
tmpBaseName := "refs/remotes/" + tmpRemote + "/tmp_" + base
tmpBaseName := RemotePrefix + tmpRemote + "/tmp_" + base
// Fetch commit into a temporary branch in order to be able to handle commits and tags
_, err := NewCommandContext(repo.Ctx, "fetch", tmpRemote, base+":"+tmpBaseName).RunInDir(repo.Path)
if err == nil {
Expand Down
2 changes: 1 addition & 1 deletion modules/git/repo_ref_nogogit.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ func (repo *Repository) GetRefsFiltered(pattern string) ([]*Reference, error) {
refName = refName[:len(refName)-1]

// refName cannot be HEAD but can be remotes or stash
if strings.HasPrefix(refName, "/refs/remotes/") || refName == "/refs/stash" {
if strings.HasPrefix(refName, RemotePrefix) || refName == "/refs/stash" {
continue
}

Expand Down
2 changes: 1 addition & 1 deletion modules/gitgraph/graph.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ func GetCommitGraph(r *git.Repository, page int, maxAllowedColors int, hidePRRef
args = append(args, "--graph", "--date-order", "--decorate=full")

if hidePRRefs {
args = append(args, "--exclude=refs/pull/*")
args = append(args, "--exclude="+git.PullPrefix+"*")
}

if len(branches) == 0 {
Expand Down
4 changes: 3 additions & 1 deletion modules/migration/pullrequest.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ package migration
import (
"fmt"
"time"

"code.gitea.io/gitea/modules/git"
)

// PullRequest defines a standard pull request information
Expand Down Expand Up @@ -43,7 +45,7 @@ func (p *PullRequest) IsForkPullRequest() bool {

// GetGitRefName returns pull request relative path to head
func (p PullRequest) GetGitRefName() string {
return fmt.Sprintf("refs/pull/%d/head", p.Number)
return fmt.Sprintf(git.PullPrefix+"%d/head", p.Number)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue:

Suggested change
return fmt.Sprintf(git.PullPrefix+"%d/head", p.Number)
return fmt.Sprintf("%s%d/head", git.PullPrefix, p.Number)

}

// PullRequestBranch represents a pull request branch
Expand Down
2 changes: 1 addition & 1 deletion routers/web/repo/commit.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ func Graph(ctx *context.Context) {
copy(realBranches, branches)
for i, branch := range realBranches {
if strings.HasPrefix(branch, "--") {
realBranches[i] = "refs/heads/" + branch
realBranches[i] = git.BranchPrefix + branch
}
}
ctx.Data["SelectedBranches"] = realBranches
Expand Down
4 changes: 2 additions & 2 deletions services/mirror/mirror_pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,7 @@ func SyncPullMirror(ctx context.Context, repoID int64) bool {

for _, result := range results {
// Discard GitHub pull requests, i.e. refs/pull/*
if strings.HasPrefix(result.refName, "refs/pull/") {
if strings.HasPrefix(result.refName, git.PullPrefix) {
continue
}

Expand Down Expand Up @@ -499,7 +499,7 @@ func checkAndUpdateEmptyRepository(m *models.Mirror, gitRepo *git.Repository, re
}
firstName := ""
for _, result := range results {
if strings.HasPrefix(result.refName, "refs/pull/") {
if strings.HasPrefix(result.refName, git.PullPrefix) {
continue
}
tp, name := git.SplitRefName(result.refName)
Expand Down
4 changes: 2 additions & 2 deletions services/pull/merge.go
Original file line number Diff line number Diff line change
Expand Up @@ -421,9 +421,9 @@ func rawMerge(pr *models.PullRequest, doer *user_model.User, mergeStyle models.M
var pushCmd *git.Command
if mergeStyle == models.MergeStyleRebaseUpdate {
// force push the rebase result to head brach
pushCmd = git.NewCommand("push", "-f", "head_repo", stagingBranch+":refs/heads/"+pr.HeadBranch)
pushCmd = git.NewCommand("push", "-f", "head_repo", stagingBranch+":"+git.BranchPrefix+pr.HeadBranch)
} else {
pushCmd = git.NewCommand("push", "origin", baseBranch+":refs/heads/"+pr.BaseBranch)
pushCmd = git.NewCommand("push", "origin", baseBranch+":"+git.BranchPrefix+pr.BaseBranch)
}

// Push back to upstream.
Expand Down
4 changes: 2 additions & 2 deletions services/pull/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -451,8 +451,8 @@ func pushToBaseRepoHelper(pr *models.PullRequest, prefixHeadBranch string) (err
log.Info("Can't push with %s%s", prefixHeadBranch, pr.HeadBranch)
return err
}
log.Info("Retrying to push with refs/heads/%s", pr.HeadBranch)
err = pushToBaseRepoHelper(pr, "refs/heads/")
log.Info("Retrying to push with "+git.BranchPrefix+"%s", pr.HeadBranch)
err = pushToBaseRepoHelper(pr, git.BranchPrefix)
return err
}
log.Error("Unable to push PR head for %s#%d (%-v:%s) due to Error: %v", pr.BaseRepo.FullName(), pr.Index, pr.BaseRepo, gitRefName, err)
Expand Down
4 changes: 2 additions & 2 deletions services/repository/branch.go
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,8 @@ func RenameBranch(repo *models.Repository, doer *user_model.User, gitRepo *git.R
return "", err
}

notification.NotifyDeleteRef(doer, repo, "branch", "refs/heads/"+from)
notification.NotifyCreateRef(doer, repo, "branch", "refs/heads/"+to)
notification.NotifyDeleteRef(doer, repo, "branch", git.BranchPrefix+from)
notification.NotifyCreateRef(doer, repo, "branch", git.BranchPrefix+to)

return "", nil
}
Expand Down
2 changes: 1 addition & 1 deletion services/repository/files/temp_repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ func (t *TemporaryUploadRepository) Push(doer *user_model.User, commitHash strin
env := models.PushingEnvironment(doer, t.repo)
if err := git.Push(t.gitRepo.Ctx, t.basePath, git.PushOptions{
Remote: t.repo.RepoPath(),
Branch: strings.TrimSpace(commitHash) + ":refs/heads/" + strings.TrimSpace(branch),
Branch: strings.TrimSpace(commitHash) + ":" + git.BranchPrefix + strings.TrimSpace(branch),
Env: env,
}); err != nil {
if git.IsErrPushOutOfDate(err) {
Expand Down