Refactor "change file" API (#34855)

Follow up the "editor" refactor, use the same approach to simplify code,
and fix some docs & comments

---------

Signed-off-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: delvh <dev.lh@web.de>

modules/structs/repo_file.go

@@ -22,6 +22,23 @@ type FileOptions struct {
 	Signoff bool `json:"signoff"`
 }
 
+type FileOptionsWithSHA struct {
+	FileOptions
+	// the blob ID (SHA) for the file that already exists, it is required for changing existing files
+	// required: true
+	SHA string `json:"sha" binding:"Required"`
+}
+
+func (f *FileOptions) GetFileOptions() *FileOptions {
+	return f
+}
+
+type FileOptionsInterface interface {
+	GetFileOptions() *FileOptions
+}
+
+var _ FileOptionsInterface = (*FileOptions)(nil)
+
 // CreateFileOptions options for creating files
 // Note: `author` and `committer` are optional (if only one is given, it will be used for the other, otherwise the authenticated user will be used)
 type CreateFileOptions struct {
@@ -31,29 +48,16 @@ type CreateFileOptions struct {
 	ContentBase64 string `json:"content"`
 }
 
-// Branch returns branch name
-func (o *CreateFileOptions) Branch() string {
-	return o.FileOptions.BranchName
-}
-
 // DeleteFileOptions options for deleting files (used for other File structs below)
 // Note: `author` and `committer` are optional (if only one is given, it will be used for the other, otherwise the authenticated user will be used)
 type DeleteFileOptions struct {
-	FileOptions
+	FileOptionsWithSHA
-	// sha is the SHA for the file that already exists
-	// required: true
-	SHA string `json:"sha" binding:"Required"`
-}
-
-// Branch returns branch name
-func (o *DeleteFileOptions) Branch() string {
-	return o.FileOptions.BranchName
 }
 
 // UpdateFileOptions options for updating files
 // Note: `author` and `committer` are optional (if only one is given, it will be used for the other, otherwise the authenticated user will be used)
 type UpdateFileOptions struct {
-	DeleteFileOptions
+	FileOptionsWithSHA
 	// content must be base64 encoded
 	// required: true
 	ContentBase64 string `json:"content"`
@@ -61,25 +65,21 @@ type UpdateFileOptions struct {
 	FromPath string `json:"from_path" binding:"MaxSize(500)"`
 }
 
-// Branch returns branch name
+// FIXME: there is no LastCommitID in FileOptions, actually it should be an alternative to the SHA in ChangeFileOperation
-func (o *UpdateFileOptions) Branch() string {
-	return o.FileOptions.BranchName
-}
-
-// FIXME: ChangeFileOperation.SHA is NOT required for update or delete if last commit is provided in the options.
 
 // ChangeFileOperation for creating, updating or deleting a file
 type ChangeFileOperation struct {
-	// indicates what to do with the file
+	// indicates what to do with the file: "create" for creating a new file, "update" for updating an existing file,
+	// "upload" for creating or updating a file, "rename" for renaming a file, and "delete" for deleting an existing file.
 	// required: true
-	// enum: create,update,delete
+	// enum: create,update,upload,rename,delete
 	Operation string `json:"operation" binding:"Required"`
 	// path to the existing or new file
 	// required: true
 	Path string `json:"path" binding:"Required;MaxSize(500)"`
-	// new or updated file content, must be base64 encoded
+	// new or updated file content, it must be base64 encoded
 	ContentBase64 string `json:"content"`
-	// sha is the SHA for the file that already exists, required for update or delete
+	// the blob ID (SHA) for the file that already exists, required for changing existing files
 	SHA string `json:"sha"`
 	// old path of the file to move
 	FromPath string `json:"from_path"`
@@ -94,20 +94,10 @@ type ChangeFilesOptions struct {
 	Files []*ChangeFileOperation `json:"files" binding:"Required"`
 }
 
-// Branch returns branch name
-func (o *ChangeFilesOptions) Branch() string {
-	return o.FileOptions.BranchName
-}
-
-// FileOptionInterface provides a unified interface for the different file options
-type FileOptionInterface interface {
-	Branch() string
-}
-
 // ApplyDiffPatchFileOptions options for applying a diff patch
 // Note: `author` and `committer` are optional (if only one is given, it will be used for the other, otherwise the authenticated user will be used)
 type ApplyDiffPatchFileOptions struct {
-	DeleteFileOptions
+	FileOptions
 	// required: true
 	Content string `json:"content"`
 }

routers/api/v1/api.go

@@ -455,15 +455,6 @@ func reqRepoWriter(unitTypes ...unit.Type) func(ctx *context.APIContext) {
 	}
 }
 
-// reqRepoBranchWriter user should have a permission to write to a branch, or be a site admin
-func reqRepoBranchWriter(ctx *context.APIContext) {
-	options, ok := web.GetForm(ctx).(api.FileOptionInterface)
-	if !ok || (!ctx.Repo.CanWriteToBranch(ctx, ctx.Doer, options.Branch()) && !ctx.IsUserSiteAdmin()) {
-		ctx.APIError(http.StatusForbidden, "user should have a permission to write to this branch")
-		return
-	}
-}
-
 // reqRepoReader user should have specific read permission or be a repo admin or a site admin
 func reqRepoReader(unitType unit.Type) func(ctx *context.APIContext) {
 	return func(ctx *context.APIContext) {
@@ -744,9 +735,17 @@ func mustEnableWiki(ctx *context.APIContext) {
 	}
 }
 
+// FIXME: for consistency, maybe most mustNotBeArchived checks should be replaced with mustEnableEditor
 func mustNotBeArchived(ctx *context.APIContext) {
 	if ctx.Repo.Repository.IsArchived {
-		ctx.APIError(http.StatusLocked, fmt.Errorf("%s is archived", ctx.Repo.Repository.LogString()))
+		ctx.APIError(http.StatusLocked, fmt.Errorf("%s is archived", ctx.Repo.Repository.FullName()))
+		return
+	}
+}
+
+func mustEnableEditor(ctx *context.APIContext) {
+	if !ctx.Repo.Repository.CanEnableEditor() {
+		ctx.APIError(http.StatusLocked, fmt.Errorf("%s is not allowed to edit", ctx.Repo.Repository.FullName()))
 		return
 	}
 }
@@ -1424,16 +1423,19 @@ func Routes() *web.Router {
 					m.Get("/tags/{sha}", repo.GetAnnotatedTag)
 					m.Get("/notes/{sha}", repo.GetNote)
 				}, context.ReferencesGitRepo(true), reqRepoReader(unit.TypeCode))
-				m.Post("/diffpatch", reqRepoWriter(unit.TypeCode), reqToken(), bind(api.ApplyDiffPatchFileOptions{}), mustNotBeArchived, repo.ApplyDiffPatch)
 				m.Group("/contents", func() {
 					m.Get("", repo.GetContentsList)
 					m.Get("/*", repo.GetContents)
-					m.Post("", reqToken(), bind(api.ChangeFilesOptions{}), reqRepoBranchWriter, mustNotBeArchived, repo.ChangeFiles)
+					m.Group("", func() {
-					m.Group("/*", func() {
+						// "change file" operations, need permission to write to the target branch provided by the form
-						m.Post("", bind(api.CreateFileOptions{}), reqRepoBranchWriter, mustNotBeArchived, repo.CreateFile)
+						m.Post("", bind(api.ChangeFilesOptions{}), repo.ReqChangeRepoFileOptionsAndCheck, repo.ChangeFiles)
-						m.Put("", bind(api.UpdateFileOptions{}), reqRepoBranchWriter, mustNotBeArchived, repo.UpdateFile)
+						m.Group("/*", func() {
-						m.Delete("", bind(api.DeleteFileOptions{}), reqRepoBranchWriter, mustNotBeArchived, repo.DeleteFile)
+							m.Post("", bind(api.CreateFileOptions{}), repo.ReqChangeRepoFileOptionsAndCheck, repo.CreateFile)
-					}, reqToken())
+							m.Put("", bind(api.UpdateFileOptions{}), repo.ReqChangeRepoFileOptionsAndCheck, repo.UpdateFile)
+							m.Delete("", bind(api.DeleteFileOptions{}), repo.ReqChangeRepoFileOptionsAndCheck, repo.DeleteFile)
+						})
+						m.Post("/diffpatch", bind(api.ApplyDiffPatchFileOptions{}), repo.ReqChangeRepoFileOptionsAndCheck, repo.ApplyDiffPatch)
+					}, mustEnableEditor, reqToken())
 				}, reqRepoReader(unit.TypeCode), context.ReferencesGitRepo())
 				m.Group("/contents-ext", func() {
 					m.Get("", repo.GetContentsExt)
@@ -1441,7 +1443,7 @@ func Routes() *web.Router {
 				}, reqRepoReader(unit.TypeCode), context.ReferencesGitRepo())
 				m.Combo("/file-contents", reqRepoReader(unit.TypeCode), context.ReferencesGitRepo()).
 					Get(repo.GetFileContentsGet).
-					Post(bind(api.GetFilesOptions{}), repo.GetFileContentsPost) // POST method requires "write" permission, so we also support "GET" method above
+					Post(bind(api.GetFilesOptions{}), repo.GetFileContentsPost) // the POST method requires "write" permission, so we also support "GET" method above
 				m.Get("/signing-key.gpg", misc.SigningKeyGPG)
 				m.Get("/signing-key.pub", misc.SigningKeySSH)
 				m.Group("/topics", func() {

routers/api/v1/repo/file.go

@@ -62,7 +62,7 @@ func GetRawFile(ctx *context.APIContext) {
 	//   required: true
 	// - name: ref
 	//   in: query
-	//   description: "The name of the commit/branch/tag. Default the repository’s default branch"
+	//   description: "The name of the commit/branch/tag. Default to the repository’s default branch"
 	//   type: string
 	//   required: false
 	// responses:
@@ -115,7 +115,7 @@ func GetRawFileOrLFS(ctx *context.APIContext) {
 	//   required: true
 	// - name: ref
 	//   in: query
-	//   description: "The name of the commit/branch/tag. Default the repository’s default branch"
+	//   description: "The name of the commit/branch/tag. Default to the repository’s default branch"
 	//   type: string
 	//   required: false
 	// responses:
@@ -139,27 +139,27 @@ func GetRawFileOrLFS(ctx *context.APIContext) {
 	ctx.RespHeader().Set(giteaObjectTypeHeader, string(files_service.GetObjectTypeFromTreeEntry(entry)))
 
 	// LFS Pointer files are at most 1024 bytes - so any blob greater than 1024 bytes cannot be an LFS file
-	if blob.Size() > 1024 {
+	if blob.Size() > lfs.MetaFileMaxSize {
 		// First handle caching for the blob
 		if httpcache.HandleGenericETagTimeCache(ctx.Req, ctx.Resp, `"`+blob.ID.String()+`"`, lastModified) {
 			return
 		}
 
-		// OK not cached - serve!
+		// If not cached - serve!
 		if err := common.ServeBlob(ctx.Base, ctx.Repo.Repository, ctx.Repo.TreePath, blob, lastModified); err != nil {
 			ctx.APIErrorInternal(err)
 		}
 		return
 	}
 
-	// OK, now the blob is known to have at most 1024 bytes we can simply read this in one go (This saves reading it twice)
+	// OK, now the blob is known to have at most 1024 (lfs pointer max size) bytes,
+	// we can simply read this in one go (This saves reading it twice)
 	dataRc, err := blob.DataAsync()
 	if err != nil {
 		ctx.APIErrorInternal(err)
 		return
 	}
 
-	// FIXME: code from #19689, what if the file is large ... OOM ...
 	buf, err := io.ReadAll(dataRc)
 	if err != nil {
 		_ = dataRc.Close()
@@ -181,7 +181,7 @@ func GetRawFileOrLFS(ctx *context.APIContext) {
 			return
 		}
 
-		// OK not cached - serve!
+		// If not cached - serve!
 		common.ServeContentByReader(ctx.Base, ctx.Repo.TreePath, blob.Size(), bytes.NewReader(buf))
 		return
 	}
@@ -405,13 +405,6 @@ func GetEditorconfig(ctx *context.APIContext) {
 	ctx.JSON(http.StatusOK, def)
 }
 
-// canWriteFiles returns true if repository is editable and user has proper access level.
-func canWriteFiles(ctx *context.APIContext, branch string) bool {
-	return ctx.Repo.CanWriteToBranch(ctx, ctx.Doer, branch) &&
-		!ctx.Repo.Repository.IsMirror &&
-		!ctx.Repo.Repository.IsArchived
-}
-
 func base64Reader(s string) (io.ReadSeeker, error) {
 	b, err := base64.StdEncoding.DecodeString(s)
 	if err != nil {
@@ -420,6 +413,45 @@ func base64Reader(s string) (io.ReadSeeker, error) {
 	return bytes.NewReader(b), nil
 }
 
+func ReqChangeRepoFileOptionsAndCheck(ctx *context.APIContext) {
+	commonOpts := web.GetForm(ctx).(api.FileOptionsInterface).GetFileOptions()
+	commonOpts.BranchName = util.IfZero(commonOpts.BranchName, ctx.Repo.Repository.DefaultBranch)
+	commonOpts.NewBranchName = util.IfZero(commonOpts.NewBranchName, commonOpts.BranchName)
+	if !ctx.Repo.CanWriteToBranch(ctx, ctx.Doer, commonOpts.NewBranchName) && !ctx.IsUserSiteAdmin() {
+		ctx.APIError(http.StatusForbidden, "user should have a permission to write to the target branch")
+		return
+	}
+	changeFileOpts := &files_service.ChangeRepoFilesOptions{
+		Message:   commonOpts.Message,
+		OldBranch: commonOpts.BranchName,
+		NewBranch: commonOpts.NewBranchName,
+		Committer: &files_service.IdentityOptions{
+			GitUserName:  commonOpts.Committer.Name,
+			GitUserEmail: commonOpts.Committer.Email,
+		},
+		Author: &files_service.IdentityOptions{
+			GitUserName:  commonOpts.Author.Name,
+			GitUserEmail: commonOpts.Author.Email,
+		},
+		Dates: &files_service.CommitDateOptions{
+			Author:    commonOpts.Dates.Author,
+			Committer: commonOpts.Dates.Committer,
+		},
+		Signoff: commonOpts.Signoff,
+	}
+	if commonOpts.Dates.Author.IsZero() {
+		commonOpts.Dates.Author = time.Now()
+	}
+	if commonOpts.Dates.Committer.IsZero() {
+		commonOpts.Dates.Committer = time.Now()
+	}
+	ctx.Data["__APIChangeRepoFilesOptions"] = changeFileOpts
+}
+
+func getAPIChangeRepoFileOptions[T api.FileOptionsInterface](ctx *context.APIContext) (apiOpts T, opts *files_service.ChangeRepoFilesOptions) {
+	return web.GetForm(ctx).(T), ctx.Data["__APIChangeRepoFilesOptions"].(*files_service.ChangeRepoFilesOptions)
+}
+
 // ChangeFiles handles API call for modifying multiple files
 func ChangeFiles(ctx *context.APIContext) {
 	// swagger:operation POST /repos/{owner}/{repo}/contents repository repoChangeFiles
@@ -456,23 +488,18 @@ func ChangeFiles(ctx *context.APIContext) {
 	//     "$ref": "#/responses/error"
 	//   "423":
 	//     "$ref": "#/responses/repoArchivedError"
-
+	apiOpts, opts := getAPIChangeRepoFileOptions[*api.ChangeFilesOptions](ctx)
-	apiOpts := web.GetForm(ctx).(*api.ChangeFilesOptions)
+	if ctx.Written() {
-
+		return
-	if apiOpts.BranchName == "" {
-		apiOpts.BranchName = ctx.Repo.Repository.DefaultBranch
 	}
-
-	var files []*files_service.ChangeRepoFile
 	for _, file := range apiOpts.Files {
 		contentReader, err := base64Reader(file.ContentBase64)
 		if err != nil {
 			ctx.APIError(http.StatusUnprocessableEntity, err)
 			return
 		}
-		// FIXME: actually now we support more operations like "rename", "upload"
+		// FIXME: ChangeFileOperation.SHA is NOT required for update or delete if last commit is provided in the options
-		// FIXME: ChangeFileOperation.SHA is NOT required for update or delete if last commit is provided in the options.
+		// But the LastCommitID is not provided in the API options, need to fully fix them in API
-		// Need to fully fix them in API
 		changeRepoFile := &files_service.ChangeRepoFile{
 			Operation:     file.Operation,
 			TreePath:      file.Path,
@@ -480,41 +507,15 @@ func ChangeFiles(ctx *context.APIContext) {
 			ContentReader: contentReader,
 			SHA:           file.SHA,
 		}
-		files = append(files, changeRepoFile)
+		opts.Files = append(opts.Files, changeRepoFile)
-	}
-
-	opts := &files_service.ChangeRepoFilesOptions{
-		Files:     files,
-		Message:   apiOpts.Message,
-		OldBranch: apiOpts.BranchName,
-		NewBranch: apiOpts.NewBranchName,
-		Committer: &files_service.IdentityOptions{
-			GitUserName:  apiOpts.Committer.Name,
-			GitUserEmail: apiOpts.Committer.Email,
-		},
-		Author: &files_service.IdentityOptions{
-			GitUserName:  apiOpts.Author.Name,
-			GitUserEmail: apiOpts.Author.Email,
-		},
-		Dates: &files_service.CommitDateOptions{
-			Author:    apiOpts.Dates.Author,
-			Committer: apiOpts.Dates.Committer,
-		},
-		Signoff: apiOpts.Signoff,
-	}
-	if opts.Dates.Author.IsZero() {
-		opts.Dates.Author = time.Now()
-	}
-	if opts.Dates.Committer.IsZero() {
-		opts.Dates.Committer = time.Now()
 	}
 
 	if opts.Message == "" {
-		opts.Message = changeFilesCommitMessage(ctx, files)
+		opts.Message = changeFilesCommitMessage(ctx, opts.Files)
 	}
 
-	if filesResponse, err := createOrUpdateFiles(ctx, opts); err != nil {
+	if filesResponse, err := files_service.ChangeRepoFiles(ctx, ctx.Repo.Repository, ctx.Doer, opts); err != nil {
-		handleCreateOrUpdateFileError(ctx, err)
+		handleChangeRepoFilesError(ctx, err)
 	} else {
 		ctx.JSON(http.StatusCreated, filesResponse)
 	}
@@ -562,56 +563,27 @@ func CreateFile(ctx *context.APIContext) {
 	//   "423":
 	//     "$ref": "#/responses/repoArchivedError"
 
-	apiOpts := web.GetForm(ctx).(*api.CreateFileOptions)
+	apiOpts, opts := getAPIChangeRepoFileOptions[*api.CreateFileOptions](ctx)
-
+	if ctx.Written() {
-	if apiOpts.BranchName == "" {
+		return
-		apiOpts.BranchName = ctx.Repo.Repository.DefaultBranch
 	}
-
 	contentReader, err := base64Reader(apiOpts.ContentBase64)
 	if err != nil {
 		ctx.APIError(http.StatusUnprocessableEntity, err)
 		return
 	}
 
-	opts := &files_service.ChangeRepoFilesOptions{
+	opts.Files = append(opts.Files, &files_service.ChangeRepoFile{
-		Files: []*files_service.ChangeRepoFile{
+		Operation:     "create",
-			{
+		TreePath:      ctx.PathParam("*"),
-				Operation:     "create",
+		ContentReader: contentReader,
-				TreePath:      ctx.PathParam("*"),
+	})
-				ContentReader: contentReader,
-			},
-		},
-		Message:   apiOpts.Message,
-		OldBranch: apiOpts.BranchName,
-		NewBranch: apiOpts.NewBranchName,
-		Committer: &files_service.IdentityOptions{
-			GitUserName:  apiOpts.Committer.Name,
-			GitUserEmail: apiOpts.Committer.Email,
-		},
-		Author: &files_service.IdentityOptions{
-			GitUserName:  apiOpts.Author.Name,
-			GitUserEmail: apiOpts.Author.Email,
-		},
-		Dates: &files_service.CommitDateOptions{
-			Author:    apiOpts.Dates.Author,
-			Committer: apiOpts.Dates.Committer,
-		},
-		Signoff: apiOpts.Signoff,
-	}
-	if opts.Dates.Author.IsZero() {
-		opts.Dates.Author = time.Now()
-	}
-	if opts.Dates.Committer.IsZero() {
-		opts.Dates.Committer = time.Now()
-	}
-
 	if opts.Message == "" {
 		opts.Message = changeFilesCommitMessage(ctx, opts.Files)
 	}
 
-	if filesResponse, err := createOrUpdateFiles(ctx, opts); err != nil {
+	if filesResponse, err := files_service.ChangeRepoFiles(ctx, ctx.Repo.Repository, ctx.Doer, opts); err != nil {
-		handleCreateOrUpdateFileError(ctx, err)
+		handleChangeRepoFilesError(ctx, err)
 	} else {
 		fileResponse := files_service.GetFileResponseFromFilesResponse(filesResponse, 0)
 		ctx.JSON(http.StatusCreated, fileResponse)
@@ -659,96 +631,55 @@ func UpdateFile(ctx *context.APIContext) {
 	//     "$ref": "#/responses/error"
 	//   "423":
 	//     "$ref": "#/responses/repoArchivedError"
-	apiOpts := web.GetForm(ctx).(*api.UpdateFileOptions)
+
-	if ctx.Repo.Repository.IsEmpty {
+	apiOpts, opts := getAPIChangeRepoFileOptions[*api.UpdateFileOptions](ctx)
-		ctx.APIError(http.StatusUnprocessableEntity, errors.New("repo is empty"))
+	if ctx.Written() {
 		return
 	}
-
-	if apiOpts.BranchName == "" {
-		apiOpts.BranchName = ctx.Repo.Repository.DefaultBranch
-	}
-
 	contentReader, err := base64Reader(apiOpts.ContentBase64)
 	if err != nil {
 		ctx.APIError(http.StatusUnprocessableEntity, err)
 		return
 	}
-
+	opts.Files = append(opts.Files, &files_service.ChangeRepoFile{
-	opts := &files_service.ChangeRepoFilesOptions{
+		Operation:     "update",
-		Files: []*files_service.ChangeRepoFile{
+		ContentReader: contentReader,
-			{
+		SHA:           apiOpts.SHA,
-				Operation:     "update",
+		FromTreePath:  apiOpts.FromPath,
-				ContentReader: contentReader,
+		TreePath:      ctx.PathParam("*"),
-				SHA:           apiOpts.SHA,
+	})
-				FromTreePath:  apiOpts.FromPath,
-				TreePath:      ctx.PathParam("*"),
-			},
-		},
-		Message:   apiOpts.Message,
-		OldBranch: apiOpts.BranchName,
-		NewBranch: apiOpts.NewBranchName,
-		Committer: &files_service.IdentityOptions{
-			GitUserName:  apiOpts.Committer.Name,
-			GitUserEmail: apiOpts.Committer.Email,
-		},
-		Author: &files_service.IdentityOptions{
-			GitUserName:  apiOpts.Author.Name,
-			GitUserEmail: apiOpts.Author.Email,
-		},
-		Dates: &files_service.CommitDateOptions{
-			Author:    apiOpts.Dates.Author,
-			Committer: apiOpts.Dates.Committer,
-		},
-		Signoff: apiOpts.Signoff,
-	}
-	if opts.Dates.Author.IsZero() {
-		opts.Dates.Author = time.Now()
-	}
-	if opts.Dates.Committer.IsZero() {
-		opts.Dates.Committer = time.Now()
-	}
-
 	if opts.Message == "" {
 		opts.Message = changeFilesCommitMessage(ctx, opts.Files)
 	}
 
-	if filesResponse, err := createOrUpdateFiles(ctx, opts); err != nil {
+	if filesResponse, err := files_service.ChangeRepoFiles(ctx, ctx.Repo.Repository, ctx.Doer, opts); err != nil {
-		handleCreateOrUpdateFileError(ctx, err)
+		handleChangeRepoFilesError(ctx, err)
 	} else {
 		fileResponse := files_service.GetFileResponseFromFilesResponse(filesResponse, 0)
 		ctx.JSON(http.StatusOK, fileResponse)
 	}
 }
 
-func handleCreateOrUpdateFileError(ctx *context.APIContext, err error) {
+func handleChangeRepoFilesError(ctx *context.APIContext, err error) {
 	if files_service.IsErrUserCannotCommit(err) || pull_service.IsErrFilePathProtected(err) {
 		ctx.APIError(http.StatusForbidden, err)
 		return
 	}
 	if git_model.IsErrBranchAlreadyExists(err) || files_service.IsErrFilenameInvalid(err) || pull_service.IsErrSHADoesNotMatch(err) ||
-		files_service.IsErrFilePathInvalid(err) || files_service.IsErrRepoFileAlreadyExists(err) {
+		files_service.IsErrFilePathInvalid(err) || files_service.IsErrRepoFileAlreadyExists(err) ||
+		files_service.IsErrCommitIDDoesNotMatch(err) || files_service.IsErrSHAOrCommitIDNotProvided(err) {
 		ctx.APIError(http.StatusUnprocessableEntity, err)
 		return
 	}
-	if git_model.IsErrBranchNotExist(err) || git.IsErrBranchNotExist(err) {
+	if git.IsErrBranchNotExist(err) || files_service.IsErrRepoFileDoesNotExist(err) || git.IsErrNotExist(err) {
 		ctx.APIError(http.StatusNotFound, err)
 		return
 	}
-
+	if errors.Is(err, util.ErrNotExist) {
-	ctx.APIErrorInternal(err)
+		ctx.APIError(http.StatusNotFound, err)
-}
+		return
-
-// Called from both CreateFile or UpdateFile to handle both
-func createOrUpdateFiles(ctx *context.APIContext, opts *files_service.ChangeRepoFilesOptions) (*api.FilesResponse, error) {
-	if !canWriteFiles(ctx, opts.OldBranch) {
-		return nil, repo_model.ErrUserDoesNotHaveAccessToRepo{
-			UserID:   ctx.Doer.ID,
-			RepoName: ctx.Repo.Repository.LowerName,
-		}
 	}
-
+	ctx.APIErrorInternal(err)
-	return files_service.ChangeRepoFiles(ctx, ctx.Repo.Repository, ctx.Doer, opts)
 }
 
 // format commit message if empty
@@ -762,7 +693,7 @@ func changeFilesCommitMessage(ctx *context.APIContext, files []*files_service.Ch
 		switch file.Operation {
 		case "create":
 			createFiles = append(createFiles, file.TreePath)
-		case "update":
+		case "update", "upload", "rename": // upload and rename works like "update", there is no translation for them at the moment
 			updateFiles = append(updateFiles, file.TreePath)
 		case "delete":
 			deleteFiles = append(deleteFiles, file.TreePath)
@@ -820,74 +751,27 @@ func DeleteFile(ctx *context.APIContext) {
 	//     "$ref": "#/responses/error"
 	//   "404":
 	//     "$ref": "#/responses/error"
+	//   "422":
+	//     "$ref": "#/responses/error"
 	//   "423":
 	//     "$ref": "#/responses/repoArchivedError"
 
-	apiOpts := web.GetForm(ctx).(*api.DeleteFileOptions)
+	apiOpts, opts := getAPIChangeRepoFileOptions[*api.DeleteFileOptions](ctx)
-	if !canWriteFiles(ctx, apiOpts.BranchName) {
+	if ctx.Written() {
-		ctx.APIError(http.StatusForbidden, repo_model.ErrUserDoesNotHaveAccessToRepo{
-			UserID:   ctx.Doer.ID,
-			RepoName: ctx.Repo.Repository.LowerName,
-		})
 		return
 	}
 
-	if apiOpts.BranchName == "" {
+	opts.Files = append(opts.Files, &files_service.ChangeRepoFile{
-		apiOpts.BranchName = ctx.Repo.Repository.DefaultBranch
+		Operation: "delete",
-	}
+		SHA:       apiOpts.SHA,
-
+		TreePath:  ctx.PathParam("*"),
-	opts := &files_service.ChangeRepoFilesOptions{
+	})
-		Files: []*files_service.ChangeRepoFile{
-			{
-				Operation: "delete",
-				SHA:       apiOpts.SHA,
-				TreePath:  ctx.PathParam("*"),
-			},
-		},
-		Message:   apiOpts.Message,
-		OldBranch: apiOpts.BranchName,
-		NewBranch: apiOpts.NewBranchName,
-		Committer: &files_service.IdentityOptions{
-			GitUserName:  apiOpts.Committer.Name,
-			GitUserEmail: apiOpts.Committer.Email,
-		},
-		Author: &files_service.IdentityOptions{
-			GitUserName:  apiOpts.Author.Name,
-			GitUserEmail: apiOpts.Author.Email,
-		},
-		Dates: &files_service.CommitDateOptions{
-			Author:    apiOpts.Dates.Author,
-			Committer: apiOpts.Dates.Committer,
-		},
-		Signoff: apiOpts.Signoff,
-	}
-	if opts.Dates.Author.IsZero() {
-		opts.Dates.Author = time.Now()
-	}
-	if opts.Dates.Committer.IsZero() {
-		opts.Dates.Committer = time.Now()
-	}
-
 	if opts.Message == "" {
 		opts.Message = changeFilesCommitMessage(ctx, opts.Files)
 	}
 
 	if filesResponse, err := files_service.ChangeRepoFiles(ctx, ctx.Repo.Repository, ctx.Doer, opts); err != nil {
-		if git.IsErrBranchNotExist(err) || files_service.IsErrRepoFileDoesNotExist(err) || git.IsErrNotExist(err) {
+		handleChangeRepoFilesError(ctx, err)
-			ctx.APIError(http.StatusNotFound, err)
-			return
-		} else if git_model.IsErrBranchAlreadyExists(err) ||
-			files_service.IsErrFilenameInvalid(err) ||
-			pull_service.IsErrSHADoesNotMatch(err) ||
-			files_service.IsErrCommitIDDoesNotMatch(err) ||
-			files_service.IsErrSHAOrCommitIDNotProvided(err) {
-			ctx.APIError(http.StatusBadRequest, err)
-			return
-		} else if files_service.IsErrUserCannotCommit(err) {
-			ctx.APIError(http.StatusForbidden, err)
-			return
-		}
-		ctx.APIErrorInternal(err)
 	} else {
 		fileResponse := files_service.GetFileResponseFromFilesResponse(filesResponse, 0)
 		ctx.JSON(http.StatusOK, fileResponse) // FIXME on APIv2: return http.StatusNoContent
@@ -911,6 +795,8 @@ func GetContentsExt(ctx *context.APIContext) {
 	// summary: The extended "contents" API, to get file metadata and/or content, or list a directory.
 	// description: It guarantees that only one of the response fields is set if the request succeeds.
 	//              Users can pass "includes=file_content" or "includes=lfs_metadata" to retrieve more fields.
+	//              "includes=file_content" only works for single file, if you need to retrieve file contents in batch,
+	//              use "file-contents" API after listing the directory.
 	// produces:
 	// - application/json
 	// parameters:
@@ -964,12 +850,11 @@ func GetContentsExt(ctx *context.APIContext) {
 	ctx.JSON(http.StatusOK, getRepoContents(ctx, opts))
 }
 
-// GetContents Get the metadata and contents (if a file) of an entry in a repository, or a list of entries if a dir
 func GetContents(ctx *context.APIContext) {
 	// swagger:operation GET /repos/{owner}/{repo}/contents/{filepath} repository repoGetContents
 	// ---
 	// summary: Gets the metadata and contents (if a file) of an entry in a repository, or a list of entries if a dir.
-	// description: This API follows GitHub's design, and it is not easy to use. Recommend to use our "contents-ext" API instead.
+	// description: This API follows GitHub's design, and it is not easy to use. Recommend users to use the "contents-ext" API instead.
 	// produces:
 	// - application/json
 	// parameters:
@@ -1021,12 +906,11 @@ func getRepoContents(ctx *context.APIContext, opts files_service.GetContentsOrLi
 	return &ret
 }
 
-// GetContentsList Get the metadata of all the entries of the root dir
 func GetContentsList(ctx *context.APIContext) {
 	// swagger:operation GET /repos/{owner}/{repo}/contents repository repoGetContentsList
 	// ---
 	// summary: Gets the metadata of all the entries of the root dir.
-	// description: This API follows GitHub's design, and it is not easy to use. Recommend to use our "contents-ext" API instead.
+	// description: This API follows GitHub's design, and it is not easy to use. Recommend users to use our "contents-ext" API instead.
 	// produces:
 	// - application/json
 	// parameters:
@@ -1059,7 +943,7 @@ func GetFileContentsGet(ctx *context.APIContext) {
 	// swagger:operation GET /repos/{owner}/{repo}/file-contents repository repoGetFileContents
 	// ---
 	// summary: Get the metadata and contents of requested files
-	// description: See the POST method. This GET method supports to use JSON encoded request body in query parameter.
+	// description: See the POST method. This GET method supports using JSON encoded request body in query parameter.
 	// produces:
 	// - application/json
 	// parameters:
@@ -1089,7 +973,7 @@ func GetFileContentsGet(ctx *context.APIContext) {
 	//   "404":
 	//     "$ref": "#/responses/notFound"
 
-	// POST method requires "write" permission, so we also support this "GET" method
+	// The POST method requires "write" permission, so we also support this "GET" method
 	handleGetFileContents(ctx)
 }
 
@@ -1133,7 +1017,7 @@ func GetFileContentsPost(ctx *context.APIContext) {
 
 	// This is actually a "read" request, but we need to accept a "files" list, then POST method seems easy to use.
 	// But the permission system requires that the caller must have "write" permission to use POST method.
-	// At the moment there is no other way to get around the permission check, so there is a "GET" workaround method above.
+	// At the moment, there is no other way to get around the permission check, so there is a "GET" workaround method above.
 	handleGetFileContents(ctx)
 }
 

routers/api/v1/repo/patch.go

@@ -5,15 +5,10 @@ package repo
 
 import (
 	"net/http"
-	"time"
 
-	git_model "code.gitea.io/gitea/models/git"
-	repo_model "code.gitea.io/gitea/models/repo"
-	"code.gitea.io/gitea/modules/git"
 	api "code.gitea.io/gitea/modules/structs"
-	"code.gitea.io/gitea/modules/web"
+	"code.gitea.io/gitea/modules/util"
 	"code.gitea.io/gitea/services/context"
-	pull_service "code.gitea.io/gitea/services/pull"
 	"code.gitea.io/gitea/services/repository/files"
 )
 
@@ -49,63 +44,22 @@ func ApplyDiffPatch(ctx *context.APIContext) {
 	//     "$ref": "#/responses/notFound"
 	//   "423":
 	//     "$ref": "#/responses/repoArchivedError"
-	apiOpts := web.GetForm(ctx).(*api.ApplyDiffPatchFileOptions)
+	apiOpts, changeRepoFileOpts := getAPIChangeRepoFileOptions[*api.ApplyDiffPatchFileOptions](ctx)
-
 	opts := &files.ApplyDiffPatchOptions{
-		Content:   apiOpts.Content,
+		Content: apiOpts.Content,
-		SHA:       apiOpts.SHA,
+		Message: util.IfZero(apiOpts.Message, "apply-patch"),
-		Message:   apiOpts.Message,
-		OldBranch: apiOpts.BranchName,
-		NewBranch: apiOpts.NewBranchName,
-		Committer: &files.IdentityOptions{
-			GitUserName:  apiOpts.Committer.Name,
-			GitUserEmail: apiOpts.Committer.Email,
-		},
-		Author: &files.IdentityOptions{
-			GitUserName:  apiOpts.Author.Name,
-			GitUserEmail: apiOpts.Author.Email,
-		},
-		Dates: &files.CommitDateOptions{
-			Author:    apiOpts.Dates.Author,
-			Committer: apiOpts.Dates.Committer,
-		},
-		Signoff: apiOpts.Signoff,
-	}
-	if opts.Dates.Author.IsZero() {
-		opts.Dates.Author = time.Now()
-	}
-	if opts.Dates.Committer.IsZero() {
-		opts.Dates.Committer = time.Now()
-	}
 
-	if opts.Message == "" {
+		OldBranch: changeRepoFileOpts.OldBranch,
-		opts.Message = "apply-patch"
+		NewBranch: changeRepoFileOpts.NewBranch,
-	}
+		Committer: changeRepoFileOpts.Committer,
-
+		Author:    changeRepoFileOpts.Author,
-	if !canWriteFiles(ctx, apiOpts.BranchName) {
+		Dates:     changeRepoFileOpts.Dates,
-		ctx.APIErrorInternal(repo_model.ErrUserDoesNotHaveAccessToRepo{
+		Signoff:   changeRepoFileOpts.Signoff,
-			UserID:   ctx.Doer.ID,
-			RepoName: ctx.Repo.Repository.LowerName,
-		})
-		return
 	}
 
 	fileResponse, err := files.ApplyDiffPatch(ctx, ctx.Repo.Repository, ctx.Doer, opts)
 	if err != nil {
-		if files.IsErrUserCannotCommit(err) || pull_service.IsErrFilePathProtected(err) {
+		handleChangeRepoFilesError(ctx, err)
-			ctx.APIError(http.StatusForbidden, err)
-			return
-		}
-		if git_model.IsErrBranchAlreadyExists(err) || files.IsErrFilenameInvalid(err) || pull_service.IsErrSHADoesNotMatch(err) ||
-			files.IsErrFilePathInvalid(err) || files.IsErrRepoFileAlreadyExists(err) {
-			ctx.APIError(http.StatusUnprocessableEntity, err)
-			return
-		}
-		if git_model.IsErrBranchNotExist(err) || git.IsErrBranchNotExist(err) {
-			ctx.APIError(http.StatusNotFound, err)
-			return
-		}
-		ctx.APIErrorInternal(err)
 	} else {
 		ctx.JSON(http.StatusCreated, fileResponse)
 	}

routers/web/repo/editor.go

@@ -12,6 +12,7 @@ import (
 	"strings"
 
 	git_model "code.gitea.io/gitea/models/git"
+	"code.gitea.io/gitea/models/issues"
 	"code.gitea.io/gitea/models/unit"
 	"code.gitea.io/gitea/modules/charset"
 	"code.gitea.io/gitea/modules/git"
@@ -138,6 +139,11 @@ func prepareEditorCommitSubmittedForm[T forms.CommitCommonFormInterface](ctx *co
 		return nil
 	}
 
+	if !issues.CanMaintainerWriteToBranch(ctx, ctx.Repo.Permission, targetBranchName, ctx.Doer) {
+		ctx.NotFound(nil)
+		return nil
+	}
+
 	// Committer user info
 	gitCommitter, valid := WebGitOperationGetCommitChosenEmailIdentity(ctx, commonForm.CommitEmail)
 	if !valid {

services/repository/files/patch.go

@@ -44,7 +44,6 @@ type ApplyDiffPatchOptions struct {
 	NewBranch    string
 	Message      string
 	Content      string
-	SHA          string
 	Author       *IdentityOptions
 	Committer    *IdentityOptions
 	Dates        *CommitDateOptions

services/repository/files/update.go

@@ -113,7 +113,7 @@ func ChangeRepoFiles(ctx context.Context, repo *repo_model.Repository, doer *use
 		return nil, err
 	}
 
-	// If no branch name is set, assume default branch
+	// If no branch name is set, assume the default branch
 	if opts.OldBranch == "" {
 		opts.OldBranch = repo.DefaultBranch
 	}

templates/swagger/v1_json.tmpl

@@ -7424,7 +7424,7 @@
     },
     "/repos/{owner}/{repo}/contents": {
       "get": {
-        "description": "This API follows GitHub's design, and it is not easy to use. Recommend to use our \"contents-ext\" API instead.",
+        "description": "This API follows GitHub's design, and it is not easy to use. Recommend users to use our \"contents-ext\" API instead.",
         "produces": [
           "application/json"
         ],
@@ -7521,7 +7521,7 @@
     },
     "/repos/{owner}/{repo}/contents-ext/{filepath}": {
       "get": {
-        "description": "It guarantees that only one of the response fields is set if the request succeeds. Users can pass \"includes=file_content\" or \"includes=lfs_metadata\" to retrieve more fields.",
+        "description": "It guarantees that only one of the response fields is set if the request succeeds. Users can pass \"includes=file_content\" or \"includes=lfs_metadata\" to retrieve more fields. \"includes=file_content\" only works for single file, if you need to retrieve file contents in batch, use \"file-contents\" API after listing the directory.",
         "produces": [
           "application/json"
         ],
@@ -7577,7 +7577,7 @@
     },
     "/repos/{owner}/{repo}/contents/{filepath}": {
       "get": {
-        "description": "This API follows GitHub's design, and it is not easy to use. Recommend to use our \"contents-ext\" API instead.",
+        "description": "This API follows GitHub's design, and it is not easy to use. Recommend users to use the \"contents-ext\" API instead.",
         "produces": [
           "application/json"
         ],
@@ -7802,6 +7802,9 @@
           "404": {
             "$ref": "#/responses/error"
           },
+          "422": {
+            "$ref": "#/responses/error"
+          },
           "423": {
             "$ref": "#/responses/repoArchivedError"
           }
@@ -7909,7 +7912,7 @@
     },
     "/repos/{owner}/{repo}/file-contents": {
       "get": {
-        "description": "See the POST method. This GET method supports to use JSON encoded request body in query parameter.",
+        "description": "See the POST method. This GET method supports using JSON encoded request body in query parameter.",
         "produces": [
           "application/json"
         ],
@@ -12876,7 +12879,7 @@
           },
           {
             "type": "string",
-            "description": "The name of the commit/branch/tag. Default the repository’s default branch",
+            "description": "The name of the commit/branch/tag. Default to the repository’s default branch",
             "name": "ref",
             "in": "query"
           }
@@ -15020,7 +15023,7 @@
           },
           {
             "type": "string",
-            "description": "The name of the commit/branch/tag. Default the repository’s default branch",
+            "description": "The name of the commit/branch/tag. Default to the repository’s default branch",
             "name": "ref",
             "in": "query"
           }
@@ -21867,7 +21870,7 @@
       ],
       "properties": {
         "content": {
-          "description": "new or updated file content, must be base64 encoded",
+          "description": "new or updated file content, it must be base64 encoded",
           "type": "string",
           "x-go-name": "ContentBase64"
         },
@@ -21877,11 +21880,13 @@
           "x-go-name": "FromPath"
         },
         "operation": {
-          "description": "indicates what to do with the file",
+          "description": "indicates what to do with the file: \"create\" for creating a new file, \"update\" for updating an existing file,\n\"upload\" for creating or updating a file, \"rename\" for renaming a file, and \"delete\" for deleting an existing file.",
           "type": "string",
           "enum": [
             "create",
             "update",
+            "upload",
+            "rename",
             "delete"
           ],
           "x-go-name": "Operation"
@@ -21892,7 +21897,7 @@
           "x-go-name": "Path"
         },
         "sha": {
-          "description": "sha is the SHA for the file that already exists, required for update or delete",
+          "description": "the blob ID (SHA) for the file that already exists, required for changing existing files",
           "type": "string",
           "x-go-name": "SHA"
         }
@@ -23657,7 +23662,7 @@
           "x-go-name": "NewBranchName"
         },
         "sha": {
-          "description": "sha is the SHA for the file that already exists",
+          "description": "the blob ID (SHA) for the file that already exists, it is required for changing existing files",
           "type": "string",
           "x-go-name": "SHA"
         },
@@ -28106,7 +28111,7 @@
           "x-go-name": "NewBranchName"
         },
         "sha": {
-          "description": "sha is the SHA for the file that already exists",
+          "description": "the blob ID (SHA) for the file that already exists, it is required for changing existing files",
           "type": "string",
           "x-go-name": "SHA"
         },

tests/integration/api_repo_file_delete_test.go

@@ -20,20 +20,22 @@ import (
 
 func getDeleteFileOptions() *api.DeleteFileOptions {
 	return &api.DeleteFileOptions{
-		FileOptions: api.FileOptions{
+		FileOptionsWithSHA: api.FileOptionsWithSHA{
-			BranchName:    "master",
+			FileOptions: api.FileOptions{
-			NewBranchName: "master",
+				BranchName:    "master",
-			Message:       "Removing the file new/file.txt",
+				NewBranchName: "master",
-			Author: api.Identity{
+				Message:       "Removing the file new/file.txt",
-				Name:  "John Doe",
+				Author: api.Identity{
-				Email: "johndoe@example.com",
+					Name:  "John Doe",
-			},
+					Email: "johndoe@example.com",
-			Committer: api.Identity{
+				},
-				Name:  "Jane Doe",
+				Committer: api.Identity{
-				Email: "janedoe@example.com",
+					Name:  "Jane Doe",
+					Email: "janedoe@example.com",
+				},
 			},
+			SHA: "103ff9234cefeee5ec5361d22b49fbb04d385885",
 		},
-		SHA: "103ff9234cefeee5ec5361d22b49fbb04d385885",
 	}
 }
 
@@ -110,7 +112,7 @@ func TestAPIDeleteFile(t *testing.T) {
 		deleteFileOptions.SHA = "badsha"
 		req = NewRequestWithJSON(t, "DELETE", fmt.Sprintf("/api/v1/repos/%s/%s/contents/%s", user2.Name, repo1.Name, treePath), &deleteFileOptions).
 			AddTokenAuth(token2)
-		MakeRequest(t, req, http.StatusBadRequest)
+		MakeRequest(t, req, http.StatusUnprocessableEntity)
 
 		// Test creating a file in repo16 by user4 who does not have write access
 		fileID++

tests/integration/api_repo_file_update_test.go

@@ -27,7 +27,7 @@ func getUpdateFileOptions() *api.UpdateFileOptions {
 	content := "This is updated text"
 	contentEncoded := base64.StdEncoding.EncodeToString([]byte(content))
 	return &api.UpdateFileOptions{
-		DeleteFileOptions: api.DeleteFileOptions{
+		FileOptionsWithSHA: api.FileOptionsWithSHA{
 			FileOptions: api.FileOptions{
 				BranchName:    "master",
 				NewBranchName: "master",