From 708f87209c89e5aa3b8c66e6bd435473034d862b Mon Sep 17 00:00:00 2001 From: Richard Mahn Date: Mon, 15 Jul 2019 17:34:45 -0400 Subject: [PATCH 1/7] Fixes #7474 - Handles all redirects for Web UI File CRUD --- options/locale/locale_en-US.ini | 1 + public/js/index.js | 1 + routers/repo/editor.go | 70 +++++++++++++++++++++++--- templates/repo/editor/commit_form.tmpl | 8 +-- 4 files changed, 70 insertions(+), 10 deletions(-) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 6ef1277c62543..969a0953a2894 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -696,6 +696,7 @@ editor.delete = Delete '%s' editor.commit_message_desc = Add an optional extended description… editor.commit_directly_to_this_branch = Commit directly to the %s branch. editor.create_new_branch = Create a new branch for this commit and start a pull request. +editor.propose_file_change = Propose file change editor.new_branch_name_desc = New branch name… editor.cancel = Cancel editor.filename_cannot_be_empty = The filename cannot be empty. diff --git a/public/js/index.js b/public/js/index.js index 39a283c431f3d..d588c72658c3c 100644 --- a/public/js/index.js +++ b/public/js/index.js @@ -1308,6 +1308,7 @@ function initEditor() { $('.quick-pull-branch-name').hide(); $('.quick-pull-branch-name input').prop('required',false); } + $('#commit-button').text($(this).attr('button_text')); }); var $editFilename = $("#file-name"); diff --git a/routers/repo/editor.go b/routers/repo/editor.go index f3327017e5f36..a03b60a5dafdb 100644 --- a/routers/repo/editor.go +++ b/routers/repo/editor.go @@ -8,6 +8,7 @@ import ( "fmt" "io/ioutil" "path" + "path/filepath" "strings" "code.gitea.io/gitea/models" @@ -137,7 +138,7 @@ func editFile(ctx *context.Context, isNewFile bool) { } else { ctx.Data["commit_choice"] = frmCommitChoiceNewBranch } - ctx.Data["new_branch_name"] = "" + ctx.Data["new_branch_name"] = GetUniquePatchBranchName(ctx) ctx.Data["last_commit"] = ctx.Repo.CommitID ctx.Data["MarkdownFileExts"] = strings.Join(setting.Markdown.FileExtensions, ",") ctx.Data["LineWrapExtensions"] = strings.Join(setting.Repository.Editor.LineWrapExtensions, ",") @@ -266,6 +267,10 @@ func editFilePost(ctx *context.Context, form auth.EditRepoFileForm, isNewFile bo } else { ctx.RenderWithErr(ctx.Tr("repo.editor.fail_to_update_file", form.TreePath, err), tplEditFile, &form) } + } + + if form.CommitChoice == frmCommitChoiceNewBranch { + ctx.Redirect(ctx.Repo.RepoLink + "/compare/" + ctx.Repo.BranchName + "..." + form.NewBranchName) } else { ctx.Redirect(ctx.Repo.RepoLink + "/src/branch/" + util.PathEscapeSegments(branchName) + "/" + util.PathEscapeSegments(form.TreePath)) } @@ -335,7 +340,7 @@ func DeleteFile(ctx *context.Context) { } else { ctx.Data["commit_choice"] = frmCommitChoiceNewBranch } - ctx.Data["new_branch_name"] = "" + ctx.Data["new_branch_name"] = GetUniquePatchBranchName(ctx) ctx.HTML(200, tplDeleteFile) } @@ -426,9 +431,23 @@ func DeleteFilePost(ctx *context.Context, form auth.DeleteRepoFileForm) { } else { ctx.ServerError("DeleteRepoFile", err) } + } + + ctx.Flash.Success(ctx.Tr("repo.editor.file_delete_success", ctx.Repo.TreePath)) + if form.CommitChoice == frmCommitChoiceNewBranch { + ctx.Redirect(ctx.Repo.RepoLink + "/compare/" + ctx.Repo.BranchName + "..." + form.NewBranchName) } else { - ctx.Flash.Success(ctx.Tr("repo.editor.file_delete_success", ctx.Repo.TreePath)) - ctx.Redirect(ctx.Repo.RepoLink + "/src/branch/" + util.PathEscapeSegments(branchName)) + treePath := filepath.Dir(ctx.Repo.TreePath) + if len(treePath) > 0 && treePath != "." { + // Need to get the latest commit since it changed + commit, err := ctx.Repo.GitRepo.GetBranchCommit(ctx.Repo.BranchName) + if err == nil && commit != nil { + treePath = GetClosestParentWithFiles(treePath, commit) + } else { + treePath = "" + } + } + ctx.Redirect(ctx.Repo.RepoLink + "/src/branch/" + util.PathEscapeSegments(branchName) + "/" + util.PathEscapeSegments(treePath)) } } @@ -467,7 +486,7 @@ func UploadFile(ctx *context.Context) { } else { ctx.Data["commit_choice"] = frmCommitChoiceNewBranch } - ctx.Data["new_branch_name"] = "" + ctx.Data["new_branch_name"] = GetUniquePatchBranchName(ctx) ctx.HTML(200, tplUploadFile) } @@ -565,7 +584,11 @@ func UploadFilePost(ctx *context.Context, form auth.UploadRepoFileForm) { return } - ctx.Redirect(ctx.Repo.RepoLink + "/src/branch/" + util.PathEscapeSegments(branchName) + "/" + util.PathEscapeSegments(form.TreePath)) + if form.CommitChoice == frmCommitChoiceNewBranch { + ctx.Redirect(ctx.Repo.RepoLink + "/compare/" + ctx.Repo.BranchName + "..." + form.NewBranchName) + } else { + ctx.Redirect(ctx.Repo.RepoLink + "/src/branch/" + util.PathEscapeSegments(branchName) + "/" + util.PathEscapeSegments(form.TreePath)) + } } func cleanUploadFileName(name string) string { @@ -636,3 +659,38 @@ func RemoveUploadFileFromServer(ctx *context.Context, form auth.RemoveUploadFile log.Trace("Upload file removed: %s", form.File) ctx.Status(204) } + +// GetUniquePatchBranchName Gets a unquie branch name for a new patch branch +// It will be in the form of -patch- where is the first branch of this format +// that doesn't already exist +func GetUniquePatchBranchName(ctx *context.Context) string { + prefix := ctx.User.LowerName + "-patch-" + for i := 1; i <= 1000; i++ { + branchName := fmt.Sprintf("%s%d", prefix, i) + if _, err := ctx.Repo.Repository.GetBranch(branchName); err != nil { + if git.IsErrBranchNotExist(err) { + return branchName + } else { + return "" + } + } + } + return "" +} + +// GetClosestParentWithFiles Recursively gets the path of parent in a tree that has files (used when file in a tree is +// deleted). Returns "" for the root if no parents other than the root have files +func GetClosestParentWithFiles(treePath string, commit *git.Commit) string { + if len(treePath) == 0 || treePath == "." { + return "" + } + // see if the tree has entries + if tree, err := commit.SubTree(treePath); err != nil { + // failed to get tree, going up a dir + return GetClosestParentWithFiles(filepath.Dir(treePath), commit) + } else if entries, err := tree.ListEntries(); err != nil || len(entries) == 0 { + // no files in this dir, going up a dir + return GetClosestParentWithFiles(filepath.Dir(treePath), commit) + } + return treePath +} diff --git a/templates/repo/editor/commit_form.tmpl b/templates/repo/editor/commit_form.tmpl index 86749623eba3d..d22cd07b03fb2 100644 --- a/templates/repo/editor/commit_form.tmpl +++ b/templates/repo/editor/commit_form.tmpl @@ -11,7 +11,7 @@
- +
- +
- {{.i18n.Tr "repo.editor.cancel"}}
From 9c44e7076c399f0ce26bf482cd02f2e648284d69 Mon Sep 17 00:00:00 2001 From: Richard Mahn Date: Mon, 15 Jul 2019 17:40:05 -0400 Subject: [PATCH 2/7] Fixes lint errors --- routers/repo/editor.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/routers/repo/editor.go b/routers/repo/editor.go index a03b60a5dafdb..5411f842473c9 100644 --- a/routers/repo/editor.go +++ b/routers/repo/editor.go @@ -670,9 +670,8 @@ func GetUniquePatchBranchName(ctx *context.Context) string { if _, err := ctx.Repo.Repository.GetBranch(branchName); err != nil { if git.IsErrBranchNotExist(err) { return branchName - } else { - return "" } + return "" } } return "" From 2760d0ea8280de438227627a5077565eaa2e77f8 Mon Sep 17 00:00:00 2001 From: Richard Mahn Date: Mon, 15 Jul 2019 22:46:21 -0400 Subject: [PATCH 3/7] Typo fix --- routers/repo/editor.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/repo/editor.go b/routers/repo/editor.go index 5411f842473c9..9f4da2c56b5c8 100644 --- a/routers/repo/editor.go +++ b/routers/repo/editor.go @@ -660,7 +660,7 @@ func RemoveUploadFileFromServer(ctx *context.Context, form auth.RemoveUploadFile ctx.Status(204) } -// GetUniquePatchBranchName Gets a unquie branch name for a new patch branch +// GetUniquePatchBranchName Gets a unique branch name for a new patch branch // It will be in the form of -patch- where is the first branch of this format // that doesn't already exist func GetUniquePatchBranchName(ctx *context.Context) string { From 27ba88cbc90d32ca4a0a388dc59cdfca855b23ef Mon Sep 17 00:00:00 2001 From: Richard Mahn Date: Mon, 15 Jul 2019 23:43:57 -0400 Subject: [PATCH 4/7] Adds unit tests for a few helper functions --- routers/repo/editor_test.go | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/routers/repo/editor_test.go b/routers/repo/editor_test.go index 660f088e8848b..7287c6f43f14c 100644 --- a/routers/repo/editor_test.go +++ b/routers/repo/editor_test.go @@ -5,6 +5,8 @@ package repo import ( + "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/test" "testing" "code.gitea.io/gitea/models" @@ -37,3 +39,33 @@ func TestCleanUploadName(t *testing.T) { assert.EqualValues(t, cleanUploadFileName(k), v) } } + +func TestGetUniquePatchBranchName(t *testing.T) { + models.PrepareTestEnv(t) + ctx := test.MockContext(t, "user2/repo1") + ctx.SetParams(":id", "1") + test.LoadRepo(t, ctx, 1) + test.LoadRepoCommit(t, ctx) + test.LoadUser(t, ctx, 2) + test.LoadGitRepo(t, ctx) + expectedBranchName := "user2-patch-1" + branchName := GetUniquePatchBranchName(ctx) + assert.Equal(t, expectedBranchName, branchName) +} + +func TestGetClosestParentWithFiles(t *testing.T) { + models.PrepareTestEnv(t) + ctx := test.MockContext(t, "user2/repo1") + ctx.SetParams(":id", "1") + test.LoadRepo(t, ctx, 1) + test.LoadRepoCommit(t, ctx) + test.LoadUser(t, ctx, 2) + test.LoadGitRepo(t, ctx) + repo := ctx.Repo.Repository + branch := repo.DefaultBranch + gitRepo, _ := git.OpenRepository(repo.RepoPath()) + commit, _ := gitRepo.GetBranchCommit(branch) + expectedTreePath := "" + treePath := GetClosestParentWithFiles("dir/dir/dir", commit) + assert.Equal(t, expectedTreePath, treePath) +} From 568b7d132528646b106c9e3ef1ab9a93a603ecc7 Mon Sep 17 00:00:00 2001 From: Richard Mahn Date: Tue, 16 Jul 2019 06:47:00 -0400 Subject: [PATCH 5/7] Fixes per review --- routers/repo/editor.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/routers/repo/editor.go b/routers/repo/editor.go index 9f4da2c56b5c8..d9ac3549d2b4c 100644 --- a/routers/repo/editor.go +++ b/routers/repo/editor.go @@ -661,8 +661,9 @@ func RemoveUploadFileFromServer(ctx *context.Context, form auth.RemoveUploadFile } // GetUniquePatchBranchName Gets a unique branch name for a new patch branch -// It will be in the form of -patch- where is the first branch of this format -// that doesn't already exist +// It will be in the form of -patch- where is the first branch of this format +// that doesn't already exist. If we exceed 1000 tries or an error is thrown, we just return "" so the user has to +// type in the branch name themselves (will be an empty field) func GetUniquePatchBranchName(ctx *context.Context) string { prefix := ctx.User.LowerName + "-patch-" for i := 1; i <= 1000; i++ { @@ -671,6 +672,7 @@ func GetUniquePatchBranchName(ctx *context.Context) string { if git.IsErrBranchNotExist(err) { return branchName } + log.Error("GetUniquePatchBranchName: %v", err) return "" } } @@ -678,7 +680,8 @@ func GetUniquePatchBranchName(ctx *context.Context) string { } // GetClosestParentWithFiles Recursively gets the path of parent in a tree that has files (used when file in a tree is -// deleted). Returns "" for the root if no parents other than the root have files +// deleted). Returns "" for the root if no parents other than the root have files. If the given treePath isn't a +// SubTree or it has no entries, we go up one dir and see if we can return the user to that listing. func GetClosestParentWithFiles(treePath string, commit *git.Commit) string { if len(treePath) == 0 || treePath == "." { return "" From d19a1f8af73ad9e5a69073ab035aefbee46badb2 Mon Sep 17 00:00:00 2001 From: Richard Mahn Date: Tue, 16 Jul 2019 07:12:18 -0400 Subject: [PATCH 6/7] Fix for new branch creation and to unit test --- routers/repo/editor.go | 11 ++++++++--- routers/repo/editor_test.go | 11 +++++++++-- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/routers/repo/editor.go b/routers/repo/editor.go index d9ac3549d2b4c..be9eb574491c6 100644 --- a/routers/repo/editor.go +++ b/routers/repo/editor.go @@ -367,7 +367,7 @@ func DeleteFilePost(ctx *context.Context, form auth.DeleteRepoFileForm) { return } - if branchName != ctx.Repo.BranchName && !canCommit { + if branchName == ctx.Repo.BranchName && !canCommit { ctx.Data["Err_NewBranchName"] = true ctx.Data["commit_choice"] = frmCommitChoiceNewBranch ctx.RenderWithErr(ctx.Tr("repo.editor.cannot_commit_to_protected_branch", branchName), tplDeleteFile, &form) @@ -438,13 +438,18 @@ func DeleteFilePost(ctx *context.Context, form auth.DeleteRepoFileForm) { ctx.Redirect(ctx.Repo.RepoLink + "/compare/" + ctx.Repo.BranchName + "..." + form.NewBranchName) } else { treePath := filepath.Dir(ctx.Repo.TreePath) - if len(treePath) > 0 && treePath != "." { + if treePath == "." { + treePath = "" // the file deleted was in the root, so we return the user to the root directory + } + if len(treePath) > 0 { // Need to get the latest commit since it changed commit, err := ctx.Repo.GitRepo.GetBranchCommit(ctx.Repo.BranchName) if err == nil && commit != nil { + // We have the comment, now find what directory we can return the user to + // (must have entries) treePath = GetClosestParentWithFiles(treePath, commit) } else { - treePath = "" + treePath = "" // otherwise return them to the root of the repo } } ctx.Redirect(ctx.Repo.RepoLink + "/src/branch/" + util.PathEscapeSegments(branchName) + "/" + util.PathEscapeSegments(treePath)) diff --git a/routers/repo/editor_test.go b/routers/repo/editor_test.go index 7287c6f43f14c..7a68ecc9b78f8 100644 --- a/routers/repo/editor_test.go +++ b/routers/repo/editor_test.go @@ -66,6 +66,13 @@ func TestGetClosestParentWithFiles(t *testing.T) { gitRepo, _ := git.OpenRepository(repo.RepoPath()) commit, _ := gitRepo.GetBranchCommit(branch) expectedTreePath := "" - treePath := GetClosestParentWithFiles("dir/dir/dir", commit) - assert.Equal(t, expectedTreePath, treePath) + + expectedTreePath = "" // Should return the root dir, empty string, since there are no subdirs in this repo + for _, deletedFile := range []string{ + "dir1/dir2/dir3/file.txt", + "file.txt", + } { + treePath := GetClosestParentWithFiles(deletedFile, commit) + assert.Equal(t, expectedTreePath, treePath) + } } From 6057b879cf1641da4c7d621b8c895900061b8082 Mon Sep 17 00:00:00 2001 From: Richard Mahn Date: Tue, 16 Jul 2019 07:39:31 -0400 Subject: [PATCH 7/7] Fixes the template used for errors on delete --- routers/repo/editor.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/routers/repo/editor.go b/routers/repo/editor.go index be9eb574491c6..1c7eee98ac6f8 100644 --- a/routers/repo/editor.go +++ b/routers/repo/editor.go @@ -392,20 +392,20 @@ func DeleteFilePost(ctx *context.Context, form auth.DeleteRepoFileForm) { }); err != nil { // This is where we handle all the errors thrown by repofiles.DeleteRepoFile if git.IsErrNotExist(err) || models.IsErrRepoFileDoesNotExist(err) { - ctx.RenderWithErr(ctx.Tr("repo.editor.file_deleting_no_longer_exists", ctx.Repo.TreePath), tplEditFile, &form) + ctx.RenderWithErr(ctx.Tr("repo.editor.file_deleting_no_longer_exists", ctx.Repo.TreePath), tplDeleteFile, &form) } else if models.IsErrFilenameInvalid(err) { ctx.Data["Err_TreePath"] = true - ctx.RenderWithErr(ctx.Tr("repo.editor.filename_is_invalid", ctx.Repo.TreePath), tplEditFile, &form) + ctx.RenderWithErr(ctx.Tr("repo.editor.filename_is_invalid", ctx.Repo.TreePath), tplDeleteFile, &form) } else if models.IsErrFilePathInvalid(err) { ctx.Data["Err_TreePath"] = true if fileErr, ok := err.(models.ErrFilePathInvalid); ok { switch fileErr.Type { case git.EntryModeSymlink: - ctx.RenderWithErr(ctx.Tr("repo.editor.file_is_a_symlink", fileErr.Path), tplEditFile, &form) + ctx.RenderWithErr(ctx.Tr("repo.editor.file_is_a_symlink", fileErr.Path), tplDeleteFile, &form) case git.EntryModeTree: - ctx.RenderWithErr(ctx.Tr("repo.editor.filename_is_a_directory", fileErr.Path), tplEditFile, &form) + ctx.RenderWithErr(ctx.Tr("repo.editor.filename_is_a_directory", fileErr.Path), tplDeleteFile, &form) case git.EntryModeBlob: - ctx.RenderWithErr(ctx.Tr("repo.editor.directory_is_a_file", fileErr.Path), tplEditFile, &form) + ctx.RenderWithErr(ctx.Tr("repo.editor.directory_is_a_file", fileErr.Path), tplDeleteFile, &form) default: ctx.ServerError("DeleteRepoFile", err) } @@ -415,19 +415,19 @@ func DeleteFilePost(ctx *context.Context, form auth.DeleteRepoFileForm) { } else if git.IsErrBranchNotExist(err) { // For when a user deletes a file to a branch that no longer exists if branchErr, ok := err.(git.ErrBranchNotExist); ok { - ctx.RenderWithErr(ctx.Tr("repo.editor.branch_does_not_exist", branchErr.Name), tplEditFile, &form) + ctx.RenderWithErr(ctx.Tr("repo.editor.branch_does_not_exist", branchErr.Name), tplDeleteFile, &form) } else { ctx.Error(500, err.Error()) } } else if models.IsErrBranchAlreadyExists(err) { // For when a user specifies a new branch that already exists if branchErr, ok := err.(models.ErrBranchAlreadyExists); ok { - ctx.RenderWithErr(ctx.Tr("repo.editor.branch_already_exists", branchErr.BranchName), tplEditFile, &form) + ctx.RenderWithErr(ctx.Tr("repo.editor.branch_already_exists", branchErr.BranchName), tplDeleteFile, &form) } else { ctx.Error(500, err.Error()) } } else if models.IsErrCommitIDDoesNotMatch(err) { - ctx.RenderWithErr(ctx.Tr("repo.editor.file_changed_while_editing", ctx.Repo.RepoLink+"/compare/"+form.LastCommit+"..."+ctx.Repo.CommitID), tplEditFile, &form) + ctx.RenderWithErr(ctx.Tr("repo.editor.file_changed_while_deleting", ctx.Repo.RepoLink+"/compare/"+form.LastCommit+"..."+ctx.Repo.CommitID), tplDeleteFile, &form) } else { ctx.ServerError("DeleteRepoFile", err) }