Handle more pathological branch and tag names (#11843)

* Handle more pathological branch and tag names

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Fix failing test

Signed-off-by: Andrew Thornton <art27@cantab.net>

Co-authored-by: techknowlogick <techknowlogick@gitea.io>

integrations/branches_test.go

@@ -32,14 +32,14 @@ func TestDeleteBranch(t *testing.T) {
 }
 
 func TestUndoDeleteBranch(t *testing.T) {
-	defer prepareTestEnv(t)()
+	onGiteaRun(t, func(t *testing.T, u *url.URL) {
-
+		deleteBranch(t)
-	deleteBranch(t)
+		htmlDoc, name := branchAction(t, ".undo-button")
-	htmlDoc, name := branchAction(t, ".undo-button")
+		assert.Contains(t,
-	assert.Contains(t,
+			htmlDoc.doc.Find(".ui.positive.message").Text(),
-		htmlDoc.doc.Find(".ui.positive.message").Text(),
+			i18n.Tr("en", "repo.branch.restore_success", name),
-		i18n.Tr("en", "repo.branch.restore_success", name),
+		)
-	)
+	})
 }
 
 func deleteBranch(t *testing.T) {

modules/git/repo_commit.go

@@ -46,7 +46,7 @@ func (repo *Repository) GetBranchCommitID(name string) (string, error) {
 
 // GetTagCommitID returns last commit ID string of given tag.
 func (repo *Repository) GetTagCommitID(name string) (string, error) {
-	stdout, err := NewCommand("rev-list", "-n", "1", name).RunInDir(repo.Path)
+	stdout, err := NewCommand("rev-list", "-n", "1", TagPrefix+name).RunInDir(repo.Path)
 	if err != nil {
 		if strings.Contains(err.Error(), "unknown revision or path") {
 			return "", ErrNotExist{name, ""}

modules/repository/branch.go

@@ -9,7 +9,6 @@ import (
 
 	"code.gitea.io/gitea/models"
 	"code.gitea.io/gitea/modules/git"
-	"code.gitea.io/gitea/modules/log"
 )
 
 // GetBranch returns a branch by its name
@@ -76,39 +75,9 @@ func CreateNewBranch(doer *models.User, repo *models.Repository, oldBranchName,
 		}
 	}
 
-	basePath, err := models.CreateTemporaryPath("branch-maker")
+	if err := git.Push(repo.RepoPath(), git.PushOptions{
-	if err != nil {
+		Remote: repo.RepoPath(),
-		return err
+		Branch: fmt.Sprintf("%s:%s%s", oldBranchName, git.BranchPrefix, branchName),
-	}
-	defer func() {
-		if err := models.RemoveTemporaryPath(basePath); err != nil {
-			log.Error("CreateNewBranch: RemoveTemporaryPath: %s", err)
-		}
-	}()
-
-	if err := git.Clone(repo.RepoPath(), basePath, git.CloneRepoOptions{
-		Bare:   true,
-		Shared: true,
-	}); err != nil {
-		log.Error("Failed to clone repository: %s (%v)", repo.FullName(), err)
-		return fmt.Errorf("Failed to clone repository: %s (%v)", repo.FullName(), err)
-	}
-
-	gitRepo, err := git.OpenRepository(basePath)
-	if err != nil {
-		log.Error("Unable to open temporary repository: %s (%v)", basePath, err)
-		return fmt.Errorf("Failed to open new temporary repository in: %s %v", basePath, err)
-	}
-	defer gitRepo.Close()
-
-	if err = gitRepo.CreateBranch(branchName, oldBranchName); err != nil {
-		log.Error("Unable to create branch: %s from %s. (%v)", branchName, oldBranchName, err)
-		return fmt.Errorf("Unable to create branch: %s from %s. (%v)", branchName, oldBranchName, err)
-	}
-
-	if err = git.Push(basePath, git.PushOptions{
-		Remote: "origin",
-		Branch: branchName,
 		Env:    models.PushingEnvironment(doer, repo),
 	}); err != nil {
 		if git.IsErrPushOutOfDate(err) || git.IsErrPushRejected(err) {
@@ -126,39 +95,10 @@ func CreateNewBranchFromCommit(doer *models.User, repo *models.Repository, commi
 	if err := checkBranchName(repo, branchName); err != nil {
 		return err
 	}
-	basePath, err := models.CreateTemporaryPath("branch-maker")
-	if err != nil {
-		return err
-	}
-	defer func() {
-		if err := models.RemoveTemporaryPath(basePath); err != nil {
-			log.Error("CreateNewBranchFromCommit: RemoveTemporaryPath: %s", err)
-		}
-	}()
 
-	if err := git.Clone(repo.RepoPath(), basePath, git.CloneRepoOptions{
+	if err := git.Push(repo.RepoPath(), git.PushOptions{
-		Bare:   true,
+		Remote: repo.RepoPath(),
-		Shared: true,
+		Branch: fmt.Sprintf("%s:%s%s", commit, git.BranchPrefix, branchName),
-	}); err != nil {
-		log.Error("Failed to clone repository: %s (%v)", repo.FullName(), err)
-		return fmt.Errorf("Failed to clone repository: %s (%v)", repo.FullName(), err)
-	}
-
-	gitRepo, err := git.OpenRepository(basePath)
-	if err != nil {
-		log.Error("Unable to open temporary repository: %s (%v)", basePath, err)
-		return fmt.Errorf("Failed to open new temporary repository in: %s %v", basePath, err)
-	}
-	defer gitRepo.Close()
-
-	if err = gitRepo.CreateBranch(branchName, commit); err != nil {
-		log.Error("Unable to create branch: %s from %s. (%v)", branchName, commit, err)
-		return fmt.Errorf("Unable to create branch: %s from %s. (%v)", branchName, commit, err)
-	}
-
-	if err = git.Push(basePath, git.PushOptions{
-		Remote: "origin",
-		Branch: branchName,
 		Env:    models.PushingEnvironment(doer, repo),
 	}); err != nil {
 		if git.IsErrPushOutOfDate(err) || git.IsErrPushRejected(err) {

routers/repo/branch.go

@@ -6,6 +6,7 @@
 package repo
 
 import (
+	"fmt"
 	"strings"
 
 	"code.gitea.io/gitea/models"
@@ -102,7 +103,11 @@ func RestoreBranchPost(ctx *context.Context) {
 		return
 	}
 
-	if err := ctx.Repo.GitRepo.CreateBranch(deletedBranch.Name, deletedBranch.Commit); err != nil {
+	if err := git.Push(ctx.Repo.Repository.RepoPath(), git.PushOptions{
+		Remote: ctx.Repo.Repository.RepoPath(),
+		Branch: fmt.Sprintf("%s:%s%s", deletedBranch.Commit, git.BranchPrefix, deletedBranch.Name),
+		Env:    models.PushingEnvironment(ctx.User, ctx.Repo.Repository),
+	}); err != nil {
 		if strings.Contains(err.Error(), "already exists") {
 			ctx.Flash.Error(ctx.Tr("repo.branch.already_exists", deletedBranch.Name))
 			return
@@ -112,12 +117,6 @@ func RestoreBranchPost(ctx *context.Context) {
 		return
 	}
 
-	if err := ctx.Repo.Repository.RemoveDeletedBranch(deletedBranch.ID); err != nil {
-		log.Error("RemoveDeletedBranch: %v", err)
-		ctx.Flash.Error(ctx.Tr("repo.branch.restore_failed", deletedBranch.Name))
-		return
-	}
-
 	// Don't return error below this
 	if err := repofiles.PushUpdate(
 		ctx.Repo.Repository,
@@ -216,7 +215,7 @@ func loadBranches(ctx *context.Context) []*Branch {
 			}
 		}
 
-		divergence, divergenceError := repofiles.CountDivergingCommits(ctx.Repo.Repository, branchName)
+		divergence, divergenceError := repofiles.CountDivergingCommits(ctx.Repo.Repository, git.BranchPrefix+branchName)
 		if divergenceError != nil {
 			ctx.ServerError("CountDivergingCommits", divergenceError)
 			return nil
@@ -331,6 +330,8 @@ func CreateBranch(ctx *context.Context, form auth.NewBranchForm) {
 	var err error
 	if ctx.Repo.IsViewBranch {
 		err = repo_module.CreateNewBranch(ctx.User, ctx.Repo.Repository, ctx.Repo.BranchName, form.NewBranchName)
+	} else if ctx.Repo.IsViewTag {
+		err = repo_module.CreateNewBranchFromCommit(ctx.User, ctx.Repo.Repository, ctx.Repo.CommitID, form.NewBranchName)
 	} else {
 		err = repo_module.CreateNewBranchFromCommit(ctx.User, ctx.Repo.Repository, ctx.Repo.BranchName, form.NewBranchName)
 	}

routers/repo/compare.go

@@ -381,7 +381,20 @@ func ParseCompareInfo(ctx *context.Context) (*models.User, *models.Repository, *
 		return nil, nil, nil, nil, "", ""
 	}
 
-	compareInfo, err := headGitRepo.GetCompareInfo(baseRepo.RepoPath(), baseBranch, headBranch)
+	baseBranchRef := baseBranch
+	if baseIsBranch {
+		baseBranchRef = git.BranchPrefix + baseBranch
+	} else if baseIsTag {
+		baseBranchRef = git.TagPrefix + baseBranch
+	}
+	headBranchRef := headBranch
+	if headIsBranch {
+		headBranchRef = git.BranchPrefix + headBranch
+	} else if headIsTag {
+		headBranchRef = git.TagPrefix + headBranch
+	}
+
+	compareInfo, err := headGitRepo.GetCompareInfo(baseRepo.RepoPath(), baseBranchRef, headBranchRef)
 	if err != nil {
 		ctx.ServerError("GetCompareInfo", err)
 		return nil, nil, nil, nil, "", ""

routers/repo/pull.go

@@ -486,7 +486,7 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare
 	}
 
 	compareInfo, err := baseGitRepo.GetCompareInfo(pull.BaseRepo.RepoPath(),
-		pull.BaseBranch, pull.GetGitRefName())
+		git.BranchPrefix+pull.BaseBranch, pull.GetGitRefName())
 	if err != nil {
 		if strings.Contains(err.Error(), "fatal: Not a valid object name") {
 			ctx.Data["IsPullRequestBroken"] = true

services/pull/pull.go

@@ -66,7 +66,7 @@ func NewPullRequest(repo *models.Repository, pull *models.Issue, labelIDs []int6
 	defer baseGitRepo.Close()
 
 	compareInfo, err := baseGitRepo.GetCompareInfo(pr.BaseRepo.RepoPath(),
-		pr.BaseBranch, pr.GetGitRefName())
+		git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName())
 	if err != nil {
 		return err
 	}

services/pull/temp_repo.go

@@ -100,7 +100,7 @@ func createTemporaryRepo(pr *models.PullRequest) (string, error) {
 	outbuf.Reset()
 	errbuf.Reset()
 
-	if err := git.NewCommand("fetch", "origin", "--no-tags", pr.BaseBranch+":"+baseBranch, pr.BaseBranch+":original_"+baseBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil {
+	if err := git.NewCommand("fetch", "origin", "--no-tags", "--", pr.BaseBranch+":"+baseBranch, pr.BaseBranch+":original_"+baseBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil {
 		log.Error("Unable to fetch origin base branch [%s:%s -> base, original_base in %s]: %v:\n%s\n%s", pr.BaseRepo.FullName(), pr.BaseBranch, tmpBasePath, err, outbuf.String(), errbuf.String())
 		if err := models.RemoveTemporaryPath(tmpBasePath); err != nil {
 			log.Error("CreateTempRepo: RemoveTemporaryPath: %s", err)
@@ -140,7 +140,7 @@ func createTemporaryRepo(pr *models.PullRequest) (string, error) {
 
 	trackingBranch := "tracking"
 	// Fetch head branch
-	if err := git.NewCommand("fetch", "--no-tags", remoteRepoName, pr.HeadBranch+":"+trackingBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil {
+	if err := git.NewCommand("fetch", "--no-tags", remoteRepoName, git.BranchPrefix+pr.HeadBranch+":"+trackingBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil {
 		log.Error("Unable to fetch head_repo head branch [%s:%s -> tracking in %s]: %v:\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, tmpBasePath, err, outbuf.String(), errbuf.String())
 		if err := models.RemoveTemporaryPath(tmpBasePath); err != nil {
 			log.Error("CreateTempRepo: RemoveTemporaryPath: %s", err)