Refactor some file edit related code (#34744)

Follow up #34350

---------

Co-authored-by: delvh <dev.lh@web.de>

8 files changed, 224 insertions, 225 deletions

diff --git a/modules/optional/option.go b/modules/optional/option.go
index ccbad259c2..6075c6347e 100644
--- a/modules/optional/option.go
+++ b/modules/optional/option.go
@@ -5,6 +5,12 @@ package optional
 
 import "strconv"
 
+// Option is a generic type that can hold a value of type T or be empty (None).
+//
+// It must use the slice type to work with "chi" form values binding:
+// * non-existing value are represented as an empty slice (None)
+// * existing value is represented as a slice with one element (Some)
+// * multiple values are represented as a slice with multiple elements (Some), the Value is the first element (not well-defined in this case)
 type Option[T any] []T
 
 func None[T any]() Option[T] {
diff --git a/routers/web/repo/editor.go b/routers/web/repo/editor.go
index 62bf8b182f..6aea152d23 100644
--- a/routers/web/repo/editor.go
+++ b/routers/web/repo/editor.go
@@ -288,13 +288,20 @@ func editFilePost(ctx *context.Context, form forms.EditRepoFileForm, isNewFile b
 		return
 	}
 
-	operation := "update"
+	var operation string
 	if isNewFile {
 		operation = "create"
-	} else if !form.Content.Has() && ctx.Repo.TreePath != form.TreePath {
-		// The form content only has data if file is representable as text, is not too large and not in lfs. If it doesn't
-		// have data, the only possible operation is a rename
+	} else if form.Content.Has() {
+		// The form content only has data if the file is representable as text, is not too large and not in lfs.
+		operation = "update"
+	} else if ctx.Repo.TreePath != form.TreePath {
+		// If it doesn't have data, the only possible operation is a "rename"
 		operation = "rename"
+	} else {
+		// It should never happen, just in case
+		ctx.Flash.Error(ctx.Tr("error.occurred"))
+		ctx.HTML(http.StatusOK, tplEditFile)
+		return
 	}
 
 	if _, err := files_service.ChangeRepoFiles(ctx, ctx.Repo.Repository, ctx.Doer, &files_service.ChangeRepoFilesOptions{
diff --git a/services/packages/cargo/index.go b/services/packages/cargo/index.go
index e8a2f189c8..605335d0f1 100644
--- a/services/packages/cargo/index.go
+++ b/services/packages/cargo/index.go
@@ -310,7 +310,7 @@ func alterRepositoryContent(ctx context.Context, doer *user_model.User, repo *re
 }
 
 func writeObjectToIndex(ctx context.Context, t *files_service.TemporaryUploadRepository, path string, r io.Reader) error {
-	hash, err := t.HashObject(ctx, r)
+	hash, err := t.HashObjectAndWrite(ctx, r)
 	if err != nil {
 		return err
 	}
diff --git a/services/repository/files/diff.go b/services/repository/files/diff.go
index 0b3550452a..50d01f9d7c 100644
--- a/services/repository/files/diff.go
+++ b/services/repository/files/diff.go
@@ -29,7 +29,7 @@ func GetDiffPreview(ctx context.Context, repo *repo_model.Repository, branch, tr
 	}
 
 	// Add the object to the database
-	objectHash, err := t.HashObject(ctx, strings.NewReader(content))
+	objectHash, err := t.HashObjectAndWrite(ctx, strings.NewReader(content))
 	if err != nil {
 		return nil, err
 	}
diff --git a/services/repository/files/temp_repo.go b/services/repository/files/temp_repo.go
index 1cf30edc7b..7009be3d4e 100644
--- a/services/repository/files/temp_repo.go
+++ b/services/repository/files/temp_repo.go
@@ -164,8 +164,8 @@ func (t *TemporaryUploadRepository) RemoveFilesFromIndex(ctx context.Context, fi
 	return nil
 }
 
-// HashObject writes the provided content to the object db and returns its hash
-func (t *TemporaryUploadRepository) HashObject(ctx context.Context, content io.Reader) (string, error) {
+// HashObjectAndWrite writes the provided content to the object db and returns its hash
+func (t *TemporaryUploadRepository) HashObjectAndWrite(ctx context.Context, content io.Reader) (string, error) {
 	stdOut := new(bytes.Buffer)
 	stdErr := new(bytes.Buffer)
 
diff --git a/services/repository/files/update.go b/services/repository/files/update.go
index e1acf6a92f..07d89b56a4 100644
--- a/services/repository/files/update.go
+++ b/services/repository/files/update.go
@@ -225,7 +225,7 @@ func ChangeRepoFiles(ctx context.Context, repo *repo_model.Repository, doer *use
 			return nil, err // Couldn't get a commit for the branch
 		}
 
-		// Assigned LastCommitID in opts if it hasn't been set
+		// Assigned LastCommitID in "opts" if it hasn't been set
 		if opts.LastCommitID == "" {
 			opts.LastCommitID = commit.ID.String()
 		} else {
@@ -237,22 +237,21 @@ func ChangeRepoFiles(ctx context.Context, repo *repo_model.Repository, doer *use
 		}
 
 		for _, file := range opts.Files {
-			if err := handleCheckErrors(file, commit, opts); err != nil {
+			if err = handleCheckErrors(file, commit, opts); err != nil {
 				return nil, err
 			}
 		}
 	}
 
-	contentStore := lfs.NewContentStore()
+	lfsContentStore := lfs.NewContentStore()
 	for _, file := range opts.Files {
 		switch file.Operation {
 		case "create", "update", "rename":
-			if err := CreateOrUpdateFile(ctx, t, file, contentStore, repo.ID, hasOldBranch); err != nil {
+			if err = CreateUpdateRenameFile(ctx, t, file, lfsContentStore, repo.ID, hasOldBranch); err != nil {
 				return nil, err
 			}
 		case "delete":
-			// Remove the file from the index
-			if err := t.RemoveFilesFromIndex(ctx, file.TreePath); err != nil {
+			if err = t.RemoveFilesFromIndex(ctx, file.TreePath); err != nil {
 				return nil, err
 			}
 		default:
@@ -372,13 +371,13 @@ func (err ErrSHAOrCommitIDNotProvided) Error() string {
 
 // handles the check for various issues for ChangeRepoFiles
 func handleCheckErrors(file *ChangeRepoFile, commit *git.Commit, opts *ChangeRepoFilesOptions) error {
-	if file.Operation == "update" || file.Operation == "delete" {
+	if file.Operation == "update" || file.Operation == "delete" || file.Operation == "rename" {
 		fromEntry, err := commit.GetTreeEntryByPath(file.Options.fromTreePath)
 		if err != nil {
 			return err
 		}
 		if file.SHA != "" {
-			// If a SHA was given and the SHA given doesn't match the SHA of the fromTreePath, throw error
+			// If the SHA given doesn't match the SHA of the fromTreePath, throw error
 			if file.SHA != fromEntry.ID.String() {
 				return pull_service.ErrSHADoesNotMatch{
 					Path:       file.Options.treePath,
@@ -387,7 +386,7 @@ func handleCheckErrors(file *ChangeRepoFile, commit *git.Commit, opts *ChangeRep
 				}
 			}
 		} else if opts.LastCommitID != "" {
-			// If a lastCommitID was given and it doesn't match the commitID of the head of the branch throw
+			// If a lastCommitID given doesn't match the branch head's commitID 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 {
@@ -405,13 +404,14 @@ func handleCheckErrors(file *ChangeRepoFile, commit *git.Commit, opts *ChangeRep
 			// haven't been made. We throw an error if one wasn't provided.
 			return ErrSHAOrCommitIDNotProvided{}
 		}
+		// FIXME: legacy hacky approach, it shouldn't prepare the "Options" in the "check" function
 		file.Options.executable = fromEntry.IsExecutable()
 	}
-	if file.Operation == "create" || file.Operation == "update" {
-		// For the path where this file will be created/updated, we need to make
-		// sure no parts of the path are existing files or links except for the last
-		// item in the path which is the file name, and that shouldn't exist IF it is
-		// a new file OR is being moved to a new path.
+
+	if file.Operation == "create" || file.Operation == "update" || file.Operation == "rename" {
+		// For operation's target path, we need to make sure no parts of the path are existing files or links
+		// except for the last item in the path (which is the file name).
+		// And that shouldn't exist IF it is a new file OR is being moved to a new path.
 		treePathParts := strings.Split(file.Options.treePath, "/")
 		subTreePath := ""
 		for index, part := range treePathParts {
@@ -448,7 +448,7 @@ func handleCheckErrors(file *ChangeRepoFile, commit *git.Commit, opts *ChangeRep
 					Type:    git.EntryModeTree,
 				}
 			} else if file.Options.fromTreePath != file.Options.treePath || file.Operation == "create" {
-				// The entry shouldn't exist if we are creating new file or moving to a new path
+				// The entry shouldn't exist if we are creating the new file or moving to a new path
 				return ErrRepoFileAlreadyExists{
 					Path: file.Options.treePath,
 				}
@@ -459,8 +459,7 @@ func handleCheckErrors(file *ChangeRepoFile, commit *git.Commit, opts *ChangeRep
 	return nil
 }
 
-// CreateOrUpdateFile handles creating or updating a file for ChangeRepoFiles
-func CreateOrUpdateFile(ctx context.Context, t *TemporaryUploadRepository, file *ChangeRepoFile, contentStore *lfs.ContentStore, repoID int64, hasOldBranch bool) error {
+func CreateUpdateRenameFile(ctx context.Context, t *TemporaryUploadRepository, file *ChangeRepoFile, contentStore *lfs.ContentStore, repoID int64, hasOldBranch bool) error {
 	// Get the two paths (might be the same if not moving) from the index if they exist
 	filesInIndex, err := t.LsFiles(ctx, file.TreePath, file.FromTreePath)
 	if err != nil {
@@ -481,181 +480,177 @@ func CreateOrUpdateFile(ctx context.Context, t *TemporaryUploadRepository, file
 	if file.Options.fromTreePath != file.Options.treePath && len(filesInIndex) > 0 {
 		for _, indexFile := range filesInIndex {
 			if indexFile == file.Options.fromTreePath {
-				if err := t.RemoveFilesFromIndex(ctx, file.FromTreePath); err != nil {
+				if err = t.RemoveFilesFromIndex(ctx, file.FromTreePath); err != nil {
 					return err
 				}
 			}
 		}
 	}
 
-	var oldEntry *git.TreeEntry
-	// Assume that the file.ContentReader of a pure rename operation is invalid. Use the file content how it's present in
-	// git instead
-	if file.Operation == "rename" {
-		lastCommitID, err := t.GetLastCommit(ctx)
-		if err != nil {
-			return err
-		}
-		commit, err := t.GetCommit(lastCommitID)
-		if err != nil {
-			return err
-		}
-
-		if oldEntry, err = commit.GetTreeEntryByPath(file.Options.fromTreePath); err != nil {
-			return err
-		}
-	}
-
-	var objectHash string
-	var lfsPointer *lfs.Pointer
+	var writeObjectRet *writeRepoObjectRet
 	switch file.Operation {
 	case "create", "update":
-		objectHash, lfsPointer, err = createOrUpdateFileHash(ctx, t, file, hasOldBranch)
+		writeObjectRet, err = writeRepoObjectForCreateOrUpdate(ctx, t, file)
 	case "rename":
-		objectHash, lfsPointer, err = renameFileHash(ctx, t, oldEntry, file)
+		writeObjectRet, err = writeRepoObjectForRename(ctx, t, file)
+	default:
+		return util.NewInvalidArgumentErrorf("unknown file modification operation: '%s'", file.Operation)
 	}
 	if err != nil {
 		return err
 	}
 
-	// Add the object to the index
-	if file.Options.executable {
-		if err := t.AddObjectToIndex(ctx, "100755", objectHash, file.Options.treePath); err != nil {
-			return err
-		}
-	} else {
-		if err := t.AddObjectToIndex(ctx, "100644", objectHash, file.Options.treePath); err != nil {
-			return err
-		}
+	// Add the object to the index, the "file.Options.executable" is set in handleCheckErrors by the caller (legacy hacky approach)
+	if err = t.AddObjectToIndex(ctx, util.Iif(file.Options.executable, "100755", "100644"), writeObjectRet.ObjectHash, file.Options.treePath); err != nil {
+		return err
 	}
 
-	if lfsPointer != nil {
-		// We have an LFS object - create it
-		lfsMetaObject, err := git_model.NewLFSMetaObject(ctx, repoID, *lfsPointer)
-		if err != nil {
-			return err
-		}
-		exist, err := contentStore.Exists(lfsMetaObject.Pointer)
-		if err != nil {
-			return err
-		}
-		if !exist {
-			var lfsContentReader io.Reader
-			if file.Operation != "rename" {
-				if _, err := file.ContentReader.Seek(0, io.SeekStart); err != nil {
-					return err
-				}
-				lfsContentReader = file.ContentReader
-			} else {
-				if lfsContentReader, err = oldEntry.Blob().DataAsync(); err != nil {
-					return err
-				}
-				defer lfsContentReader.(io.ReadCloser).Close()
-			}
+	if writeObjectRet.LfsContent == nil {
+		return nil // No LFS pointer, so nothing to do
+	}
+	defer writeObjectRet.LfsContent.Close()
 
-			if err := contentStore.Put(lfsMetaObject.Pointer, lfsContentReader); err != nil {
-				if _, err2 := git_model.RemoveLFSMetaObjectByOid(ctx, repoID, lfsMetaObject.Oid); err2 != nil {
-					return fmt.Errorf("unable to remove failed inserted LFS object %s: %v (Prev Error: %w)", lfsMetaObject.Oid, err2, err)
-				}
-				return err
-			}
+	// Now we must store the content into an LFS object
+	lfsMetaObject, err := git_model.NewLFSMetaObject(ctx, repoID, writeObjectRet.LfsPointer)
+	if err != nil {
+		return err
+	}
+	if exist, err := contentStore.Exists(lfsMetaObject.Pointer); err != nil {
+		return err
+	} else if exist {
+		return nil
+	}
+
+	err = contentStore.Put(lfsMetaObject.Pointer, writeObjectRet.LfsContent)
+	if err != nil {
+		if _, errRemove := git_model.RemoveLFSMetaObjectByOid(ctx, repoID, lfsMetaObject.Oid); errRemove != nil {
+			return fmt.Errorf("unable to remove failed inserted LFS object %s: %v (Prev Error: %w)", lfsMetaObject.Oid, errRemove, err)
 		}
 	}
+	return err
+}
 
-	return nil
+func checkIsLfsFileInGitAttributes(ctx context.Context, t *TemporaryUploadRepository, paths []string) (ret []bool, err error) {
+	attributesMap, err := attribute.CheckAttributes(ctx, t.gitRepo, "" /* use temp repo's working dir */, attribute.CheckAttributeOpts{
+		Attributes: []string{attribute.Filter},
+		Filenames:  paths,
+	})
+	if err != nil {
+		return nil, err
+	}
+	for _, p := range paths {
+		isLFSFile := attributesMap[p] != nil && attributesMap[p].Get(attribute.Filter).ToString().Value() == "lfs"
+		ret = append(ret, isLFSFile)
+	}
+	return ret, nil
 }
 
-func createOrUpdateFileHash(ctx context.Context, t *TemporaryUploadRepository, file *ChangeRepoFile, hasOldBranch bool) (string, *lfs.Pointer, error) {
+type writeRepoObjectRet struct {
+	ObjectHash string
+	LfsContent io.ReadCloser // if not nil, then the caller should store its content in LfsPointer, then close it
+	LfsPointer lfs.Pointer
+}
+
+// writeRepoObjectForCreateOrUpdate hashes the git object for create or update operations
+func writeRepoObjectForCreateOrUpdate(ctx context.Context, t *TemporaryUploadRepository, file *ChangeRepoFile) (ret *writeRepoObjectRet, err error) {
+	ret = &writeRepoObjectRet{}
 	treeObjectContentReader := file.ContentReader
-	var lfsPointer *lfs.Pointer
-	if setting.LFS.StartServer && hasOldBranch {
-		// Check there is no way this can return multiple infos
-		attributesMap, err := attribute.CheckAttributes(ctx, t.gitRepo, "" /* use temp repo's working dir */, attribute.CheckAttributeOpts{
-			Attributes: []string{attribute.Filter},
-			Filenames:  []string{file.Options.treePath},
-		})
+	if setting.LFS.StartServer {
+		checkIsLfsFiles, err := checkIsLfsFileInGitAttributes(ctx, t, []string{file.Options.treePath})
 		if err != nil {
-			return "", nil, err
+			return nil, err
 		}
-
-		if attributesMap[file.Options.treePath] != nil && attributesMap[file.Options.treePath].Get(attribute.Filter).ToString().Value() == "lfs" {
-			// OK so we are supposed to LFS this data!
-			pointer, err := lfs.GeneratePointer(treeObjectContentReader)
+		if checkIsLfsFiles[0] {
+			// OK, so we are supposed to LFS this data!
+			ret.LfsPointer, err = lfs.GeneratePointer(file.ContentReader)
 			if err != nil {
-				return "", nil, err
+				return nil, err
 			}
-			lfsPointer = &pointer
-			treeObjectContentReader = strings.NewReader(pointer.StringContent())
+			if _, err = file.ContentReader.Seek(0, io.SeekStart); err != nil {
+				return nil, err
+			}
+			ret.LfsContent = io.NopCloser(file.ContentReader)
+			treeObjectContentReader = strings.NewReader(ret.LfsPointer.StringContent())
 		}
 	}
 
-	// Add the object to the database
-	objectHash, err := t.HashObject(ctx, treeObjectContentReader)
+	ret.ObjectHash, err = t.HashObjectAndWrite(ctx, treeObjectContentReader)
 	if err != nil {
-		return "", nil, err
+		return nil, err
 	}
-
-	return objectHash, lfsPointer, nil
+	return ret, nil
 }
 
-func renameFileHash(ctx context.Context, t *TemporaryUploadRepository, oldEntry *git.TreeEntry, file *ChangeRepoFile) (string, *lfs.Pointer, error) {
-	if setting.LFS.StartServer {
-		attributesMap, err := attribute.CheckAttributes(ctx, t.gitRepo, "" /* use temp repo's working dir */, attribute.CheckAttributeOpts{
-			Attributes: []string{attribute.Filter},
-			Filenames:  []string{file.Options.treePath, file.Options.fromTreePath},
-		})
-		if err != nil {
-			return "", nil, err
-		}
+// writeRepoObjectForRename the same as writeRepoObjectForCreateOrUpdate buf for "rename"
+func writeRepoObjectForRename(ctx context.Context, t *TemporaryUploadRepository, file *ChangeRepoFile) (ret *writeRepoObjectRet, err error) {
+	lastCommitID, err := t.GetLastCommit(ctx)
+	if err != nil {
+		return nil, err
+	}
+	commit, err := t.GetCommit(lastCommitID)
+	if err != nil {
+		return nil, err
+	}
+	oldEntry, err := commit.GetTreeEntryByPath(file.Options.fromTreePath)
+	if err != nil {
+		return nil, err
+	}
 
-		oldIsLfs := attributesMap[file.Options.fromTreePath] != nil && attributesMap[file.Options.fromTreePath].Get(attribute.Filter).ToString().Value() == "lfs"
-		newIsLfs := attributesMap[file.Options.treePath] != nil && attributesMap[file.Options.treePath].Get(attribute.Filter).ToString().Value() == "lfs"
+	ret = &writeRepoObjectRet{ObjectHash: oldEntry.ID.String()}
+	if !setting.LFS.StartServer {
+		return ret, nil
+	}
 
-		// If the old and new paths are both in lfs or both not in lfs, the object hash of the old file can be used directly
-		// as the object doesn't change
-		if oldIsLfs == newIsLfs {
-			return oldEntry.ID.String(), nil, nil
-		}
+	checkIsLfsFiles, err := checkIsLfsFileInGitAttributes(ctx, t, []string{file.Options.fromTreePath, file.Options.treePath})
+	if err != nil {
+		return nil, err
+	}
+	oldIsLfs, newIsLfs := checkIsLfsFiles[0], checkIsLfsFiles[1]
 
-		oldEntryReader, err := oldEntry.Blob().DataAsync()
+	// If the old and new paths are both in lfs or both not in lfs, the object hash of the old file can be used directly
+	// as the object doesn't change
+	if oldIsLfs == newIsLfs {
+		return ret, nil
+	}
+
+	oldEntryBlobPointerBy := func(f func(r io.Reader) (lfs.Pointer, error)) (lfsPointer lfs.Pointer, err error) {
+		r, err := oldEntry.Blob().DataAsync()
 		if err != nil {
-			return "", nil, err
+			return lfsPointer, err
 		}
-		defer oldEntryReader.Close()
+		defer r.Close()
+		return f(r)
+	}
 
-		var treeObjectContentReader io.Reader
-		var lfsPointer *lfs.Pointer
-		// If the old path is in lfs but the new isn't, read the content from lfs and add it as normal git object
-		// If the new path is in lfs but the old isn't, read the content from the git object and generate a lfs
-		// pointer of it
-		if oldIsLfs {
-			pointer, err := lfs.ReadPointer(oldEntryReader)
-			if err != nil {
-				return "", nil, err
-			}
-			treeObjectContentReader, err = lfs.ReadMetaObject(pointer)
-			if err != nil {
-				return "", nil, err
-			}
-			defer treeObjectContentReader.(io.ReadCloser).Close()
-		} else {
-			pointer, err := lfs.GeneratePointer(oldEntryReader)
-			if err != nil {
-				return "", nil, err
-			}
-			treeObjectContentReader = strings.NewReader(pointer.StringContent())
-			lfsPointer = &pointer
+	var treeObjectContentReader io.ReadCloser
+	if oldIsLfs {
+		// If the old is in lfs but the new isn't, read the content from lfs and add it as a normal git object
+		pointer, err := oldEntryBlobPointerBy(lfs.ReadPointer)
+		if err != nil {
+			return nil, err
 		}
-
-		// Add the object to the database
-		objectID, err := t.HashObject(ctx, treeObjectContentReader)
+		treeObjectContentReader, err = lfs.ReadMetaObject(pointer)
 		if err != nil {
-			return "", nil, err
+			return nil, err
+		}
+		defer treeObjectContentReader.Close()
+	} else {
+		// If the new is in lfs but the old isn't, read the content from the git object and generate a lfs pointer of it
+		ret.LfsPointer, err = oldEntryBlobPointerBy(lfs.GeneratePointer)
+		if err != nil {
+			return nil, err
+		}
+		ret.LfsContent, err = oldEntry.Blob().DataAsync()
+		if err != nil {
+			return nil, err
 		}
-		return objectID, lfsPointer, nil
+		treeObjectContentReader = io.NopCloser(strings.NewReader(ret.LfsPointer.StringContent()))
 	}
-
-	return oldEntry.ID.String(), nil, nil
+	ret.ObjectHash, err = t.HashObjectAndWrite(ctx, treeObjectContentReader)
+	if err != nil {
+		return nil, err
+	}
+	return ret, nil
 }
 
 // VerifyBranchProtection verify the branch protection for modifying the given treePath on the given branch
diff --git a/services/repository/files/upload.go b/services/repository/files/upload.go
index 68a071cd28..b004e3cc4c 100644
--- a/services/repository/files/upload.go
+++ b/services/repository/files/upload.go
@@ -201,10 +201,10 @@ func copyUploadedLFSFileIntoRepository(ctx context.Context, info *uploadInfo, at
 
 		info.lfsMetaObject = &git_model.LFSMetaObject{Pointer: pointer, RepositoryID: t.repo.ID}
 
-		if objectHash, err = t.HashObject(ctx, strings.NewReader(pointer.StringContent())); err != nil {
+		if objectHash, err = t.HashObjectAndWrite(ctx, strings.NewReader(pointer.StringContent())); err != nil {
 			return err
 		}
-	} else if objectHash, err = t.HashObject(ctx, file); err != nil {
+	} else if objectHash, err = t.HashObjectAndWrite(ctx, file); err != nil {
 		return err
 	}
 
diff --git a/tests/integration/repofiles_change_test.go b/tests/integration/repofiles_change_test.go
index 4678e52a9c..461175e1cc 100644
--- a/tests/integration/repofiles_change_test.go
+++ b/tests/integration/repofiles_change_test.go
@@ -17,6 +17,7 @@ import (
 	"code.gitea.io/gitea/modules/gitrepo"
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
+	"code.gitea.io/gitea/modules/util"
 	"code.gitea.io/gitea/services/contexttest"
 	files_service "code.gitea.io/gitea/services/repository/files"
 
@@ -63,42 +64,32 @@ func getUpdateRepoFilesRenameOptions(repo *repo_model.Repository) *files_service
 		Files: []*files_service.ChangeRepoFile{
 			// move normally
 			{
-				Operation:     "rename",
-				FromTreePath:  "README.md",
-				TreePath:      "README.txt",
-				SHA:           "",
-				ContentReader: nil,
+				Operation:    "rename",
+				FromTreePath: "README.md",
+				TreePath:     "README.txt",
 			},
 			// move from in lfs
 			{
-				Operation:     "rename",
-				FromTreePath:  "crypt.bin",
-				TreePath:      "crypt1.bin",
-				SHA:           "",
-				ContentReader: nil,
+				Operation:    "rename",
+				FromTreePath: "crypt.bin",
+				TreePath:     "crypt1.bin",
 			},
 			// move from lfs to normal
 			{
-				Operation:     "rename",
-				FromTreePath:  "jpeg.jpg",
-				TreePath:      "jpeg.jpeg",
-				SHA:           "",
-				ContentReader: nil,
+				Operation:    "rename",
+				FromTreePath: "jpeg.jpg",
+				TreePath:     "jpeg.jpeg",
 			},
 			// move from normal to lfs
 			{
-				Operation:     "rename",
-				FromTreePath:  "CONTRIBUTING.md",
-				TreePath:      "CONTRIBUTING.md.bin",
-				SHA:           "",
-				ContentReader: nil,
+				Operation:    "rename",
+				FromTreePath: "CONTRIBUTING.md",
+				TreePath:     "CONTRIBUTING.md.bin",
 			},
 		},
 		OldBranch: repo.DefaultBranch,
 		NewBranch: repo.DefaultBranch,
 		Message:   "Rename files",
-		Author:    nil,
-		Committer: nil,
 	}
 }
 
@@ -292,58 +283,57 @@ func getExpectedFileResponseForRepoFilesUpdate(commitID, filename, lastCommitSHA
 	}
 }
 
-func getExpectedFileResponseForRepoFilesUpdateRename(commitID, lastCommitSHA string, lastCommitterWhen, lastAuthorWhen time.Time) *api.FilesResponse {
-	details := []map[string]any{
+func getExpectedFileResponseForRepoFilesUpdateRename(commitID, lastCommitSHA string) *api.FilesResponse {
+	details := []struct {
+		filename, sha, content string
+		size                   int64
+	}{
 		{
-			"filename": "README.txt",
-			"sha":      "8276d2a29779af982c0afa976bdb793b52d442a8",
-			"size":     22,
-			"content":  "IyBBbiBMRlMtZW5hYmxlZCByZXBvCg==",
+			filename: "README.txt",
+			sha:      "8276d2a29779af982c0afa976bdb793b52d442a8",
+			size:     22,
+			content:  "IyBBbiBMRlMtZW5hYmxlZCByZXBvCg==",
 		},
 		{
-			"filename": "crypt1.bin",
-			"sha":      "d4a41a0d4db4949e129bd22f871171ea988103ef",
-			"size":     129,
-			"content":  "dmVyc2lvbiBodHRwczovL2dpdC1sZnMuZ2l0aHViLmNvbS9zcGVjL3YxCm9pZCBzaGEyNTY6MmVjY2RiNDM4MjVkMmE0OWQ5OWQ1NDJkYWEyMDA3NWNmZjFkOTdkOWQyMzQ5YTg5NzdlZmU5YzAzNjYxNzM3YwpzaXplIDIwNDgK",
+			filename: "crypt1.bin",
+			sha:      "d4a41a0d4db4949e129bd22f871171ea988103ef",
+			size:     129,
+			content:  "dmVyc2lvbiBodHRwczovL2dpdC1sZnMuZ2l0aHViLmNvbS9zcGVjL3YxCm9pZCBzaGEyNTY6MmVjY2RiNDM4MjVkMmE0OWQ5OWQ1NDJkYWEyMDA3NWNmZjFkOTdkOWQyMzQ5YTg5NzdlZmU5YzAzNjYxNzM3YwpzaXplIDIwNDgK",
 		},
 		{
-			"filename": "jpeg.jpeg",
-			"sha":      "71911bf48766c7181518c1070911019fbb00b1fc",
-			"size":     107,
-			"content":  "/9j/2wBDAAMCAgICAgMCAgIDAwMDBAYEBAQEBAgGBgUGCQgKCgkICQkKDA8MCgsOCwkJDRENDg8QEBEQCgwSExIQEw8QEBD/yQALCAABAAEBAREA/8wABgAQEAX/2gAIAQEAAD8A0s8g/9k=",
+			filename: "jpeg.jpeg",
+			sha:      "71911bf48766c7181518c1070911019fbb00b1fc",
+			size:     107,
+			content:  "/9j/2wBDAAMCAgICAgMCAgIDAwMDBAYEBAQEBAgGBgUGCQgKCgkICQkKDA8MCgsOCwkJDRENDg8QEBEQCgwSExIQEw8QEBD/yQALCAABAAEBAREA/8wABgAQEAX/2gAIAQEAAD8A0s8g/9k=",
 		},
 		{
-			"filename": "CONTRIBUTING.md.bin",
-			"sha":      "2b6c6c4eaefa24b22f2092c3d54b263ff26feb58",
-			"size":     127,
-			"content":  "dmVyc2lvbiBodHRwczovL2dpdC1sZnMuZ2l0aHViLmNvbS9zcGVjL3YxCm9pZCBzaGEyNTY6N2I2YjJjODhkYmE5Zjc2MGExYTU4NDY5YjY3ZmVlMmI2OThlZjdlOTM5OWM0Y2E0ZjM0YTE0Y2NiZTM5ZjYyMwpzaXplIDI3Cg==",
+			filename: "CONTRIBUTING.md.bin",
+			sha:      "2b6c6c4eaefa24b22f2092c3d54b263ff26feb58",
+			size:     127,
+			content:  "dmVyc2lvbiBodHRwczovL2dpdC1sZnMuZ2l0aHViLmNvbS9zcGVjL3YxCm9pZCBzaGEyNTY6N2I2YjJjODhkYmE5Zjc2MGExYTU4NDY5YjY3ZmVlMmI2OThlZjdlOTM5OWM0Y2E0ZjM0YTE0Y2NiZTM5ZjYyMwpzaXplIDI3Cg==",
 		},
 	}
 
 	var responses []*api.ContentsResponse
 	for _, detail := range details {
-		encoding := "base64"
-		content := detail["content"].(string)
-		selfURL := setting.AppURL + "api/v1/repos/user2/lfs/contents/" + detail["filename"].(string) + "?ref=master"
-		htmlURL := setting.AppURL + "user2/lfs/src/branch/master/" + detail["filename"].(string)
-		gitURL := setting.AppURL + "api/v1/repos/user2/lfs/git/blobs/" + detail["sha"].(string)
-		downloadURL := setting.AppURL + "user2/lfs/raw/branch/master/" + detail["filename"].(string)
-
+		selfURL := setting.AppURL + "api/v1/repos/user2/lfs/contents/" + detail.filename + "?ref=master"
+		htmlURL := setting.AppURL + "user2/lfs/src/branch/master/" + detail.filename
+		gitURL := setting.AppURL + "api/v1/repos/user2/lfs/git/blobs/" + detail.sha
+		downloadURL := setting.AppURL + "user2/lfs/raw/branch/master/" + detail.filename
+		// don't set time related fields because there might be different time in one operation
 		responses = append(responses, &api.ContentsResponse{
-			Name:              detail["filename"].(string),
-			Path:              detail["filename"].(string),
-			SHA:               detail["sha"].(string),
-			LastCommitSHA:     lastCommitSHA,
-			LastCommitterDate: lastCommitterWhen,
-			LastAuthorDate:    lastAuthorWhen,
-			Type:              "file",
-			Size:              int64(detail["size"].(int)),
-			Encoding:          &encoding,
-			Content:           &content,
-			URL:               &selfURL,
-			HTMLURL:           &htmlURL,
-			GitURL:            &gitURL,
-			DownloadURL:       &downloadURL,
+			Name:          detail.filename,
+			Path:          detail.filename,
+			SHA:           detail.sha,
+			LastCommitSHA: lastCommitSHA,
+			Type:          "file",
+			Size:          detail.size,
+			Encoding:      util.ToPointer("base64"),
+			Content:       &detail.content,
+			URL:           &selfURL,
+			HTMLURL:       &htmlURL,
+			GitURL:        &gitURL,
+			DownloadURL:   &downloadURL,
 			Links: &api.FileLinksResponse{
 				Self:    &selfURL,
 				GitURL:  &gitURL,
@@ -365,14 +355,12 @@ func getExpectedFileResponseForRepoFilesUpdateRename(commitID, lastCommitSHA str
 					Name:  "User Two",
 					Email: "user2@noreply.example.org",
 				},
-				Date: time.Now().UTC().Format(time.RFC3339),
 			},
 			Committer: &api.CommitUser{
 				Identity: api.Identity{
 					Name:  "User Two",
 					Email: "user2@noreply.example.org",
 				},
-				Date: time.Now().UTC().Format(time.RFC3339),
 			},
 			Parents: []*api.CommitMeta{
 				{
@@ -527,11 +515,10 @@ func TestChangeRepoFilesForUpdateWithFileRename(t *testing.T) {
 		defer ctx.Repo.GitRepo.Close()
 
 		repo := ctx.Repo.Repository
-		doer := ctx.Doer
 		opts := getUpdateRepoFilesRenameOptions(repo)
 
 		// test
-		filesResponse, err := files_service.ChangeRepoFiles(git.DefaultContext, repo, doer, opts)
+		filesResponse, err := files_service.ChangeRepoFiles(git.DefaultContext, repo, ctx.Doer, opts)
 
 		// asserts
 		assert.NoError(t, err)
@@ -540,8 +527,12 @@ func TestChangeRepoFilesForUpdateWithFileRename(t *testing.T) {
 
 		commit, _ := gitRepo.GetBranchCommit(repo.DefaultBranch)
 		lastCommit, _ := commit.GetCommitByPath(opts.Files[0].TreePath)
-		expectedFileResponse := getExpectedFileResponseForRepoFilesUpdateRename(commit.ID.String(), lastCommit.ID.String(), lastCommit.Committer.When, lastCommit.Author.When)
-		assert.Equal(t, expectedFileResponse, filesResponse)
+		expectedFileResponse := getExpectedFileResponseForRepoFilesUpdateRename(commit.ID.String(), lastCommit.ID.String())
+		for _, file := range filesResponse.Files {
+			file.LastCommitterDate, file.LastAuthorDate = time.Time{}, time.Time{} // there might be different time in one operation, so we ignore them
+		}
+		assert.Len(t, filesResponse.Files, 4)
+		assert.Equal(t, expectedFileResponse.Files, filesResponse.Files)
 	})
 }