Skip to content

Commit 320031f

Browse files
zeripathtechknowlogick
zeripath
and
techknowlogick
authored
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 <[email protected]> Co-authored-by: techknowlogick <[email protected]>
1 parent ef2f189 commit 320031f

File tree

7 files changed

+41
-87
lines changed

7 files changed

+41
-87
lines changed

‎integrations/branches_test.go‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -32,14 +32,14 @@ func TestDeleteBranch(t *testing.T) {
3232
}
3333

3434
func TestUndoDeleteBranch(t *testing.T) {
35-
defer prepareTestEnv(t)()
36-
37-
deleteBranch(t)
38-
htmlDoc, name := branchAction(t, ".undo-button")
39-
assert.Contains(t,
40-
htmlDoc.doc.Find(".ui.positive.message").Text(),
41-
i18n.Tr("en", "repo.branch.restore_success", name),
42-
)
35+
onGiteaRun(t, func(t *testing.T, u *url.URL) {
36+
deleteBranch(t)
37+
htmlDoc, name := branchAction(t, ".undo-button")
38+
assert.Contains(t,
39+
htmlDoc.doc.Find(".ui.positive.message").Text(),
40+
i18n.Tr("en", "repo.branch.restore_success", name),
41+
)
42+
})
4343
}
4444

4545
func deleteBranch(t *testing.T) {

‎modules/git/repo_commit.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ func (repo *Repository) GetBranchCommitID(name string) (string, error) {
4646

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

‎modules/repository/branch.go‎

Lines changed: 6 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ import (
99

1010
"code.gitea.io/gitea/models"
1111
"code.gitea.io/gitea/modules/git"
12-
"code.gitea.io/gitea/modules/log"
1312
)
1413

1514
// GetBranch returns a branch by its name
@@ -74,39 +73,9 @@ func CreateNewBranch(doer *models.User, repo *models.Repository, oldBranchName,
7473
return fmt.Errorf("OldBranch: %s does not exist. Cannot create new branch from this", oldBranchName)
7574
}
7675

77-
basePath, err := models.CreateTemporaryPath("branch-maker")
78-
if err != nil {
79-
return err
80-
}
81-
defer func() {
82-
if err := models.RemoveTemporaryPath(basePath); err != nil {
83-
log.Error("CreateNewBranch: RemoveTemporaryPath: %s", err)
84-
}
85-
}()
86-
87-
if err := git.Clone(repo.RepoPath(), basePath, git.CloneRepoOptions{
88-
Bare: true,
89-
Shared: true,
90-
}); err != nil {
91-
log.Error("Failed to clone repository: %s (%v)", repo.FullName(), err)
92-
return fmt.Errorf("Failed to clone repository: %s (%v)", repo.FullName(), err)
93-
}
94-
95-
gitRepo, err := git.OpenRepository(basePath)
96-
if err != nil {
97-
log.Error("Unable to open temporary repository: %s (%v)", basePath, err)
98-
return fmt.Errorf("Failed to open new temporary repository in: %s %v", basePath, err)
99-
}
100-
defer gitRepo.Close()
101-
102-
if err = gitRepo.CreateBranch(branchName, oldBranchName); err != nil {
103-
log.Error("Unable to create branch: %s from %s. (%v)", branchName, oldBranchName, err)
104-
return fmt.Errorf("Unable to create branch: %s from %s. (%v)", branchName, oldBranchName, err)
105-
}
106-
107-
if err = git.Push(basePath, git.PushOptions{
108-
Remote: "origin",
109-
Branch: branchName,
76+
if err := git.Push(repo.RepoPath(), git.PushOptions{
77+
Remote: repo.RepoPath(),
78+
Branch: fmt.Sprintf("%s:%s%s", oldBranchName, git.BranchPrefix, branchName),
11079
Env: models.PushingEnvironment(doer, repo),
11180
}); err != nil {
11281
if git.IsErrPushOutOfDate(err) || git.IsErrPushRejected(err) {
@@ -124,39 +93,10 @@ func CreateNewBranchFromCommit(doer *models.User, repo *models.Repository, commi
12493
if err := checkBranchName(repo, branchName); err != nil {
12594
return err
12695
}
127-
basePath, err := models.CreateTemporaryPath("branch-maker")
128-
if err != nil {
129-
return err
130-
}
131-
defer func() {
132-
if err := models.RemoveTemporaryPath(basePath); err != nil {
133-
log.Error("CreateNewBranchFromCommit: RemoveTemporaryPath: %s", err)
134-
}
135-
}()
136-
137-
if err := git.Clone(repo.RepoPath(), basePath, git.CloneRepoOptions{
138-
Bare: true,
139-
Shared: true,
140-
}); err != nil {
141-
log.Error("Failed to clone repository: %s (%v)", repo.FullName(), err)
142-
return fmt.Errorf("Failed to clone repository: %s (%v)", repo.FullName(), err)
143-
}
144-
145-
gitRepo, err := git.OpenRepository(basePath)
146-
if err != nil {
147-
log.Error("Unable to open temporary repository: %s (%v)", basePath, err)
148-
return fmt.Errorf("Failed to open new temporary repository in: %s %v", basePath, err)
149-
}
150-
defer gitRepo.Close()
151-
152-
if err = gitRepo.CreateBranch(branchName, commit); err != nil {
153-
log.Error("Unable to create branch: %s from %s. (%v)", branchName, commit, err)
154-
return fmt.Errorf("Unable to create branch: %s from %s. (%v)", branchName, commit, err)
155-
}
15696

157-
if err = git.Push(basePath, git.PushOptions{
158-
Remote: "origin",
159-
Branch: branchName,
97+
if err := git.Push(repo.RepoPath(), git.PushOptions{
98+
Remote: repo.RepoPath(),
99+
Branch: fmt.Sprintf("%s:%s%s", commit, git.BranchPrefix, branchName),
160100
Env: models.PushingEnvironment(doer, repo),
161101
}); err != nil {
162102
if git.IsErrPushOutOfDate(err) || git.IsErrPushRejected(err) {

‎routers/repo/branch.go‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
package repo
77

88
import (
9+
"fmt"
910
"strings"
1011

1112
"code.gitea.io/gitea/models"
@@ -102,7 +103,11 @@ func RestoreBranchPost(ctx *context.Context) {
102103
return
103104
}
104105

105-
if err := ctx.Repo.GitRepo.CreateBranch(deletedBranch.Name, deletedBranch.Commit); err != nil {
106+
if err := git.Push(ctx.Repo.Repository.RepoPath(), git.PushOptions{
107+
Remote: ctx.Repo.Repository.RepoPath(),
108+
Branch: fmt.Sprintf("%s:%s%s", deletedBranch.Commit, git.BranchPrefix, deletedBranch.Name),
109+
Env: models.PushingEnvironment(ctx.User, ctx.Repo.Repository),
110+
}); err != nil {
106111
if strings.Contains(err.Error(), "already exists") {
107112
ctx.Flash.Error(ctx.Tr("repo.branch.already_exists", deletedBranch.Name))
108113
return
@@ -112,12 +117,6 @@ func RestoreBranchPost(ctx *context.Context) {
112117
return
113118
}
114119

115-
if err := ctx.Repo.Repository.RemoveDeletedBranch(deletedBranch.ID); err != nil {
116-
log.Error("RemoveDeletedBranch: %v", err)
117-
ctx.Flash.Error(ctx.Tr("repo.branch.restore_failed", deletedBranch.Name))
118-
return
119-
}
120-
121120
// Don't return error below this
122121
if err := repofiles.PushUpdate(
123122
ctx.Repo.Repository,
@@ -216,7 +215,7 @@ func loadBranches(ctx *context.Context) []*Branch {
216215
}
217216
}
218217

219-
divergence, divergenceError := repofiles.CountDivergingCommits(ctx.Repo.Repository, branchName)
218+
divergence, divergenceError := repofiles.CountDivergingCommits(ctx.Repo.Repository, git.BranchPrefix+branchName)
220219
if divergenceError != nil {
221220
ctx.ServerError("CountDivergingCommits", divergenceError)
222221
return nil
@@ -331,6 +330,8 @@ func CreateBranch(ctx *context.Context, form auth.NewBranchForm) {
331330
var err error
332331
if ctx.Repo.IsViewBranch {
333332
err = repo_module.CreateNewBranch(ctx.User, ctx.Repo.Repository, ctx.Repo.BranchName, form.NewBranchName)
333+
} else if ctx.Repo.IsViewTag {
334+
err = repo_module.CreateNewBranchFromCommit(ctx.User, ctx.Repo.Repository, ctx.Repo.CommitID, form.NewBranchName)
334335
} else {
335336
err = repo_module.CreateNewBranchFromCommit(ctx.User, ctx.Repo.Repository, ctx.Repo.BranchName, form.NewBranchName)
336337
}

‎routers/repo/compare.go‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -381,7 +381,20 @@ func ParseCompareInfo(ctx *context.Context) (*models.User, *models.Repository, *
381381
return nil, nil, nil, nil, "", ""
382382
}
383383

384-
compareInfo, err := headGitRepo.GetCompareInfo(baseRepo.RepoPath(), baseBranch, headBranch)
384+
baseBranchRef := baseBranch
385+
if baseIsBranch {
386+
baseBranchRef = git.BranchPrefix + baseBranch
387+
} else if baseIsTag {
388+
baseBranchRef = git.TagPrefix + baseBranch
389+
}
390+
headBranchRef := headBranch
391+
if headIsBranch {
392+
headBranchRef = git.BranchPrefix + headBranch
393+
} else if headIsTag {
394+
headBranchRef = git.TagPrefix + headBranch
395+
}
396+
397+
compareInfo, err := headGitRepo.GetCompareInfo(baseRepo.RepoPath(), baseBranchRef, headBranchRef)
385398
if err != nil {
386399
ctx.ServerError("GetCompareInfo", err)
387400
return nil, nil, nil, nil, "", ""

‎routers/repo/pull.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -484,7 +484,7 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare
484484
}
485485

486486
compareInfo, err := baseGitRepo.GetCompareInfo(pull.BaseRepo.RepoPath(),
487-
pull.BaseBranch, pull.GetGitRefName())
487+
git.BranchPrefix+pull.BaseBranch, pull.GetGitRefName())
488488
if err != nil {
489489
if strings.Contains(err.Error(), "fatal: Not a valid object name") {
490490
ctx.Data["IsPullRequestBroken"] = true

‎services/pull/temp_repo.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ func createTemporaryRepo(pr *models.PullRequest) (string, error) {
100100
outbuf.Reset()
101101
errbuf.Reset()
102102

103-
if err := git.NewCommand("fetch", "origin", "--no-tags", pr.BaseBranch+":"+baseBranch, pr.BaseBranch+":original_"+baseBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil {
103+
if err := git.NewCommand("fetch", "origin", "--no-tags", "--", pr.BaseBranch+":"+baseBranch, pr.BaseBranch+":original_"+baseBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil {
104104
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())
105105
if err := models.RemoveTemporaryPath(tmpBasePath); err != nil {
106106
log.Error("CreateTempRepo: RemoveTemporaryPath: %s", err)
@@ -140,7 +140,7 @@ func createTemporaryRepo(pr *models.PullRequest) (string, error) {
140140

141141
trackingBranch := "tracking"
142142
// Fetch head branch
143-
if err := git.NewCommand("fetch", "--no-tags", remoteRepoName, pr.HeadBranch+":"+trackingBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil {
143+
if err := git.NewCommand("fetch", "--no-tags", remoteRepoName, git.BranchPrefix+pr.HeadBranch+":"+trackingBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil {
144144
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())
145145
if err := models.RemoveTemporaryPath(tmpBasePath); err != nil {
146146
log.Error("CreateTempRepo: RemoveTemporaryPath: %s", err)

0 commit comments

Comments
 (0)