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.

Sign up for GitHub

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

Jump to bottom

Handle more pathological branch and tag names (#11843) #11863

Merged
merged 2 commits into from
Jun 12, 2020
Merged

Handle more pathological branch and tag names (#11843) #11863

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a4fc1d7
Handle more pathological branch and tag names (#11843)
zeripath Jun 11, 2020
bec27f0
Merge branch 'release/v1.12' into backport-11843
techknowlogick Jun 12, 2020
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
Next Next commit
Handle more pathological branch and tag names (#11843) 
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 <[email protected]>
  • Loading branch information
@zeripath
zeripath committed Jun 12, 2020
commit a4fc1d778b838a3571c88d154a73435c1570cebe
16 changes: 8 additions & 8 deletions integrations/branches_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion modules/git/repo_commit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, ""}
Expand Down
72 changes: 6 additions & 66 deletions modules/repository/branch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down
17 changes: 9 additions & 8 deletions routers/repo/branch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package repo

import (
"fmt"
"strings"

"code.gitea.io/gitea/models"
Expand Down Expand Up @@ -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
Expand All @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
Expand Down
15 changes: 14 additions & 1 deletion routers/repo/compare.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, "", ""
Expand Down
2 changes: 1 addition & 1 deletion routers/repo/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions services/pull/temp_repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down