diff --git a/models/error.go b/models/error.go index e8962f386b..ebaa8a135d 100644 --- a/models/error.go +++ b/models/error.go @@ -414,7 +414,7 @@ func IsErrSHAOrCommitIDNotProvided(err error) bool { } func (err ErrSHAOrCommitIDNotProvided) Error() string { - return "a SHA or commit ID must be proved when updating a file" + return "a SHA or commit ID must be provided when updating a file" } // ErrInvalidMergeStyle represents an error if merging with disabled merge strategy diff --git a/routers/api/v1/repo/file.go b/routers/api/v1/repo/file.go index 3408f88dd1..549fe9fae0 100644 --- a/routers/api/v1/repo/file.go +++ b/routers/api/v1/repo/file.go @@ -480,6 +480,8 @@ func ChangeFiles(ctx *context.APIContext) { // "$ref": "#/responses/error" // "404": // "$ref": "#/responses/notFound" + // "409": + // "$ref": "#/responses/conflict" // "413": // "$ref": "#/responses/quotaExceeded" // "422": @@ -584,6 +586,8 @@ func CreateFile(ctx *context.APIContext) { // "$ref": "#/responses/error" // "404": // "$ref": "#/responses/notFound" + // "409": + // "$ref": "#/responses/conflict" // "413": // "$ref": "#/responses/quotaExceeded" // "422": @@ -684,6 +688,8 @@ func UpdateFile(ctx *context.APIContext) { // "$ref": "#/responses/error" // "404": // "$ref": "#/responses/notFound" + // "409": + // "$ref": "#/responses/conflict" // "413": // "$ref": "#/responses/quotaExceeded" // "422": @@ -757,11 +763,19 @@ func handleCreateOrUpdateFileError(ctx *context.APIContext, err error) { ctx.Error(http.StatusForbidden, "Access", err) return } - if git_model.IsErrBranchAlreadyExists(err) || models.IsErrFilenameInvalid(err) || models.IsErrSHADoesNotMatch(err) || - models.IsErrFilePathInvalid(err) || models.IsErrRepoFileAlreadyExists(err) { + if git_model.IsErrBranchAlreadyExists(err) || + models.IsErrFilenameInvalid(err) || + models.IsErrSHAOrCommitIDNotProvided(err) || + models.IsErrFilePathInvalid(err) || + models.IsErrRepoFileAlreadyExists(err) { ctx.Error(http.StatusUnprocessableEntity, "Invalid", err) return } + if models.IsErrCommitIDDoesNotMatch(err) || + models.IsErrSHADoesNotMatch(err) { + ctx.Error(http.StatusConflict, "Conflict", err) + return + } if git_model.IsErrBranchNotExist(err) || git.IsErrBranchNotExist(err) { ctx.Error(http.StatusNotFound, "BranchDoesNotExist", err) return diff --git a/services/repository/files/update.go b/services/repository/files/update.go index 5e8834c6de..8fb9644fa4 100644 --- a/services/repository/files/update.go +++ b/services/repository/files/update.go @@ -193,28 +193,34 @@ func ChangeRepoFiles(ctx context.Context, repo *repo_model.Repository, doer *use } if hasOldBranch { - // Get the commit of the original branch - commit, err := t.GetBranchCommit(opts.OldBranch) + // Get the current commit of the original branch + actualBaseCommit, err := t.GetBranchCommit(opts.OldBranch) if err != nil { return nil, err // Couldn't get a commit for the branch } - // Assigned LastCommitID in opts if it hasn't been set - if opts.LastCommitID == "" { - opts.LastCommitID = commit.ID.String() - } else { - lastCommitID, err := t.gitRepo.ConvertToGitID(opts.LastCommitID) + var lastKnownCommit git.ObjectID // when nil, the sha provided in the opts.Files must match the current blob-sha + if opts.OldBranch != opts.NewBranch { + // when creating a new branch, ignore if a file has been changed in the meantime + // (such changes will visible when doing the merge) + lastKnownCommit = actualBaseCommit.ID + } else if opts.LastCommitID != "" { + lastKnownCommit, err = t.gitRepo.ConvertToGitID(opts.LastCommitID) if err != nil { return nil, fmt.Errorf("ConvertToSHA1: Invalid last commit ID: %w", err) } - opts.LastCommitID = lastCommitID.String() } for _, file := range opts.Files { - if err := handleCheckErrors(file, commit, opts); err != nil { + if err := handleCheckErrors(file, actualBaseCommit, lastKnownCommit); err != nil { return nil, err } } + + if opts.LastCommitID == "" { + // needed for t.CommitTree + opts.LastCommitID = actualBaseCommit.ID.String() + } } contentStore := lfs.NewContentStore() @@ -277,9 +283,9 @@ func ChangeRepoFiles(ctx context.Context, repo *repo_model.Repository, doer *use } // handles the check for various issues for ChangeRepoFiles -func handleCheckErrors(file *ChangeRepoFile, commit *git.Commit, opts *ChangeRepoFilesOptions) error { +func handleCheckErrors(file *ChangeRepoFile, actualBaseCommit *git.Commit, lastKnownCommit git.ObjectID) error { if file.Operation == "update" || file.Operation == "delete" { - fromEntry, err := commit.GetTreeEntryByPath(file.Options.fromTreePath) + fromEntry, err := actualBaseCommit.GetTreeEntryByPath(file.Options.fromTreePath) if err != nil { return err } @@ -292,22 +298,22 @@ func handleCheckErrors(file *ChangeRepoFile, commit *git.Commit, opts *ChangeRep CurrentSHA: fromEntry.ID.String(), } } - } else if opts.LastCommitID != "" { - // If a lastCommitID was given and it doesn't match the commitID of the head of the branch throw - // an error, but only if we aren't creating a new branch. - if commit.ID.String() != opts.LastCommitID && opts.OldBranch == opts.NewBranch { - if changed, err := commit.FileChangedSinceCommit(file.Options.treePath, opts.LastCommitID); err != nil { + } else if lastKnownCommit != nil { + if actualBaseCommit.ID.String() != lastKnownCommit.String() { + // If a lastKnownCommit was given and it doesn't match the actualBaseCommit, + // check if the file has been changed in between + if changed, err := actualBaseCommit.FileChangedSinceCommit(file.Options.treePath, lastKnownCommit.String()); err != nil { return err } else if changed { return models.ErrCommitIDDoesNotMatch{ - GivenCommitID: opts.LastCommitID, - CurrentCommitID: opts.LastCommitID, + GivenCommitID: lastKnownCommit.String(), + CurrentCommitID: actualBaseCommit.ID.String(), } } - // The file wasn't modified, so we are good to delete it + // The file wasn't modified, so we are good to update it } } else { - // When updating a file, a lastCommitID or SHA needs to be given to make sure other commits + // When updating a file, a lastKnownCommit or SHA needs to be given to make sure other commits // haven't been made. We throw an error if one wasn't provided. return models.ErrSHAOrCommitIDNotProvided{} } @@ -322,7 +328,7 @@ func handleCheckErrors(file *ChangeRepoFile, commit *git.Commit, opts *ChangeRep subTreePath := "" for index, part := range treePathParts { subTreePath = path.Join(subTreePath, part) - entry, err := commit.GetTreeEntryByPath(subTreePath) + entry, err := actualBaseCommit.GetTreeEntryByPath(subTreePath) if err != nil { if git.IsErrNotExist(err) { // Means there is no item with that name, so we're good diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 557ea5ea2b..1b5f55f97b 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -6897,6 +6897,9 @@ "404": { "$ref": "#/responses/notFound" }, + "409": { + "$ref": "#/responses/conflict" + }, "413": { "$ref": "#/responses/quotaExceeded" }, @@ -7010,6 +7013,9 @@ "404": { "$ref": "#/responses/notFound" }, + "409": { + "$ref": "#/responses/conflict" + }, "413": { "$ref": "#/responses/quotaExceeded" }, @@ -7074,6 +7080,9 @@ "404": { "$ref": "#/responses/notFound" }, + "409": { + "$ref": "#/responses/conflict" + }, "413": { "$ref": "#/responses/quotaExceeded" }, diff --git a/tests/e2e/declare_repos_test.go b/tests/e2e/declare_repos_test.go index 83ee40c71a..351f7821eb 100644 --- a/tests/e2e/declare_repos_test.go +++ b/tests/e2e/declare_repos_test.go @@ -74,6 +74,7 @@ func newRepo(t *testing.T, userID int64, repoName string, fileChanges []FileChan nil, ) + var lastCommitID string for _, file := range fileChanges { for i, version := range file.Versions { operation := "update" @@ -108,9 +109,12 @@ func newRepo(t *testing.T, userID int64, repoName string, fileChanges []FileChan Author: time.Now(), Committer: time.Now(), }, + LastCommitID: lastCommitID, }) require.NoError(t, err) assert.NotEmpty(t, resp) + + lastCommitID = resp.Commit.SHA } } diff --git a/tests/integration/api_issue_config_test.go b/tests/integration/api_issue_config_test.go index 809be572da..3653a859b1 100644 --- a/tests/integration/api_issue_config_test.go +++ b/tests/integration/api_issue_config_test.go @@ -155,7 +155,7 @@ func TestAPIRepoIssueConfigPaths(t *testing.T) { assert.False(t, issueConfig.BlankIssuesEnabled) assert.Empty(t, issueConfig.ContactLinks) - _, err = deleteFileInBranch(owner, repo, fullPath, repo.DefaultBranch) + err = deleteFileInBranch(owner, repo, fullPath, repo.DefaultBranch) require.NoError(t, err) }) } diff --git a/tests/integration/api_issue_templates_test.go b/tests/integration/api_issue_templates_test.go index 49b1a6f277..47ba34198a 100644 --- a/tests/integration/api_issue_templates_test.go +++ b/tests/integration/api_issue_templates_test.go @@ -47,9 +47,7 @@ func TestAPIIssueTemplateList(t *testing.T) { for _, template := range templateCandidates { t.Run(template, func(t *testing.T) { defer tests.PrintCurrentTest(t)() - defer func() { - deleteFileInBranch(user, repo, template, repo.DefaultBranch) - }() + defer deleteFileInBranch(user, repo, template, repo.DefaultBranch) err := createOrReplaceFileInBranch(user, repo, template, repo.DefaultBranch, `--- diff --git a/tests/integration/api_repo_file_helpers.go b/tests/integration/api_repo_file_helpers.go index 09cf93d8a5..61407bf1bf 100644 --- a/tests/integration/api_repo_file_helpers.go +++ b/tests/integration/api_repo_file_helpers.go @@ -10,6 +10,7 @@ import ( repo_model "forgejo.org/models/repo" user_model "forgejo.org/models/user" "forgejo.org/modules/git" + "forgejo.org/modules/gitrepo" api "forgejo.org/modules/structs" files_service "forgejo.org/services/repository/files" ) @@ -30,7 +31,12 @@ func createFileInBranch(user *user_model.User, repo *repo_model.Repository, tree return files_service.ChangeRepoFiles(git.DefaultContext, repo, user, opts) } -func deleteFileInBranch(user *user_model.User, repo *repo_model.Repository, treePath, branchName string) (*api.FilesResponse, error) { +func deleteFileInBranch(user *user_model.User, repo *repo_model.Repository, treePath, branchName string) error { + commitID, err := gitrepo.GetBranchCommitID(git.DefaultContext, repo, branchName) + if err != nil { + return err + } + opts := &files_service.ChangeRepoFilesOptions{ Files: []*files_service.ChangeRepoFile{ { @@ -38,16 +44,17 @@ func deleteFileInBranch(user *user_model.User, repo *repo_model.Repository, tree TreePath: treePath, }, }, - OldBranch: branchName, - Author: nil, - Committer: nil, + OldBranch: branchName, + Author: nil, + Committer: nil, + LastCommitID: commitID, } - return files_service.ChangeRepoFiles(git.DefaultContext, repo, user, opts) + _, err = files_service.ChangeRepoFiles(git.DefaultContext, repo, user, opts) + return err } func createOrReplaceFileInBranch(user *user_model.User, repo *repo_model.Repository, treePath, branchName, content string) error { - _, err := deleteFileInBranch(user, repo, treePath, branchName) - + err := deleteFileInBranch(user, repo, treePath, branchName) if err != nil && !models.IsErrRepoFileDoesNotExist(err) { return err } diff --git a/tests/integration/api_repo_file_update_test.go b/tests/integration/api_repo_file_update_test.go index 61deb10c92..878d865aff 100644 --- a/tests/integration/api_repo_file_update_test.go +++ b/tests/integration/api_repo_file_update_test.go @@ -214,7 +214,7 @@ func TestAPIUpdateFile(t *testing.T) { updateFileOptions.SHA = "badsha" req = NewRequestWithJSON(t, "PUT", fmt.Sprintf("/api/v1/repos/%s/%s/contents/%s", user2.Name, repo1.Name, treePath), &updateFileOptions). AddTokenAuth(token2) - resp = MakeRequest(t, req, http.StatusUnprocessableEntity) + resp = MakeRequest(t, req, http.StatusConflict) expectedAPIError := context.APIError{ Message: "sha does not match [given: " + updateFileOptions.SHA + ", expected: " + correctSHA + "]", URL: setting.API.SwaggerURL, diff --git a/tests/integration/api_repo_files_change_test.go b/tests/integration/api_repo_files_change_test.go index 1772dec6a6..6b1edd047b 100644 --- a/tests/integration/api_repo_files_change_test.go +++ b/tests/integration/api_repo_files_change_test.go @@ -214,7 +214,7 @@ func TestAPIChangeFiles(t *testing.T) { changeFilesOptions.Files[0].SHA = "badsha" req = NewRequestWithJSON(t, "POST", url, &changeFilesOptions). AddTokenAuth(token2) - resp = MakeRequest(t, req, http.StatusUnprocessableEntity) + resp = MakeRequest(t, req, http.StatusConflict) expectedAPIError := context.APIError{ Message: "sha does not match [given: " + changeFilesOptions.Files[0].SHA + ", expected: " + correctSHA + "]", URL: setting.API.SwaggerURL, diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index ed3d3baaf4..0c05b3da37 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -6,6 +6,7 @@ package integration import ( "fmt" + "io" "net/http" "net/http/httptest" "net/url" @@ -19,6 +20,7 @@ import ( repo_model "forgejo.org/models/repo" "forgejo.org/models/unittest" user_model "forgejo.org/models/user" + "forgejo.org/modules/git" "forgejo.org/modules/gitrepo" repo_module "forgejo.org/modules/repository" "forgejo.org/modules/test" @@ -93,16 +95,9 @@ func TestPullView_SelfReviewNotification(t *testing.T) { require.NoError(t, err) // create a new branch to prepare for pull request - _, err = files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{ - NewBranch: "codeowner-basebranch", - Files: []*files_service.ChangeRepoFile{ - { - Operation: "update", - TreePath: "README.md", - ContentReader: strings.NewReader("# This is a new project\n"), - }, - }, - }) + err = updateFileInBranch(user2, repo, "README.md", "codeowner-basebranch", + strings.NewReader("# This is a new project\n"), + ) require.NoError(t, err) // Create a pull request. @@ -366,16 +361,9 @@ func TestPullView_CodeOwner(t *testing.T) { defer tests.PrintCurrentTest(t)() // create a new branch to prepare for pull request - _, err := files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{ - NewBranch: "codeowner-basebranch", - Files: []*files_service.ChangeRepoFile{ - { - Operation: "update", - TreePath: "README.md", - ContentReader: strings.NewReader("# This is a new project\n"), - }, - }, - }) + err := updateFileInBranch(user2, repo, "README.md", "codeowner-basebranch", + strings.NewReader("# This is a new project\n"), + ) require.NoError(t, err) // Create a pull request. @@ -400,31 +388,18 @@ func TestPullView_CodeOwner(t *testing.T) { }) // change the default branch CODEOWNERS file to change README.md's codeowner - _, err := files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{ - Files: []*files_service.ChangeRepoFile{ - { - Operation: "update", - TreePath: "CODEOWNERS", - ContentReader: strings.NewReader("README.md @user8\n"), - }, - }, - }) + err := updateFileInBranch(user2, repo, "CODEOWNERS", "", + strings.NewReader("README.md @user8\n"), + ) require.NoError(t, err) t.Run("Second Pull Request", func(t *testing.T) { defer tests.PrintCurrentTest(t)() // create a new branch to prepare for pull request - _, err = files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{ - NewBranch: "codeowner-basebranch2", - Files: []*files_service.ChangeRepoFile{ - { - Operation: "update", - TreePath: "README.md", - ContentReader: strings.NewReader("# This is a new project2\n"), - }, - }, - }) + err := updateFileInBranch(user2, repo, "README.md", "codeowner-basebranch2", + strings.NewReader("# This is a new project2\n"), + ) require.NoError(t, err) // Create a pull request. @@ -446,16 +421,9 @@ func TestPullView_CodeOwner(t *testing.T) { require.NoError(t, err) // create a new branch to prepare for pull request - _, err = files_service.ChangeRepoFiles(db.DefaultContext, forkedRepo, user5, &files_service.ChangeRepoFilesOptions{ - NewBranch: "codeowner-basebranch-forked", - Files: []*files_service.ChangeRepoFile{ - { - Operation: "update", - TreePath: "README.md", - ContentReader: strings.NewReader("# This is a new forked project\n"), - }, - }, - }) + err = updateFileInBranch(user5, forkedRepo, "README.md", "codeowner-basebranch-forked", + strings.NewReader("# This is a new forked project\n"), + ) require.NoError(t, err) session := loginUser(t, "user5") @@ -762,3 +730,32 @@ func TestPullRequestReplyMail(t *testing.T) { }) }) } + +func updateFileInBranch(user *user_model.User, repo *repo_model.Repository, treePath, newBranch string, content io.ReadSeeker) error { + oldBranch, err := gitrepo.GetDefaultBranch(git.DefaultContext, repo) + if err != nil { + return err + } + + commitID, err := gitrepo.GetBranchCommitID(git.DefaultContext, repo, oldBranch) + if err != nil { + return err + } + + opts := &files_service.ChangeRepoFilesOptions{ + Files: []*files_service.ChangeRepoFile{ + { + Operation: "update", + TreePath: treePath, + ContentReader: content, + }, + }, + OldBranch: oldBranch, + NewBranch: newBranch, + Author: nil, + Committer: nil, + LastCommitID: commitID, + } + _, err = files_service.ChangeRepoFiles(git.DefaultContext, repo, user, opts) + return err +} diff --git a/tests/integration/repofiles_change_test.go b/tests/integration/repofiles_change_test.go index b993fdf936..42d47c4591 100644 --- a/tests/integration/repofiles_change_test.go +++ b/tests/integration/repofiles_change_test.go @@ -294,6 +294,30 @@ func TestChangeRepoFiles(t *testing.T) { assert.Equal(t, expectedFileResponse.Commit.Author.Name, filesResponse.Commit.Author.Name) }) + t.Run("Update with commit ID (without blob sha)", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + opts := getUpdateRepoFilesOptions(repo) + + commit, err := gitRepo.GetBranchCommit(opts.NewBranch) + require.NoError(t, err) + + opts.Files[0].SHA = "" + opts.LastCommitID = commit.ID.String() + filesResponse, err := files_service.ChangeRepoFiles(git.DefaultContext, repo, doer, opts) + require.NoError(t, err) + + commit, err = gitRepo.GetBranchCommit(opts.NewBranch) + require.NoError(t, err) + lastCommit, err := commit.GetCommitByPath(opts.Files[0].TreePath) + require.NoError(t, err) + expectedFileResponse := getExpectedFileResponseForRepofilesUpdate(commit.ID.String(), opts.Files[0].TreePath, lastCommit.ID.String(), lastCommit.Committer.When) + assert.Equal(t, expectedFileResponse.Content, filesResponse.Files[0]) + assert.Equal(t, expectedFileResponse.Commit.SHA, filesResponse.Commit.SHA) + assert.Equal(t, expectedFileResponse.Commit.HTMLURL, filesResponse.Commit.HTMLURL) + assert.Equal(t, expectedFileResponse.Commit.Author.Email, filesResponse.Commit.Author.Email) + assert.Equal(t, expectedFileResponse.Commit.Author.Name, filesResponse.Commit.Author.Name) + }) + t.Run("Update and move", func(t *testing.T) { defer tests.PrintCurrentTest(t)() opts := getUpdateRepoFilesOptions(repo) @@ -415,6 +439,26 @@ func TestChangeRepoFilesErrors(t *testing.T) { assert.EqualError(t, err, expectedError) }) + t.Run("missing SHA", func(t *testing.T) { + opts := getUpdateRepoFilesOptions(repo) + opts.Files[0].SHA = "" + filesResponse, err := files_service.ChangeRepoFiles(git.DefaultContext, repo, doer, opts) + assert.Nil(t, filesResponse) + require.Error(t, err) + expectedError := "a SHA or commit ID must be provided when updating a file" + assert.EqualError(t, err, expectedError) + }) + + t.Run("bad last commit ID", func(t *testing.T) { + opts := getUpdateRepoFilesOptions(repo) + opts.LastCommitID = "bad" + filesResponse, err := files_service.ChangeRepoFiles(git.DefaultContext, repo, doer, opts) + assert.Nil(t, filesResponse) + require.Error(t, err) + expectedError := "ConvertToSHA1: Invalid last commit ID: object does not exist [id: bad, rel_path: ]" + assert.EqualError(t, err, expectedError) + }) + t.Run("new branch already exists", func(t *testing.T) { opts := getUpdateRepoFilesOptions(repo) opts.NewBranch = "develop" diff --git a/tests/test_utils.go b/tests/test_utils.go index 66da5e6bea..75d1f98914 100644 --- a/tests/test_utils.go +++ b/tests/test_utils.go @@ -24,6 +24,7 @@ import ( user_model "forgejo.org/models/user" "forgejo.org/modules/base" "forgejo.org/modules/git" + "forgejo.org/modules/gitrepo" "forgejo.org/modules/graceful" "forgejo.org/modules/log" "forgejo.org/modules/optional" @@ -408,6 +409,9 @@ func CreateDeclarativeRepoWithOptions(t *testing.T, owner *user_model.User, opts assert.True(t, autoInit, "Files cannot be specified if AutoInit is disabled") files := opts.Files.Value() + commitID, err := gitrepo.GetBranchCommitID(git.DefaultContext, repo, "main") + require.NoError(t, err) + resp, err := files_service.ChangeRepoFiles(git.DefaultContext, repo, owner, &files_service.ChangeRepoFilesOptions{ Files: files, Message: "add files", @@ -425,6 +429,7 @@ func CreateDeclarativeRepoWithOptions(t *testing.T, owner *user_model.User, opts Author: time.Now(), Committer: time.Now(), }, + LastCommitID: commitID, }) require.NoError(t, err) assert.NotEmpty(t, resp)