From 0973c036019c955172ebce99d58eede2e9ac55ca Mon Sep 17 00:00:00 2001 From: zeripath Date: Fri, 12 Jun 2020 00:49:47 +0100 Subject: [PATCH] Handle more pathological branch and tag names (#11843) * Handle more pathological branch and tag names Signed-off-by: Andrew Thornton * Fix failing test Signed-off-by: Andrew Thornton Co-authored-by: techknowlogick --- integrations/branches_test.go | 16 ++++---- modules/git/repo_commit.go | 2 +- modules/repository/branch.go | 72 +++-------------------------------- routers/repo/branch.go | 17 +++++---- routers/repo/compare.go | 15 +++++++- routers/repo/pull.go | 2 +- services/pull/pull.go | 2 +- services/pull/temp_repo.go | 4 +- 8 files changed, 42 insertions(+), 88 deletions(-) diff --git a/integrations/branches_test.go b/integrations/branches_test.go index ed6a73fdb..2b9fc8dda 100644 --- a/integrations/branches_test.go +++ b/integrations/branches_test.go @@ -32,14 +32,14 @@ func TestDeleteBranch(t *testing.T) { } func TestUndoDeleteBranch(t *testing.T) { - defer prepareTestEnv(t)() - - deleteBranch(t) - htmlDoc, name := branchAction(t, ".undo-button") - assert.Contains(t, - htmlDoc.doc.Find(".ui.positive.message").Text(), - i18n.Tr("en", "repo.branch.restore_success", name), - ) + onGiteaRun(t, func(t *testing.T, u *url.URL) { + deleteBranch(t) + htmlDoc, name := branchAction(t, ".undo-button") + assert.Contains(t, + htmlDoc.doc.Find(".ui.positive.message").Text(), + i18n.Tr("en", "repo.branch.restore_success", name), + ) + }) } func deleteBranch(t *testing.T) { diff --git a/modules/git/repo_commit.go b/modules/git/repo_commit.go index e282c9c67..479a0d037 100644 --- a/modules/git/repo_commit.go +++ b/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, ""} diff --git a/modules/repository/branch.go b/modules/repository/branch.go index 94be6f0f5..d369a200b 100644 --- a/modules/repository/branch.go +++ b/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 != nil { - return err - } - 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, + if err := git.Push(repo.RepoPath(), git.PushOptions{ + Remote: repo.RepoPath(), + Branch: fmt.Sprintf("%s:%s%s", oldBranchName, git.BranchPrefix, 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{ - 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, 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, + if err := git.Push(repo.RepoPath(), git.PushOptions{ + Remote: repo.RepoPath(), + Branch: fmt.Sprintf("%s:%s%s", commit, git.BranchPrefix, branchName), Env: models.PushingEnvironment(doer, repo), }); err != nil { if git.IsErrPushOutOfDate(err) || git.IsErrPushRejected(err) { diff --git a/routers/repo/branch.go b/routers/repo/branch.go index e7eac04bc..4d8b9158f 100644 --- a/routers/repo/branch.go +++ b/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) } diff --git a/routers/repo/compare.go b/routers/repo/compare.go index 858e4e548..25aff0eeb 100644 --- a/routers/repo/compare.go +++ b/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, "", "" diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 42846fb93..ebc4439dd 100644 --- a/routers/repo/pull.go +++ b/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 diff --git a/services/pull/pull.go b/services/pull/pull.go index e8912e47c..f7a187848 100644 --- a/services/pull/pull.go +++ b/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 } diff --git a/services/pull/temp_repo.go b/services/pull/temp_repo.go index 91f48d062..45cd10b65 100644 --- a/services/pull/temp_repo.go +++ b/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)