From 320031fce693ffe8869da952f3f872b46d3d6550 Mon Sep 17 00:00:00 2001 From: zeripath Date: Fri, 12 Jun 2020 19:01:44 +0100 Subject: [PATCH] Handle more pathological branch and tag names (#11843) (#11863) Backport #11843 It's possible to push quite pathological appearing branch names to gitea using git push gitea reasonable-branch:refs/heads/-- at which point large parts of the UI will break. Similarly you can git push origin reasonable-tag:refs/tags/-- which wil return an error. This PR fixes the problems these cause. It also changes the code from creating branches to pushing to ensure that branch restoration has to pass hooks. 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/temp_repo.go | 4 +- 7 files changed, 41 insertions(+), 87 deletions(-) diff --git a/integrations/branches_test.go b/integrations/branches_test.go index ed6a73fdb4..2b9fc8dda5 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 c5f6d6cdd6..18c732c6f2 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 418ba25c89..358e59f39f 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 @@ -74,39 +73,9 @@ func CreateNewBranch(doer *models.User, repo *models.Repository, oldBranchName, return fmt.Errorf("OldBranch: %s does not exist. Cannot create new branch from this", 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) { @@ -124,39 +93,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 e7eac04bce..4d8b9158fe 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 97bb5e6b18..3ffabc14a1 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 ec82d14f18..da826026b3 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -484,7 +484,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/temp_repo.go b/services/pull/temp_repo.go index 91f48d0626..45cd10b65b 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)