Create new branch from branch selection dropdown #2130

lafriks · 2017-07-09T01:06:09Z

Also fix Branches and Tags tab alignment in branch selection select box

Screenshot:

bkcsoft

and a question

bkcsoft · 2017-07-09T19:33:06Z

models/repo.go

+		return fmt.Errorf("UpdateLocalCopyBranch: %v", err)
+	}
+
+	if err = repo.CheckoutNewBranch(commit, branchName); err != nil {


Are you sure that we need a clone to do this? Since one can create new branches in a bare repo...

Also see https://github.com/go-gitea/gitea/blob/master/models/repo.go#L2341 for reference

It is for creating new branch based on existing branch/tag/commit, so no bare repositories. I was basing it exactly on your pointed out code. Branch can be created directly in real repository without cloning but I just thought that there are reasons why existing branch creation from branch was made this way

Yeah I noticed that after I commented on it. I wonder why it does that 🤔

That is very good question that I would really be interested to get answer, it could be just for that it is also used by file editor but could be also other reasons, hard to tell

bkcsoft · 2017-07-09T19:36:52Z

routers/routes/routes.go

@@ -523,6 +523,10 @@ func RegisterRoutes(m *macaron.Macaron) {
 				return
 			}
 		})
+
+		m.Group("/branches", func() {
+			m.Get("/_new/*", context.RepoRef(), repo.CreateBranch)


Why _new instead of new ? (Except that we'd need to add another reserved name)

Just to clarify, I'm fairly certain that no one would create a branch called "new" anyhow 😆

I was following same naming rules that are for editing files and I don't think we need to add new reserved name, it is completely new group under repo

Shouldn't this be POST? GET requests shouldn't update state.

@ethantkoenig it would be right way but semantic-ui dropdown does not have enough events exposed to do that :(

@lafriks Can't we just add some Javascript/JQuery to make a POST request?

@ethantkoenig not with jquery if only doing with some js timeouts to check if semantic ui dropdown has rendered that link and than binding on click but that is quite dirty that I would want to avoid. Also it could be done using <a onclick with bunch of javascript but that code also seems more like hack than good solution

@lafriks If we use GET, then there will no be CSRF check

@lafriks What do you think of this: https://github.com/lafriks/gitea/pull/1?

bkcsoft · 2017-07-09T19:43:52Z

models/repo.go

@@ -2363,3 +2401,28 @@ func (repo *Repository) CreateNewBranch(doer *User, oldBranchName, branchName st

 	return nil
 }
+
+// CreateNewBranchFromCommit creates a new repository branch
+func (repo *Repository) CreateNewBranchFromCommit(doer *User, commit, branchName string) (err error) {


Please put this function in models/repo_branch.go instead. Let's not add more to the God File unless we have to 😄

should I also move CreateNewBranch function that was already there to repo_branch.go? As these functions are doing very similar things I just did not want them to be in different files

@lafriks Yes please 🙂

Maybe if possible make it the same function (renaming oldBranch to baseRef or similar)

They have different functionality a bit and different checks and some code is called in different order, so making all one function would overcomplicate it

ethantkoenig · 2017-07-09T21:50:27Z

integrations/repo_branch_test.go

+func testCreateBranch(t *testing.T, session *TestSession, user, repo, oldRefName, newBranchName string, expectedStatus int) string {
+	req := NewRequest(t, "GET", path.Join(user, repo, "branches/_new", oldRefName)+"?name="+url.QueryEscape(newBranchName))
+	resp := session.MakeRequest(t, req, expectedStatus)
+	assert.EqualValues(t, expectedStatus, resp.HeaderCode)


This line isn't necessary, session.MakeRequest(..) already checks

Will fix, I did write this code before MakeRequest was changed and forgot to remove that line

ethantkoenig · 2017-07-09T21:55:23Z

routers/routes/routes.go

@@ -523,6 +523,10 @@ func RegisterRoutes(m *macaron.Macaron) {
 				return
 			}
 		})
+
+		m.Group("/branches", func() {
+			m.Get("/_new/*", context.RepoRef(), repo.CreateBranch)


Shouldn't this be POST? GET requests shouldn't update state.

ethantkoenig · 2017-07-09T22:05:33Z

templates/repo/branch_dropdown.tmpl

@@ -1,6 +1,6 @@
 <div class="fitted item choose reference">
-	<div class="ui floating filter dropdown" data-no-results="{{.i18n.Tr "repo.pulls.no_results"}}">
-			<div class="ui basic small button">
+	<div class="ui floating filter dropdown" data-can-create-branch="{{if .Repository.CanCreateBranch}}true{{else}}false{{end}}" data-no-results="{{.i18n.Tr "repo.pulls.no_results"}}">


Can just do data-can-create-branch="{{.Repository.CanCreateBranch}}"

Did not work for me...

lafriks · 2017-07-12T21:31:34Z

@bkcsoft, @ethantkoenig fixed with all your suggestions, please review

bkcsoft

1 question, 1 suggestion. Otherwise LGTM

bkcsoft · 2017-07-13T08:35:48Z

routers/repo/branch.go

+		return
+	}
+
+	if len(form.NewBranchName) > 100 {


Where is this limit taken from? Why is it not a constant? (in go-gitea/git) :)

@bkcsoft it is limit used also in other places in gitea code where branch name can be entered (editor for example)

No clue 😆 Hence why I'm asking 😉

@bkcsoft @ethantkoenig suggested that limit and later I found it also in other places in code, so that's why it is so :)

I suggested it because I found it in other places 😆

And I'm suggesting making that a global constant variable in code.gitea.io/git

bkcsoft · 2017-07-13T08:36:31Z

routers/repo/branch.go

+	}
+
+	if _, err := ctx.Repo.GitRepo.GetTag(form.NewBranchName); err == nil {
+		ctx.Flash.Error(ctx.Tr("repo.branch.tag_already_exists", form.NewBranchName))


This should probably be repo.branch.tag_collision

ethantkoenig · 2017-07-13T17:19:52Z

As things currently are, I can't create a branch called "mast", since searing for "mast" will match the master branch (so the "Create new branch" button won't show). Maybe the button should always show, but be disabled for exact matches?

lafriks · 2017-07-13T17:23:33Z

You are right but with this dropdown I don't know if it is possible. I will check if I can work around somehow

lafriks · 2017-07-14T23:55:51Z

@ethantkoenig @bkcsoft I rewrote javascript side to allow creating branches also with names that partially match existing branches as suggested, please review

lafriks · 2017-07-16T00:28:25Z

As I could not solve small remaining issue that no data found would still show up sometimes together with create new branch item, I did complete JavaScript branch/tag dropdown rewrite to not use semantic ui js but VueJS. As a plus works a lot faster and in future more extendable.

codecov-io · 2017-09-23T18:09:30Z

Codecov Report

Merging #2130 into master will decrease coverage by 0.11%.
The diff coverage is 6.89%.

@@            Coverage Diff             @@
##           master    #2130      +/-   ##
==========================================
- Coverage    27.1%   26.98%   -0.12%     
==========================================
  Files          87       87              
  Lines       17191    17283      +92     
==========================================
+ Hits         4659     4664       +5     
- Misses      11852    11939      +87     
  Partials      680      680

Impacted Files	Coverage Δ
models/repo.go	`13.12% <ø> (+0.16%)`	⬆️
models/repo_branch.go	`8.73% <0%> (-24.61%)`	⬇️
models/error.go	`18.93% <0%> (-1%)`	⬇️
modules/validation/binding.go	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c25303b...3d0a597. Read the comment docs.

lafriks · 2017-09-25T00:22:58Z

@ethantkoenig need your approval

ethantkoenig

~~It's not working when I try it locally (eae7ad5); the link to create a new branch never appears.~~ Never mind

ethantkoenig · 2017-09-25T01:33:44Z

models/repo_branch.go

+// This function checks out target commit by default, it is safe to assume subsequent
+// operations are operating against target commit when caller has confidence for no race condition.
+func UpdateLocalCopyToCommit(repoPath, localPath, commit string) error {
+	if !com.IsExist(localPath) {


I believe this is duplicated code; this if/else can be replaced by

UpdateLocalCopyBranch(repoPath, localPath, repo.DefaultBranch)

@ethantkoenig no it can not as they are different and calls methods in different order. Code from UpdateLocalCopyBranch will fail in some cases if not updated in correct order (at least for creating new branch from commit/tag)

ethantkoenig · 2017-09-25T01:35:34Z

models/repo_branch.go

+// It creates a new clone if local copy does not exist.
+// This function checks out target commit by default, it is safe to assume subsequent
+// operations are operating against target commit when caller has confidence for no race condition.
+func UpdateLocalCopyToCommit(repoPath, localPath, commit string) error {


Since the function should only be called when the repo's "mutex" has been acquired, should we make it private? Similarly, for repo.UpdateLocalCopyToCommit?

ethantkoenig · 2017-09-25T01:39:47Z

routers/repo/branch.go

+			ctx.Flash.Error(ctx.Tr("repo.branch.branch_already_exists", branch.Name))
+			ctx.Redirect(ctx.Repo.RepoLink + "/src/" + ctx.Repo.BranchName)
+			return
+		} else if (len(branch.Name) < len(form.NewBranchName) && branch.Name+"/" == form.NewBranchName[0:len(branch.Name)+1]) ||


Could we move this condition to a helper function, something like BranchNamesConflict(name1, name2)? It could be useful in other places, and would make the code more readable IMO.

@ethantkoenig I have no idea to where to move this functions

models.(*Repository).BranchNameConflict(form.NewBranchName) perhaps?

lafriks · 2017-09-25T03:44:05Z

@ethantkoenig from what it looks like from screenshot you are not logged in. For branch creation to show up you must be logged in and have write access to that repository

ethantkoenig · 2017-09-25T04:21:25Z

@lafriks Wow, not sure how I missed that 🤦‍♂️. Links appearing as they should.

webjoel · 2017-10-11T20:55:50Z

index.css gets created by make stylesheets from less file. So shouldn't matter.
you have to call that for yourself.

can someone finish the job, I'm waiting for this feature :)

lunny · 2017-10-15T02:29:52Z

LGTM

bkcsoft · 2017-10-15T10:19:49Z

modules/validation/binding.go

 				errs.Add([]string{name}, ErrGitRefName, "GitRefName")
 				return false, errs
 			}
+			parts := strings.Split(str, "/")


How about using a regex instead of doing similar checks twice? https://stackoverflow.com/a/12093994

That won't work in golang as its regex does not support all required features. I tried that exact stackoverflow topic regex first when implementing this

bkcsoft · 2017-10-15T10:21:56Z

routers/repo/branch.go

+			ctx.Flash.Error(ctx.Tr("repo.branch.branch_already_exists", branch.Name))
+			ctx.Redirect(ctx.Repo.RepoLink + "/src/" + ctx.Repo.BranchName)
+			return
+		} else if (len(branch.Name) < len(form.NewBranchName) && branch.Name+"/" == form.NewBranchName[0:len(branch.Name)+1]) ||


models.(*Repository).BranchNameConflict(form.NewBranchName) perhaps?

lafriks · 2017-10-15T14:01:09Z

@ethantkoenig @bkcsoft moved branch name validation to model

bkcsoft · 2017-10-15T18:17:26Z

models/repo_branch.go

+	if err := repo.CheckBranchName(branchName); err != nil {
+		return err
+	}
+
 	repoWorkingPool.CheckIn(com.ToStr(repo.ID))


Do this before checking the branch-name, otherwise you'll end up with race conditions

bkcsoft · 2017-10-15T18:17:38Z

models/repo_branch.go

+	if err := repo.CheckBranchName(branchName); err != nil {
+		return err
+	}
+
 	repoWorkingPool.CheckIn(com.ToStr(repo.ID))


@bkcsoft fixed both

bkcsoft · 2017-10-15T18:19:24Z

routers/repo/branch.go

-	} else {
-		err = ctx.Repo.Repository.CreateNewBranchFromCommit(ctx.User, ctx.Repo.BranchName, form.NewBranchName)
-	}
-	if err != nil {
 		ctx.Handle(500, "CreateNewBranch", err)


so 500 error if the name is invalid?

No there is L56-L73 special error type checks before this - this one is for unexpected errors

bkcsoft · 2017-10-15T18:31:16Z

models/repo_branch.go

+		return err
+	}
+
+	for _, branch := range branches {


Instead of doing this, how about something like this that scales to thousands of branches?

if _, err := gitRepo.GetBranch(name); err != nil { return ErrConflict(name) } if strings.Contains(name, "/") { for index := strings.LastIndex(name, "/"); index > 0; index := strings.LastIndex(name, "/") { if conflict, err := gitRepo.GetBranch(name[:index]); err != nil { return ErrConflict(name[:index]) } } }

It is not the same (if name is feat and there already exists branch feat/test) this code won't return error

One option is to just try the git branch ... command, and see if it succeeds or not. I assume that internally git has an efficient way to check for naming conflicts.

I think that gitRepo.CreateBranch will throw a collision error no?

But then CheckBranchName will only provide part of validation (as I understand that that was an initial request to create it at first place to reuse it in other places later). Also currently GetBranches is called on every repo page load (in ctx RepoAssignment) so it's usage here anyway won't be that big deal

lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jul 9, 2017

lunny added this to the 1.3.0 milestone Jul 9, 2017

lafriks force-pushed the feature/create_new_branch branch from 12ac6e2 to 25d4619 Compare July 9, 2017 11:01

lafriks changed the title ~~Create new branch from branch choose select~~ Create new branch from branch selection dropdown Jul 9, 2017

lafriks force-pushed the feature/create_new_branch branch 2 times, most recently from 486a881 to 8646ee5 Compare July 9, 2017 12:15

bkcsoft suggested changes Jul 9, 2017

View reviewed changes

ethantkoenig suggested changes Jul 9, 2017

View reviewed changes

lafriks force-pushed the feature/create_new_branch branch 5 times, most recently from f29ceae to 29997f0 Compare July 12, 2017 21:28

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 12, 2017

lafriks force-pushed the feature/create_new_branch branch 2 times, most recently from a8d9b11 to 835cc95 Compare July 12, 2017 21:47

bkcsoft reviewed Jul 13, 2017

View reviewed changes

lafriks force-pushed the feature/create_new_branch branch from 835cc95 to 3e972ad Compare July 13, 2017 09:28

lafriks force-pushed the feature/create_new_branch branch from e82b933 to f2d291a Compare July 14, 2017 23:52

lafriks force-pushed the feature/create_new_branch branch 3 times, most recently from 1770bae to c21cf76 Compare July 16, 2017 00:22

lafriks force-pushed the feature/create_new_branch branch 3 times, most recently from 650e35b to fd7e742 Compare July 18, 2017 10:02

lafriks added pr/wip This PR is not ready for review and removed pr/wip This PR is not ready for review labels Sep 23, 2017

lafriks force-pushed the feature/create_new_branch branch from 213b08e to 7423bbd Compare September 25, 2017 00:21

lafriks force-pushed the feature/create_new_branch branch from 7423bbd to eae7ad5 Compare September 25, 2017 00:37

ethantkoenig suggested changes Sep 25, 2017

View reviewed changes

lafriks force-pushed the feature/create_new_branch branch 2 times, most recently from dbb75f0 to f7ebb60 Compare October 15, 2017 02:26

tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 15, 2017

lafriks force-pushed the feature/create_new_branch branch from f7ebb60 to bdd64d4 Compare October 15, 2017 02:41

bkcsoft reviewed Oct 15, 2017

View reviewed changes

ethantkoenig approved these changes Oct 15, 2017

View reviewed changes

bkcsoft suggested changes Oct 15, 2017

View reviewed changes

bkcsoft reviewed Oct 15, 2017

View reviewed changes

Gitea and others added 4 commits October 15, 2017 21:40

Create new branch from branch selection dropdown and rewrite it to VueJS

571cba5

Make updateLocalCopyToCommit as not exported

b4cca45

Move branch name validation to model

caccfb4

Fix possible race condition

3d0a597

lafriks force-pushed the feature/create_new_branch branch from 573a7d7 to 3d0a597 Compare October 15, 2017 18:44

bkcsoft approved these changes Oct 15, 2017

View reviewed changes

lafriks merged commit f3833b7 into go-gitea:master Oct 15, 2017

lafriks deleted the feature/create_new_branch branch October 15, 2017 19:59

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Create new branch from branch selection dropdown #2130

Create new branch from branch selection dropdown #2130

Conversation

lafriks commented Jul 9, 2017 • edited Loading

bkcsoft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lafriks Jul 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lafriks commented Jul 12, 2017

bkcsoft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkcsoft Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethantkoenig commented Jul 13, 2017

lafriks commented Jul 13, 2017

lafriks commented Jul 14, 2017

lafriks commented Jul 16, 2017 • edited Loading

codecov-io commented Sep 23, 2017 • edited Loading

Codecov Report

lafriks commented Sep 25, 2017

ethantkoenig left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lafriks commented Sep 25, 2017

ethantkoenig commented Sep 25, 2017

webjoel commented Oct 11, 2017

lunny commented Oct 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lafriks commented Oct 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lafriks commented Jul 9, 2017 •

edited

Loading

lafriks Jul 9, 2017 •

edited

Loading

bkcsoft Aug 28, 2017 •

edited

Loading

lafriks commented Jul 16, 2017 •

edited

Loading

codecov-io commented Sep 23, 2017 •

edited

Loading

ethantkoenig left a comment •

edited

Loading