From 2625193a4820a89355ca1b8b7c910f0f4243baab Mon Sep 17 00:00:00 2001 From: Unknwon Date: Mon, 15 Aug 2016 22:20:55 -0700 Subject: [PATCH] models/repo_editor: improve code quality --- .bra.toml | 2 +- .gopmfile | 2 +- glide.lock | 2 +- models/action.go | 1 + models/issue.go | 4 +-- models/pull.go | 2 +- models/repo.go | 10 +++---- models/repo_editor.go | 61 ++++++++++++++++++++++-------------------- routers/repo/editor.go | 2 +- 9 files changed, 45 insertions(+), 41 deletions(-) diff --git a/.bra.toml b/.bra.toml index ed0fbd225..3fd4e0c68 100644 --- a/.bra.toml +++ b/.bra.toml @@ -1,6 +1,6 @@ [run] init_cmds = [ - ["make", "build-dev", "TAGS=sqlite"], + ["make", "build-dev"], ["./gogs", "web"] ] watch_all = true diff --git a/.gopmfile b/.gopmfile index e5e077459..590eab7b5 100644 --- a/.gopmfile +++ b/.gopmfile @@ -18,7 +18,7 @@ github.com/go-xorm/core = commit:5bf745d github.com/go-xorm/xorm = commit:c6c7056 github.com/gogits/chardet = commit:2404f77 github.com/gogits/cron = commit:7f3990a -github.com/gogits/git-module = commit:2a820b5 +github.com/gogits/git-module = commit:f78bf3b github.com/gogits/go-gogs-client = commit:e363d3f github.com/issue9/identicon = commit:d36b545 github.com/jaytaylor/html2text = commit:52d9b78 diff --git a/glide.lock b/glide.lock index a06d9722d..7dae88525 100644 --- a/glide.lock +++ b/glide.lock @@ -41,7 +41,7 @@ imports: - name: github.com/gogits/cron version: 7f3990acf1833faa5ebd0e86f0a4c72a4b5eba3c - name: github.com/gogits/git-module - version: 2a820b5471795de4c8b993e15b0ed08155090c6a + version: f78bf3bf703cb3eb0e85a9475d26826939feda4f - name: github.com/gogits/go-gogs-client version: e363d3ff8f70d0fe813324eedf228684af41c29c - name: github.com/issue9/identicon diff --git a/models/action.go b/models/action.go index 13085ace7..f28aef9b5 100644 --- a/models/action.go +++ b/models/action.go @@ -434,6 +434,7 @@ func updateIssuesCommit(u *User, repo *Repository, repoUserName, repoName string } // CommitRepoAction adds new action for committing repository. +// TODO: use CommitRepoActionOptions func CommitRepoAction( userID, repoUserID int64, userName, actEmail string, diff --git a/models/issue.go b/models/issue.go index bd236c7a8..9b9dedb69 100644 --- a/models/issue.go +++ b/models/issue.go @@ -587,7 +587,7 @@ type NewIssueOptions struct { IsPull bool } -func newIssue(e *xorm.Session, opts *NewIssueOptions) (err error) { +func newIssue(e *xorm.Session, opts NewIssueOptions) (err error) { opts.Issue.Title = strings.TrimSpace(opts.Issue.Title) opts.Issue.Index = opts.Repo.NextIssueIndex() @@ -669,7 +669,7 @@ func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, uuids []string) return err } - if err = newIssue(sess, &NewIssueOptions{ + if err = newIssue(sess, NewIssueOptions{ Repo: repo, Issue: issue, LableIDs: labelIDs, diff --git a/models/pull.go b/models/pull.go index 09c20df82..fb967cd30 100644 --- a/models/pull.go +++ b/models/pull.go @@ -388,7 +388,7 @@ func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []str return err } - if err = newIssue(sess, &NewIssueOptions{ + if err = newIssue(sess, NewIssueOptions{ Repo: repo, Issue: pull, LableIDs: labelIDs, diff --git a/models/repo.go b/models/repo.go index 2eb63f54c..6c427f92e 100644 --- a/models/repo.go +++ b/models/repo.go @@ -461,26 +461,26 @@ func UpdateLocalCopyBranch(repoPath, localPath, branch string) error { Timeout: time.Duration(setting.Git.Timeout.Clone) * time.Second, Branch: branch, }); err != nil { - return fmt.Errorf("Clone: %v", err) + return fmt.Errorf("git clone %s: %v", branch, err) } } else { if err := git.Checkout(localPath, git.CheckoutOptions{ Branch: branch, }); err != nil { - return fmt.Errorf("Checkout: %v", err) + return fmt.Errorf("git checkout %s: %v", branch, err) } if err := git.Pull(localPath, git.PullRemoteOptions{ + Timeout: time.Duration(setting.Git.Timeout.Pull) * time.Second, Remote: "origin", Branch: branch, - Timeout: time.Duration(setting.Git.Timeout.Pull) * time.Second, }); err != nil { - return fmt.Errorf("Pull: %v", err) + return fmt.Errorf("git pull origin %s: %v", branch, err) } } return nil } -// UpdateLocalCopy makes sure the branch of local copy of repository is up-to-date. +// UpdateLocalCopyBranch makes sure local copy of repository in given branch is up-to-date. func (repo *Repository) UpdateLocalCopyBranch(branch string) error { return UpdateLocalCopyBranch(repo.RepoPath(), repo.LocalCopyPath(), branch) } diff --git a/models/repo_editor.go b/models/repo_editor.go index 070ffc374..740abfee1 100644 --- a/models/repo_editor.go +++ b/models/repo_editor.go @@ -29,8 +29,8 @@ import ( // /_______ /\____ | |__||__| \___ / |__|____/\___ > // \/ \/ \/ \/ -// discardLocalRepoBranchChanges discards local commits of given branch -// to make sure it is even to remote branch when local copy exists. +// discardLocalRepoBranchChanges discards local commits/changes of +// given branch to make sure it is even to remote branch. func discardLocalRepoBranchChanges(localPath, branch string) error { if !com.IsExist(localPath) { return nil @@ -39,8 +39,10 @@ func discardLocalRepoBranchChanges(localPath, branch string) error { if !git.IsBranchExist(localPath, branch) { return nil } - if err := git.ResetHEAD(localPath, true, "origin/"+branch); err != nil { - return fmt.Errorf("ResetHEAD: %v", err) + + refName := "origin/" + branch + if err := git.ResetHEAD(localPath, true, refName); err != nil { + return fmt.Errorf("git reset --hard %s: %v", refName, err) } return nil } @@ -49,23 +51,18 @@ func (repo *Repository) DiscardLocalRepoBranchChanges(branch string) error { return discardLocalRepoBranchChanges(repo.LocalCopyPath(), branch) } +// checkoutNewBranch checks out to a new branch from the a branch name. func checkoutNewBranch(repoPath, localPath, oldBranch, newBranch string) error { - if !com.IsExist(localPath) { - if err := UpdateLocalCopyBranch(repoPath, localPath, oldBranch); err != nil { - return err - } - } if err := git.Checkout(localPath, git.CheckoutOptions{ + Timeout: time.Duration(setting.Git.Timeout.Pull) * time.Second, Branch: newBranch, OldBranch: oldBranch, - Timeout: time.Duration(setting.Git.Timeout.Pull) * time.Second, }); err != nil { - return fmt.Errorf("Checkout: %v", err) + return fmt.Errorf("git checkout -b %s %s: %v", newBranch, oldBranch, err) } return nil } -// CheckoutNewBranch checks out a new branch from the given branch name. func (repo *Repository) CheckoutNewBranch(oldBranch, newBranch string) error { return checkoutNewBranch(repo.RepoPath(), repo.LocalCopyPath(), oldBranch, newBranch) } @@ -81,8 +78,8 @@ type UpdateRepoFileOptions struct { IsNewFile bool } -// updateRepoFile adds new file to repository. -func (repo *Repository) UpdateRepoFile(doer *User, opts *UpdateRepoFileOptions) (err error) { +// UpdateRepoFile adds or updates a file in repository. +func (repo *Repository) UpdateRepoFile(doer *User, opts UpdateRepoFileOptions) (err error) { repoWorkingPool.CheckIn(com.ToStr(repo.ID)) defer repoWorkingPool.CheckOut(com.ToStr(repo.ID)) @@ -98,9 +95,6 @@ func (repo *Repository) UpdateRepoFile(doer *User, opts *UpdateRepoFileOptions) } } - localPath := repo.LocalCopyPath() - filePath := path.Join(localPath, opts.NewTreeName) - if len(opts.Message) == 0 { if opts.IsNewFile { opts.Message = "Add '" + opts.NewTreeName + "'" @@ -109,16 +103,21 @@ func (repo *Repository) UpdateRepoFile(doer *User, opts *UpdateRepoFileOptions) } } + localPath := repo.LocalCopyPath() + filePath := path.Join(localPath, opts.NewTreeName) os.MkdirAll(path.Dir(filePath), os.ModePerm) - // If new file, make sure it doesn't exist; if old file, move if file name change. + // If it's meant to be a new file, make sure it doesn't exist. if opts.IsNewFile { if com.IsExist(filePath) { return ErrRepoFileAlreadyExist{filePath} } - } else if len(opts.OldTreeName) > 0 && len(opts.NewTreeName) > 0 && opts.NewTreeName != opts.OldTreeName { + } + + // If update a file, move if file name change. + if len(opts.OldTreeName) > 0 && len(opts.NewTreeName) > 0 && opts.OldTreeName != opts.NewTreeName { if err = git.MoveFile(localPath, opts.OldTreeName, opts.NewTreeName); err != nil { - return fmt.Errorf("MoveFile [old_tree_name: %s, new_tree_name: %s]: %v", opts.OldTreeName, opts.NewTreeName, err) + return fmt.Errorf("git mv %s %s: %v", opts.OldTreeName, opts.NewTreeName, err) } } @@ -127,11 +126,14 @@ func (repo *Repository) UpdateRepoFile(doer *User, opts *UpdateRepoFileOptions) } if err = git.AddChanges(localPath, true); err != nil { - return fmt.Errorf("AddChanges: %v", err) - } else if err = git.CommitChanges(localPath, opts.Message, doer.NewGitSig()); err != nil { - return fmt.Errorf("CommitChanges: %v", err) + return fmt.Errorf("git add --all: %v", err) + } + + signaure := doer.NewGitSig() + if err = git.CommitChanges(localPath, opts.Message, signaure); err != nil { + return fmt.Errorf("git commit -m %s --author='%s <%s>': %v", opts.Message, signaure.Name, signaure.Email, err) } else if err = git.Push(localPath, "origin", opts.NewBranch); err != nil { - return fmt.Errorf("Push: %v", err) + return fmt.Errorf("git push origin %s: %v", opts.NewBranch, err) } gitRepo, err := git.OpenRepository(repo.RepoPath()) @@ -145,13 +147,14 @@ func (repo *Repository) UpdateRepoFile(doer *User, opts *UpdateRepoFileOptions) return nil } + // Simulate push event. pushCommits := &PushCommits{ Len: 1, Commits: []*PushCommit{CommitToPushCommit(commit)}, } oldCommitID := opts.LastCommitID if opts.NewBranch != opts.OldBranch { - oldCommitID = "0000000000000000000000000000000000000000" // New Branch so we use all 0s + oldCommitID = git.EMPTY_SHA } if err := CommitRepoAction(doer.ID, repo.MustOwner().ID, doer.Name, doer.Email, repo.ID, repo.MustOwner().Name, repo.Name, git.BRANCH_PREFIX+opts.NewBranch, @@ -164,19 +167,19 @@ func (repo *Repository) UpdateRepoFile(doer *User, opts *UpdateRepoFileOptions) return nil } +// GetDiffPreview produces and returns diff result of a file which is not yet committed. func (repo *Repository) GetDiffPreview(branch, treeName, content string) (diff *Diff, err error) { repoWorkingPool.CheckIn(com.ToStr(repo.ID)) defer repoWorkingPool.CheckOut(com.ToStr(repo.ID)) if err = repo.DiscardLocalRepoBranchChanges(branch); err != nil { - return nil, fmt.Errorf("discardLocalRepoChanges: %s - %v", branch, err) + return nil, fmt.Errorf("DiscardLocalRepoBranchChanges [branch: %s]: %v", branch, err) } else if err = repo.UpdateLocalCopyBranch(branch); err != nil { - return nil, fmt.Errorf("UpdateLocalCopyBranch: %s - %v", branch, err) + return nil, fmt.Errorf("UpdateLocalCopyBranch [branch: %s]: %v", branch, err) } localPath := repo.LocalCopyPath() filePath := path.Join(localPath, treeName) - os.MkdirAll(filepath.Dir(filePath), os.ModePerm) if err = ioutil.WriteFile(filePath, []byte(content), 0666); err != nil { return nil, fmt.Errorf("WriteFile: %v", err) @@ -195,7 +198,7 @@ func (repo *Repository) GetDiffPreview(branch, treeName, content string) (diff * return nil, fmt.Errorf("Start: %v", err) } - pid := process.Add(fmt.Sprintf("GetDiffRange [repo_path: %s]", repo.RepoPath()), cmd) + pid := process.Add(fmt.Sprintf("GetDiffPreview [repo_path: %s]", repo.RepoPath()), cmd) defer process.Remove(pid) diff, err = ParsePatch(setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, stdout) diff --git a/routers/repo/editor.go b/routers/repo/editor.go index ed1e9a136..8b49a9e47 100644 --- a/routers/repo/editor.go +++ b/routers/repo/editor.go @@ -263,7 +263,7 @@ func editFilePost(ctx *context.Context, form auth.EditRepoFileForm, isNewFile bo message += "\n\n" + form.CommitMessage } - if err := ctx.Repo.Repository.UpdateRepoFile(ctx.User, &models.UpdateRepoFileOptions{ + if err := ctx.Repo.Repository.UpdateRepoFile(ctx.User, models.UpdateRepoFileOptions{ LastCommitID: lastCommit, OldBranch: oldBranchName, NewBranch: branchName,