Add a simple way to rename branch like gh #15870

lunny · 2021-05-14T10:08:29Z

a1012112796 commented

2021-05-14 10:08:29 +00:00

(Migrated from github.com)

update default branch if needed
Update protected branch if needed
Update all not merged pull request base branch name
rename git branch
record this rename work and auto redirect for old branch
on ui

- update default branch if needed - Update protected branch if needed - Update all not merged pull request base branch name - rename git branch - record this rename work and auto redirect for old branch on ui

👍 4

lunny reviewed 2021-05-14 16:03:45 +00:00

models/branches.go

						
				@ -591,0 +592,4 @@

				// RenamedBranch provide renamed branch log

				// will check it when a branch can't be found

				type RenamedBranch struct {

lunny commented

2021-05-14 16:03:45 +00:00

Need a condition ID(repo.ID).

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

6543 (Migrated from github.com) reviewed 2021-05-15 09:59:59 +00:00

models/branches.go

						
				@ -591,0 +613,4 @@

				// RenameBranch rename a branch

				func (repo *Repository) RenameBranch(from, to string, gitAction func(isDefault bool) error) (err error) {

					sess := db.NewSession(db.DefaultContext)

6543 (Migrated from github.com) commented

2021-05-15 09:59:59 +00:00

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"` ```

6543 (Migrated from github.com) reviewed 2021-05-15 10:04:40 +00:00

routers/repo/setting_protected_branch.go

6543 (Migrated from github.com) commented

2021-05-15 10:04:40 +00:00

can we move this into services so API could potentially use it too?

a1012112796 (Migrated from github.com) reviewed 2021-05-15 12:18:39 +00:00

models/branches.go

						
				@ -591,0 +613,4 @@

				// RenameBranch rename a branch

				func (repo *Repository) RenameBranch(from, to string, gitAction func(isDefault bool) error) (err error) {

					sess := db.NewSession(db.DefaultContext)

a1012112796 (Migrated from github.com) commented

2021-05-15 12:18:39 +00:00

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

6543 (Migrated from github.com) reviewed 2021-05-15 14:26:46 +00:00

models/branches.go

						
				@ -591,0 +613,4 @@

				// RenameBranch rename a branch

				func (repo *Repository) RenameBranch(from, to string, gitAction func(isDefault bool) error) (err error) {

					sess := db.NewSession(db.DefaultContext)

6543 (Migrated from github.com) commented

2021-05-15 14:26:46 +00:00

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

lunny reviewed 2021-05-16 01:04:21 +00:00

models/branches.go

						
				@ -591,0 +592,4 @@

				// RenamedBranch provide renamed branch log

				// will check it when a branch can't be found

				type RenamedBranch struct {

lunny commented

2021-05-16 01:04:21 +00:00

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.

lunny reviewed 2021-05-16 01:07:05 +00:00

modules/context/repo.go

lunny commented

2021-05-16 01:07:05 +00:00

I don't think we should put more and more fields in this struct, only those which could be used by all {username}/{reponame} routes should be stored on the struct. You can store them into Data.

I don't think we should put more and more fields in this struct, only those which could be used by all `{username}/{reponame}` routes should be stored on the struct. You can store them into `Data`.

delvh reviewed 2021-09-28 22:19:46 +00:00

integrations/rename_branch_test.go

						
				@ -0,0 +17,4 @@

					// get branch setting page

					session := loginUser(t, "user2")

					req := NewRequest(t, "GET", "/user2/repo1/settings/branches")

					resp := session.MakeRequest(t, req, http.StatusOK)

delvh commented

2021-09-28 21:35:43 +00:00

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.

integrations/rename_branch_test.go

						
				@ -0,0 +26,4 @@

						"to":    "main",

					}

					req = NewRequestWithValues(t, "POST", "/user2/repo1/settings/rename_branch", postData)

					session.MakeRequest(t, req, http.StatusFound)

delvh commented

2021-09-28 21:38:51 +00:00

Will then be

	MakeRequest(t, WithToken(req, token), http.StatusFound)

Will then be ```suggestion MakeRequest(t, WithToken(req, token), http.StatusFound) ```

integrations/rename_branch_test.go

						
				@ -0,0 +30,4 @@

					// check new branch link

					req = NewRequestWithValues(t, "GET", "/user2/repo1/src/branch/main/README.md", postData)

					session.MakeRequest(t, req, http.StatusOK)

delvh commented

2021-09-28 21:39:19 +00:00

And this will be

	MakeRequest(t, WithToken(req, token), http.StatusOK)

And this will be ```suggestion MakeRequest(t, WithToken(req, token), http.StatusOK) ```

integrations/rename_branch_test.go

						
				@ -0,0 +34,4 @@

					// check old branch link

					req = NewRequestWithValues(t, "GET", "/user2/repo1/src/branch/master/README.md", postData)

					resp = session.MakeRequest(t, req, http.StatusFound)

delvh commented

2021-09-28 21:39:37 +00:00

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) ```

models/branches.go

delvh commented

2021-09-28 21:40:47 +00:00

// RenamedBranch provide renamed branch log
// will check it when a branch can't be found

```suggestion // RenamedBranch provide renamed branch log // will check it when a branch can't be found ```

models/branches.go

						
				@ -591,0 +626,4 @@

						_, err = sess.ID(repo.ID).Cols("default_branch").Update(repo)

						if err != nil {

							return err

						}

delvh commented

2021-09-28 21:55:23 +00:00

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:

Have branch protection only applied to branches m.*
-> Branch main is a protected branch
Rename main -> default.
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.

options/locale/locale_en-US.ini

delvh commented

2021-09-28 22:06:18 +00:00

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

```suggestion 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 ```

routers/web/repo/setting_protected_branch.go

						
				@ -288,0 +296,4 @@

						return

					}

					if ctx.HasError() {

delvh commented

2021-09-28 22:10:45 +00:00

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.

templates/repo/settings/branches.tmpl

						
				@ -80,0 +83,4 @@

									{{.i18n.Tr "repo.settings.rename_branch"}}

								</h4>

								<div class="ui attached segment">

									<form class="ui form" action="{{$.Repository.Link}}/settings/rename_branch" method="post">

delvh commented

2021-09-28 22:18:49 +00:00

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.

a1012112796 (Migrated from github.com) reviewed 2021-10-03 04:49:14 +00:00

routers/web/repo/setting_protected_branch.go

						
				@ -288,0 +296,4 @@

						return

					}

					if ctx.HasError() {

a1012112796 (Migrated from github.com) commented

2021-10-03 04:49:14 +00:00

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

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

a1012112796 (Migrated from github.com) reviewed 2021-10-03 04:58:15 +00:00

models/branches.go

						
				@ -591,0 +626,4 @@

						_, err = sess.ID(repo.ID).Cols("default_branch").Update(repo)

						if err != nil {

							return err

						}

a1012112796 (Migrated from github.com) commented

2021-10-03 04:58:15 +00:00

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.

delvh reviewed 2021-10-03 17:47:35 +00:00

models/branches.go

						
				@ -591,0 +626,4 @@

						_, err = sess.ID(repo.ID).Cols("default_branch").Update(repo)

						if err != nil {

							return err

						}

delvh commented

2021-10-03 17:47:35 +00:00

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.

6543 (Migrated from github.com) approved these changes 2021-10-07 23:59:37 +00:00

delvh approved these changes 2021-10-08 07:24:47 +00:00

routers/web/web.go

						
				@ -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)

delvh commented

2021-10-08 07:14:58 +00:00

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 (Migrated from github.com) reviewed 2021-10-08 16:22:13 +00:00

routers/web/web.go

						
				@ -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)