Add a simple way to rename branch like gh #15870

Merged
a1012112796 merged 15 commits from rename_branch into main 2021-10-08 17:03:04 +00:00
14 changed files with 357 additions and 1 deletions

@ -0,0 +1,44 @@
// Copyright 2021 The Gitea Authors. All rights reserved.
// Use of this source code is governed by a MIT-style
// license that can be found in the LICENSE file.
package integrations
import (
"net/http"
"testing"
"code.gitea.io/gitea/models"
"code.gitea.io/gitea/models/db"
"github.com/stretchr/testify/assert"
)
func TestRenameBranch(t *testing.T) {
// get branch setting page
session := loginUser(t, "user2")
req := NewRequest(t, "GET", "/user2/repo1/settings/branches")
resp := session.MakeRequest(t, req, http.StatusOK)
Review

That can be refactored once #16052 gets merged. Will then look like

	token := getUserToken(t, "user2")
	req := NewRequest(t, "GET", "/user2/repo1/settings/branches")
	resp := session.MakeRequest(t, WithToken(req, token), http.StatusOK)

if I understand the refactoring taken there correctly.

That can be refactored once #16052 gets merged. Will then look like ```suggestion token := getUserToken(t, "user2") req := NewRequest(t, "GET", "/user2/repo1/settings/branches") resp := session.MakeRequest(t, WithToken(req, token), http.StatusOK) ``` if I understand the refactoring taken there correctly.
htmlDoc := NewHTMLParser(t, resp.Body)
postData := map[string]string{
"_csrf": htmlDoc.GetCSRF(),
"from": "master",
"to": "main",
}
req = NewRequestWithValues(t, "POST", "/user2/repo1/settings/rename_branch", postData)
session.MakeRequest(t, req, http.StatusFound)
Review

Will then be

	MakeRequest(t, WithToken(req, token), http.StatusFound)
Will then be ```suggestion MakeRequest(t, WithToken(req, token), http.StatusFound) ```
// check new branch link
req = NewRequestWithValues(t, "GET", "/user2/repo1/src/branch/main/README.md", postData)
session.MakeRequest(t, req, http.StatusOK)
Review

And this will be

	MakeRequest(t, WithToken(req, token), http.StatusOK)
And this will be ```suggestion MakeRequest(t, WithToken(req, token), http.StatusOK) ```
// check old branch link
req = NewRequestWithValues(t, "GET", "/user2/repo1/src/branch/master/README.md", postData)
resp = session.MakeRequest(t, req, http.StatusFound)
Review

And this will also be

	resp = MakeRequest(t, WithToken(req, token), http.StatusFound)
And this will also be ```suggestion resp = MakeRequest(t, WithToken(req, token), http.StatusFound) ```
location := resp.HeaderMap.Get("Location")
assert.Equal(t, "/user2/repo1/src/branch/main/README.md", location)
// check db
repo1 := db.AssertExistsAndLoadBean(t, &models.Repository{ID: 1}).(*models.Repository)
assert.Equal(t, "main", repo1.DefaultBranch)
}

@ -53,6 +53,7 @@ type ProtectedBranch struct {
func init() {
db.RegisterModel(new(ProtectedBranch))
db.RegisterModel(new(DeletedBranch))
db.RegisterModel(new(RenamedBranch))
}
// IsProtected returns if the branch is protected
@ -588,3 +589,83 @@ func RemoveOldDeletedBranches(ctx context.Context, olderThan time.Duration) {
log.Error("DeletedBranchesCleanup: %v", err)
}
}
// RenamedBranch provide renamed branch log
// will check it when a branch can't be found
type RenamedBranch struct {
Review

Need a condition ID(repo.ID).

Need a condition `ID(repo.ID)`.
Review

sess.Close will automatically Rollback if not Commit, so we can safely remove these lines.

`sess.Close` will automatically `Rollback` if not `Commit`, so we can safely remove these lines.
ID int64 `xorm:"pk autoincr"`
RepoID int64 `xorm:"INDEX NOT NULL"`
From string
To string
CreatedUnix timeutil.TimeStamp `xorm:"created"`
}
// FindRenamedBranch check if a branch was renamed
func FindRenamedBranch(repoID int64, from string) (branch *RenamedBranch, exist bool, err error) {
branch = &RenamedBranch{
RepoID: repoID,
From: from,
}
exist, err = db.GetEngine(db.DefaultContext).Get(branch)
return
}
// RenameBranch rename a branch
func (repo *Repository) RenameBranch(from, to string, gitAction func(isDefault bool) error) (err error) {
sess := db.NewSession(db.DefaultContext)
6543 commented 2021-05-15 09:59:59 +00:00 (Migrated from github.com)
Review

can you add a time stamp?

CreatedUnix      timeutil.TimeStamp `xorm:"INDEX created"`
can you add a time stamp? ```go CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` ```
a1012112796 commented 2021-05-15 12:18:39 +00:00 (Migrated from github.com)
Review

I don't think it's usefull...

I don't think it's usefull...
6543 commented 2021-05-15 14:26:46 +00:00 (Migrated from github.com)
Review

I'm having some prune after time X for admins in mind ...

I'm having some prune after time X for admins in mind ...
defer sess.Close()
if err := sess.Begin(); err != nil {
return err
}
// 1. update default branch if needed
isDefault := repo.DefaultBranch == from
if isDefault {
repo.DefaultBranch = to
_, err = sess.ID(repo.ID).Cols("default_branch").Update(repo)
if err != nil {
return err
}
Review

Am I seeing this wrong, or is it a bad idea to automatically apply branch protection to a "new" branch without testing if that branch should be protected?
Because in the current implementation, if I see this correctly, the following would lead to an inconsistent state:

  1. Have branch protection only applied to branches m.*
  2. -> Branch main is a protected branch
  3. Rename main -> default.
  4. default now has branch protection even though it shouldn't.

Furthermore, if I see that correctly, the type of branch protection also needs to be switched then if this branch is covered by another rule.

Am I seeing this wrong, or is it a bad idea to automatically apply branch protection to a "new" branch without testing if that branch should be protected? Because in the current implementation, if I see this correctly, the following would lead to an inconsistent state: 1. Have branch protection only applied to branches `m.*` 2. -> Branch `main` is a protected branch 3. Rename `main` -> `default`. 3. `default` now has branch protection even though it shouldn't. Furthermore, if I see that correctly, the type of branch protection also needs to be switched then if this branch is covered by another rule.
a1012112796 commented 2021-10-03 04:58:15 +00:00 (Migrated from github.com)
Review

Have branch protection only applied to branches m.*

sadly, gitea don't have the feature to protect more than one branches by one rule.
each branch protect rule only applied to one brach. so when the branch was renamed.
the old config is not usefull, which should be binded to new branch or be removed, I think bind it
to new branch is better.

> Have branch protection only applied to branches m.* sadly, gitea don't have the feature to protect more than one branches by one rule. each branch protect rule only applied to one brach. so when the branch was renamed. the old config is not usefull, which should be binded to new branch or be removed, I think bind it to new branch is better.
Review

Ah, thanks for the clarification. I did not need to set up branch protection on Gitea until now, I thought it was implemented just as in GitHub using a pattern for branches. Gitea's approach definitely has its advantages as well as its disadvantages.
In that case, I completely understand why you keep the branch protection. Makes total sense.

Ah, thanks for the clarification. I did not need to set up branch protection on Gitea until now, I thought it was implemented just as in GitHub using a pattern for branches. Gitea's approach definitely has its advantages as well as its disadvantages. In that case, I completely understand why you keep the branch protection. Makes total sense.
}
// 2. Update protected branch if needed
protectedBranch, err := getProtectedBranchBy(sess, repo.ID, from)
if err != nil {
return err
}
if protectedBranch != nil {
protectedBranch.BranchName = to
_, err = sess.ID(protectedBranch.ID).Cols("branch_name").Update(protectedBranch)
if err != nil {
return err
}
}
// 3. Update all not merged pull request base branch name
_, err = sess.Table(new(PullRequest)).Where("base_repo_id=? AND base_branch=? AND has_merged=?",
repo.ID, from, false).
Update(map[string]interface{}{"base_branch": to})
if err != nil {
return err
}
// 4. do git action
if err = gitAction(isDefault); err != nil {
return err
}
// 5. insert renamed branch record
renamedBranch := &RenamedBranch{
RepoID: repo.ID,
From: from,
To: to,
}
_, err = sess.Insert(renamedBranch)
if err != nil {
return err
}
return sess.Commit()
}

@ -79,3 +79,52 @@ func getDeletedBranch(t *testing.T, branch *DeletedBranch) *DeletedBranch {
return deletedBranch
}
func TestFindRenamedBranch(t *testing.T) {
assert.NoError(t, db.PrepareTestDatabase())
branch, exist, err := FindRenamedBranch(1, "dev")
assert.NoError(t, err)
assert.Equal(t, true, exist)
assert.Equal(t, "master", branch.To)
_, exist, err = FindRenamedBranch(1, "unknow")
assert.NoError(t, err)
assert.Equal(t, false, exist)
}
func TestRenameBranch(t *testing.T) {
assert.NoError(t, db.PrepareTestDatabase())
repo1 := db.AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository)
_isDefault := false
err := UpdateProtectBranch(repo1, &ProtectedBranch{
RepoID: repo1.ID,
BranchName: "master",
}, WhitelistOptions{})
assert.NoError(t, err)
assert.NoError(t, repo1.RenameBranch("master", "main", func(isDefault bool) error {
_isDefault = isDefault
return nil
}))
assert.Equal(t, true, _isDefault)
repo1 = db.AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository)
assert.Equal(t, "main", repo1.DefaultBranch)
pull := db.AssertExistsAndLoadBean(t, &PullRequest{ID: 1}).(*PullRequest) // merged
assert.Equal(t, "master", pull.BaseBranch)
pull = db.AssertExistsAndLoadBean(t, &PullRequest{ID: 2}).(*PullRequest) // open
assert.Equal(t, "main", pull.BaseBranch)
renamedBranch := db.AssertExistsAndLoadBean(t, &RenamedBranch{ID: 2}).(*RenamedBranch)
assert.Equal(t, "master", renamedBranch.From)
assert.Equal(t, "main", renamedBranch.To)
assert.Equal(t, int64(1), renamedBranch.RepoID)
db.AssertExistsAndLoadBean(t, &ProtectedBranch{
RepoID: repo1.ID,
BranchName: "main",
})
}

@ -0,0 +1,5 @@
-
id: 1
repo_id: 1
from: dev
to: master

@ -346,6 +346,8 @@ var migrations = []Migration{
NewMigration("Add table commit_status_index", addTableCommitStatusIndex),
// v196 -> v197
NewMigration("Add Color to ProjectBoard table", addColorColToProjectBoard),
// v197 -> v198
NewMigration("Add renamed_branch table", addRenamedBranchTable),
}
// GetCurrentDBVersion returns the current db version

20
models/migrations/v197.go Normal file

@ -0,0 +1,20 @@
// Copyright 2021 The Gitea Authors. All rights reserved.
// Use of this source code is governed by a MIT-style
// license that can be found in the LICENSE file.
package migrations
import (
"xorm.io/xorm"
)
func addRenamedBranchTable(x *xorm.Engine) error {
type RenamedBranch struct {
ID int64 `xorm:"pk autoincr"`
RepoID int64 `xorm:"INDEX NOT NULL"`
From string
To string
CreatedUnix int64 `xorm:"created"`
}
return x.Sync2(new(RenamedBranch))
}

@ -705,7 +705,28 @@ func getRefName(ctx *Context, pathType RepoRefType) string {
ctx.Repo.TreePath = path
return ctx.Repo.Repository.DefaultBranch
case RepoRefBranch:
return getRefNameFromPath(ctx, path, ctx.Repo.GitRepo.IsBranchExist)
ref := getRefNameFromPath(ctx, path, ctx.Repo.GitRepo.IsBranchExist)
if len(ref) == 0 {
// maybe it's a renamed branch
return getRefNameFromPath(ctx, path, func(s string) bool {
b, exist, err := models.FindRenamedBranch(ctx.Repo.Repository.ID, s)
if err != nil {
log.Error("FindRenamedBranch", err)
return false
}
if !exist {
return false
}
ctx.Data["IsRenamedBranch"] = true
ctx.Data["RenamedBranchName"] = b.To
return true
})
}
return ref
case RepoRefTag:
return getRefNameFromPath(ctx, path, ctx.Repo.GitRepo.IsTagExist)
case RepoRefCommit:
@ -784,6 +805,15 @@ func RepoRefByType(refType RepoRefType, ignoreNotExistErr ...bool) func(*Context
} else {
refName = getRefName(ctx, refType)
ctx.Repo.BranchName = refName
isRenamedBranch, has := ctx.Data["IsRenamedBranch"].(bool)
if isRenamedBranch && has {
renamedBranchName := ctx.Data["RenamedBranchName"].(string)
ctx.Flash.Info(ctx.Tr("repo.branch.renamed", refName, renamedBranchName))
link := strings.Replace(ctx.Req.RequestURI, refName, renamedBranchName, 1)
ctx.Redirect(link)
return
}
if refType.RefTypeIncludesBranches() && ctx.Repo.GitRepo.IsBranchExist(refName) {
ctx.Repo.IsViewBranch = true

@ -164,3 +164,9 @@ func (repo *Repository) RemoveRemote(name string) error {
func (branch *Branch) GetCommit() (*Commit, error) {
return branch.gitRepo.GetBranchCommit(branch.Name)
}
// RenameBranch rename a branch
func (repo *Repository) RenameBranch(from, to string) error {
_, err := NewCommand("branch", "-m", from, to).RunInDir(repo.Path)
return err
}

@ -1985,6 +1985,12 @@ settings.lfs_pointers.inRepo=In Repo
settings.lfs_pointers.exists=Exists in store
settings.lfs_pointers.accessible=Accessible to User
settings.lfs_pointers.associateAccessible=Associate accessible %d OIDs
settings.rename_branch_failed_exist=Cannot rename branch because target branch %s exists.
settings.rename_branch_failed_not_exist=Cannot rename branch %s because it does not exist.
settings.rename_branch_success =Branch %s was successfully renamed to %s.
settings.rename_branch_from=old branch name
settings.rename_branch_to=new branch name
settings.rename_branch=Rename branch
diff.browse_source = Browse Source
diff.parent = parent
@ -2106,6 +2112,7 @@ branch.create_new_branch = Create branch from branch:
branch.confirm_create_branch = Create branch
branch.new_branch = Create new branch
branch.new_branch_from = Create new branch from '%s'
branch.renamed = Branch %s was renamed to %s.
tag.create_tag = Create tag <strong>%s</strong>
tag.create_success = Tag '%s' has been created.

@ -19,6 +19,7 @@ import (
"code.gitea.io/gitea/modules/web"
"code.gitea.io/gitea/services/forms"
pull_service "code.gitea.io/gitea/services/pull"
"code.gitea.io/gitea/services/repository"
)
// ProtectedBranch render the page to protect the repository
@ -285,3 +286,40 @@ func SettingsProtectedBranchPost(ctx *context.Context) {
ctx.Redirect(fmt.Sprintf("%s/settings/branches", ctx.Repo.RepoLink))
}
}
// RenameBranchPost responses for rename a branch
func RenameBranchPost(ctx *context.Context) {
form := web.GetForm(ctx).(*forms.RenameBranchForm)
if !ctx.Repo.CanCreateBranch() {
ctx.NotFound("RenameBranch", nil)
return
}
if ctx.HasError() {
Review

I would like a , ok check here to ensure that the returned form isn't accidentally of another type.
Because in that case, gitea would most likely panic here which I doubt is wanted.

I would like a `, ok` check here to ensure that the returned form isn't accidentally of another type. Because in that case, gitea would most likely panic here which I doubt is wanted.
a1012112796 commented 2021-10-03 04:49:14 +00:00 (Migrated from github.com)
Review

Don't worry, middleware.Validate will check form before this handle.

Don't worry, `middleware.Validate` will check form before this handle.
ctx.Flash.Error(ctx.GetErrMsg())
ctx.Redirect(fmt.Sprintf("%s/settings/branches", ctx.Repo.RepoLink))
return
}
msg, err := repository.RenameBranch(ctx.Repo.Repository, ctx.User, ctx.Repo.GitRepo, form.From, form.To)
if err != nil {
ctx.ServerError("RenameBranch", err)
return
}
if msg == "target_exist" {
ctx.Flash.Error(ctx.Tr("repo.settings.rename_branch_failed_exist", form.To))
ctx.Redirect(fmt.Sprintf("%s/settings/branches", ctx.Repo.RepoLink))
return
}
if msg == "from_not_exist" {
ctx.Flash.Error(ctx.Tr("repo.settings.rename_branch_failed_not_exist", form.From))
ctx.Redirect(fmt.Sprintf("%s/settings/branches", ctx.Repo.RepoLink))
return
}
ctx.Flash.Success(ctx.Tr("repo.settings.rename_branch_success", form.From, form.To))
ctx.Redirect(fmt.Sprintf("%s/settings/branches", ctx.Repo.RepoLink))
}

@ -612,6 +612,7 @@ func RegisterRoutes(m *web.Route) {
m.Combo("/*").Get(repo.SettingsProtectedBranch).
Post(bindIgnErr(forms.ProtectBranchForm{}), context.RepoMustNotBeArchived(), repo.SettingsProtectedBranchPost)
}, repo.MustBeNotEmpty)
m.Post("/rename_branch", bindIgnErr(forms.RenameBranchForm{}), context.RepoMustNotBeArchived(), repo.RenameBranchPost)
Review

As far as I have seen, the underscore (_) syntax is uncommon for routes. What about /branches/rename instead? That would then be

				m.Combo("/*").Get(repo.SettingsProtectedBranch).
					Post(bindIgnErr(forms.ProtectBranchForm{}), context.RepoMustNotBeArchived(), repo.SettingsProtectedBranchPost)
				m.Post("/rename", bindIgnErr(forms.RenameBranchForm{}), context.RepoMustNotBeArchived(), repo.RenameBranchPost)
			}, repo.MustBeNotEmpty)

Combined with a change inside the UI dialog.

As far as I have seen, the underscore (`_`) syntax is uncommon for routes. What about `/branches/rename` instead? That would then be ```suggestion m.Combo("/*").Get(repo.SettingsProtectedBranch). Post(bindIgnErr(forms.ProtectBranchForm{}), context.RepoMustNotBeArchived(), repo.SettingsProtectedBranchPost) m.Post("/rename", bindIgnErr(forms.RenameBranchForm{}), context.RepoMustNotBeArchived(), repo.RenameBranchPost) }, repo.MustBeNotEmpty) ``` Combined with a change inside the UI dialog.
6543 commented 2021-10-08 16:22:13 +00:00 (Migrated from github.com)
Review

well I dont mind since it's only a web route - if you like to group settings bit more refactor pulls are welcome :)

well I dont mind since it's only a web route - if you like to group settings bit more refactor pulls are welcome :)
m.Group("/tags", func() {
m.Get("", repo.Tags)

@ -24,3 +24,15 @@ func (f *NewBranchForm) Validate(req *http.Request, errs binding.Errors) binding
ctx := context.GetContext(req)
return middleware.Validate(errs, ctx.Data, f, ctx.Locale)
}
// RenameBranchForm form for rename a branch
type RenameBranchForm struct {
From string `binding:"Required;MaxSize(100);GitRefName"`
To string `binding:"Required;MaxSize(100);GitRefName"`
}
// Validate validates the fields
func (f *RenameBranchForm) Validate(req *http.Request, errs binding.Errors) binding.Errors {
ctx := context.GetContext(req)
return middleware.Validate(errs, ctx.Data, f, ctx.Locale)
}

@ -10,10 +10,49 @@ import (
"code.gitea.io/gitea/models"
"code.gitea.io/gitea/modules/git"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/notification"
repo_module "code.gitea.io/gitea/modules/repository"
pull_service "code.gitea.io/gitea/services/pull"
)
// RenameBranch rename a branch
func RenameBranch(repo *models.Repository, doer *models.User, gitRepo *git.Repository, from, to string) (string, error) {
if from == to {
return "target_exist", nil
}
if gitRepo.IsBranchExist(to) {
return "target_exist", nil
}
if !gitRepo.IsBranchExist(from) {
return "from_not_exist", nil
}
if err := repo.RenameBranch(from, to, func(isDefault bool) error {
err2 := gitRepo.RenameBranch(from, to)
if err2 != nil {
return err2
}
if isDefault {
err2 = gitRepo.SetDefaultBranch(to)
if err2 != nil {
return err2
}
}
return nil
}); err != nil {
return "", err
}
notification.NotifyDeleteRef(doer, repo, "branch", "refs/heads/"+from)
notification.NotifyCreateRef(doer, repo, "branch", "refs/heads/"+to)
return "", nil
}
// enmuerates all branch related errors
var (
ErrBranchIsDefault = errors.New("branch is default")

@ -77,6 +77,28 @@
</div>
</div>
</div>
{{if $.Repository.CanCreateBranch}}
<h4 class="ui top attached header">
{{.i18n.Tr "repo.settings.rename_branch"}}
</h4>
<div class="ui attached segment">
<form class="ui form" action="{{$.Repository.Link}}/settings/rename_branch" method="post">
Review

That is one approach.
I would have guessed that you choose a branch, then a dialog pops up where the base branch is already set, and then you enter the target branch name.
But this approach should also work, but might be a bit less user-friendly.

That is one approach. I would have guessed that you choose a branch, then a dialog pops up where the base branch is already set, and then you enter the target branch name. But this approach should also work, but might be a bit less user-friendly.
{{.CsrfTokenHtml}}
<div class="required field">
<label for="from">{{.i18n.Tr "repo.settings.rename_branch_from"}}</label>
<input id="from" name="from" required>
</div>
<div class="required field {{if .Err_BranchName}}error{{end}}">
<label for="to">{{.i18n.Tr "repo.settings.rename_branch_to"}}</label>
<input id="to" name="to" required>
</div>
<div class="field">
<button class="ui green button">{{$.i18n.Tr "repo.settings.update_settings"}}</button>
</div>
</form>
</div>
{{end}}
{{end}}
</div>
</div>