merge duplicate code

Author: wxiaoguang

models/git/lfs.go

@@ -112,7 +112,6 @@ type LFSMetaObject struct {
 	ID           int64 `xorm:"pk autoincr"`
 	lfs.Pointer  `xorm:"extends"`
 	RepositoryID int64              `xorm:"UNIQUE(s) INDEX NOT NULL"`
-	Existing     bool               `xorm:"-"`
 	CreatedUnix  timeutil.TimeStamp `xorm:"created"`
 	UpdatedUnix  timeutil.TimeStamp `xorm:"INDEX updated"`
 }
@@ -146,7 +145,6 @@ func NewLFSMetaObject(ctx context.Context, repoID int64, p lfs.Pointer) (*LFSMet
 	if err != nil {
 		return nil, err
 	} else if exist {
-		m.Existing = true
 		return m, committer.Commit()
 	}
 

modules/structs/repo_file.go

@@ -66,6 +66,8 @@ 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

routers/api/v1/repo/file.go

@@ -470,6 +470,9 @@ func ChangeFiles(ctx *context.APIContext) {
 			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.
+		// Need to fully fix them in API
 		changeRepoFile := &files_service.ChangeRepoFile{
 			Operation:     file.Operation,
 			TreePath:      file.Path,

routers/web/repo/editor.go

@@ -389,24 +389,6 @@ func UploadFilePost(ctx *context.Context) {
 		return
 	}
 
-	if !ctx.Repo.Repository.IsEmpty && form.TreePath != "" {
-		_, treePaths := getParentTreeFields(form.TreePath)
-		for _, parentTreePath := range treePaths {
-			entry, err := ctx.Repo.Commit.GetTreeEntryByPath(parentTreePath)
-			if git.IsErrNotExist(err) {
-				break // Means there is no item with that name, so we're good
-			} else if err != nil {
-				ctx.ServerError("GetTreeEntryByPath", err)
-				return
-			}
-			// User can only upload files to a directory, the directory name shouldn't be an existing file.
-			if !entry.IsDir() {
-				ctx.JSONError(ctx.Tr("repo.editor.directory_is_a_file", parentTreePath))
-				return
-			}
-		}
-	}
-
 	commitMessage := buildEditorCommitMessage(ctx.Locale.TrString("repo.editor.upload_files_to_dir", util.IfZero(form.TreePath, "/")), form.CommitSummary, form.CommitMessage)
 
 	gitCommitter, valid := WebGitOperationGetCommitChosenEmailIdentity(ctx, form.CommitEmail)
@@ -416,7 +398,7 @@ func UploadFilePost(ctx *context.Context) {
 	}
 
 	err := files_service.UploadRepoFiles(ctx, ctx.Repo.Repository, ctx.Doer, &files_service.UploadRepoFileOptions{
-		LastCommitID: ctx.Repo.CommitID,
+		LastCommitID: form.LastCommit,
 		OldBranch:    oldBranchName,
 		NewBranch:    branchName,
 		TreePath:     form.TreePath,

routers/web/repo/editor_error.go

@@ -38,13 +38,13 @@ func editorHandleFileOperationErrorRender(ctx *context_service.Context, message,
 }
 
 func editorHandleFileOperationError(ctx *context_service.Context, targetBranchName string, err error) {
-	if git.IsErrNotExist(err) {
-		ctx.JSONError(ctx.Tr("repo.editor.file_modifying_no_longer_exists", ctx.Repo.TreePath))
+	if errAs, ok := errorAs[git.ErrNotExist](err); ok {
+		ctx.JSONError(ctx.Tr("repo.editor.file_modifying_no_longer_exists", errAs.RelPath))
 	} else if git_model.IsErrLFSFileLocked(err) {
 		ctx.JSONError(ctx.Tr("repo.editor.upload_file_is_locked", err.(git_model.ErrLFSFileLocked).Path, err.(git_model.ErrLFSFileLocked).UserName))
 	} else if errAs, ok := errorAs[files_service.ErrFilenameInvalid](err); ok {
 		ctx.JSONError(ctx.Tr("repo.editor.filename_is_invalid", errAs.Path))
-	} else if errAs, ok := errorAs[files_service.ErrFilePathInvalid](errAs); ok {
+	} else if errAs, ok := errorAs[files_service.ErrFilePathInvalid](err); ok {
 		switch errAs.Type {
 		case git.EntryModeSymlink:
 			ctx.JSONError(ctx.Tr("repo.editor.file_is_a_symlink", errAs.Path))
@@ -55,13 +55,13 @@ func editorHandleFileOperationError(ctx *context_service.Context, targetBranchNa
 		default:
 			ctx.JSONError(ctx.Tr("repo.editor.filename_is_invalid", errAs.Path))
 		}
-	} else if errAs, ok := errorAs[files_service.ErrRepoFileAlreadyExists](errAs); ok {
+	} else if errAs, ok := errorAs[files_service.ErrRepoFileAlreadyExists](err); ok {
 		ctx.JSONError(ctx.Tr("repo.editor.file_already_exists", errAs.Path))
-	} else if errAs, ok := errorAs[git.ErrBranchNotExist](errAs); ok {
+	} else if errAs, ok := errorAs[git.ErrBranchNotExist](err); ok {
 		ctx.JSONError(ctx.Tr("repo.editor.branch_does_not_exist", errAs.Name))
-	} else if errAs, ok := errorAs[git_model.ErrBranchAlreadyExists](errAs); ok {
+	} else if errAs, ok := errorAs[git_model.ErrBranchAlreadyExists](err); ok {
 		ctx.JSONError(ctx.Tr("repo.editor.branch_already_exists", errAs.BranchName))
-	} else if files_service.IsErrCommitIDDoesNotMatch(errAs) {
+	} else if files_service.IsErrCommitIDDoesNotMatch(err) {
 		ctx.JSONError(ctx.Tr("repo.editor.commit_id_not_matching"))
 	} else if files_service.IsErrCommitIDDoesNotMatch(err) || git.IsErrPushOutOfDate(err) {
 		ctx.JSONError(ctx.Tr("repo.editor.file_changed_while_editing", ctx.Repo.RepoLink+"/compare/"+util.PathEscapeSegments(ctx.Repo.CommitID)+"..."+util.PathEscapeSegments(targetBranchName)))
@@ -71,8 +71,10 @@ func editorHandleFileOperationError(ctx *context_service.Context, targetBranchNa
 		} else {
 			editorHandleFileOperationErrorRender(ctx, ctx.Locale.TrString("repo.editor.push_rejected"), ctx.Locale.TrString("repo.editor.push_rejected_summary"), errAs.Message)
 		}
+	} else if errors.Is(err, util.ErrNotExist) {
+		ctx.JSONError(ctx.Tr("error.not_found"))
 	} else {
-		setting.PanicInDevOrTesting("unclear err: %v", err)
+		setting.PanicInDevOrTesting("unclear err %T: %v", err, err)
 		editorHandleFileOperationErrorRender(ctx, ctx.Locale.TrString("repo.editor.failed_to_commit"), ctx.Locale.TrString("repo.editor.failed_to_commit_summary"), err.Error())
 	}
 }

services/forms/repo_form.go

@@ -757,6 +757,7 @@ type UploadRepoFileForm struct {
 	CommitMessage string
 	CommitChoice  string `binding:"Required;MaxSize(50)"`
 	NewBranchName string `binding:"GitRefName;MaxSize(100)"`
+	LastCommit    string
 	Files         []string
 	Signoff       bool
 	CommitEmail   string

services/repository/files/update.go

@@ -88,8 +88,25 @@ func (err ErrRepoFileDoesNotExist) Unwrap() error {
 	return util.ErrNotExist
 }
 
+type LazyReader interface {
+	io.Closer
+	OpenLazyReader() error
+}
+
 // ChangeRepoFiles adds, updates or removes multiple files in the given repository
-func ChangeRepoFiles(ctx context.Context, repo *repo_model.Repository, doer *user_model.User, opts *ChangeRepoFilesOptions) (*structs.FilesResponse, error) {
+func ChangeRepoFiles(ctx context.Context, repo *repo_model.Repository, doer *user_model.User, opts *ChangeRepoFilesOptions) (_ *structs.FilesResponse, errRet error) {
+	var addedLfsPointers []lfs.Pointer
+	defer func() {
+		if errRet != nil {
+			for _, lfsPointer := range addedLfsPointers {
+				_, err := git_model.RemoveLFSMetaObjectByOid(ctx, repo.ID, lfsPointer.Oid)
+				if err != nil {
+					log.Error("ChangeRepoFiles: RemoveLFSMetaObjectByOid failed: %v", err)
+				}
+			}
+		}
+	}()
+
 	err := repo.MustNotBeArchived()
 	if err != nil {
 		return nil, err
@@ -241,10 +258,14 @@ func ChangeRepoFiles(ctx context.Context, repo *repo_model.Repository, doer *use
 	lfsContentStore := lfs.NewContentStore()
 	for _, file := range opts.Files {
 		switch file.Operation {
-		case "create", "update", "rename":
-			if err = CreateUpdateRenameFile(ctx, t, file, lfsContentStore, repo.ID, hasOldBranch); err != nil {
+		case "create", "update", "rename", "upload":
+			addedLfsPointer, err := modifyFile(ctx, t, file, lfsContentStore, repo.ID)
+			if err != nil {
 				return nil, err
 			}
+			if addedLfsPointer != nil {
+				addedLfsPointers = append(addedLfsPointers, *addedLfsPointer)
+			}
 		case "delete":
 			if err = t.RemoveFilesFromIndex(ctx, file.TreePath); err != nil {
 				return nil, err
@@ -366,6 +387,7 @@ func (err ErrSHAOrCommitIDNotProvided) Error() string {
 
 // handles the check for various issues for ChangeRepoFiles
 func handleCheckErrors(file *ChangeRepoFile, commit *git.Commit, opts *ChangeRepoFilesOptions) error {
+	// create: old must not exist; update: old must exist; upload: old existence doesn't matter
 	if file.Operation == "update" || file.Operation == "delete" || file.Operation == "rename" {
 		fromEntry, err := commit.GetTreeEntryByPath(file.Options.fromTreePath)
 		if err != nil {
@@ -403,7 +425,7 @@ func handleCheckErrors(file *ChangeRepoFile, commit *git.Commit, opts *ChangeRep
 		file.Options.executable = fromEntry.IsExecutable()
 	}
 
-	if file.Operation == "create" || file.Operation == "update" || file.Operation == "rename" {
+	if file.Operation == "create" || file.Operation == "update" || file.Operation == "upload" || 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.
@@ -454,18 +476,23 @@ func handleCheckErrors(file *ChangeRepoFile, commit *git.Commit, opts *ChangeRep
 	return nil
 }
 
-func CreateUpdateRenameFile(ctx context.Context, t *TemporaryUploadRepository, file *ChangeRepoFile, contentStore *lfs.ContentStore, repoID int64, hasOldBranch bool) error {
+func modifyFile(ctx context.Context, t *TemporaryUploadRepository, file *ChangeRepoFile, contentStore *lfs.ContentStore, repoID int64) (addedLfsPointer *lfs.Pointer, _ error) {
+	if rd, ok := file.ContentReader.(LazyReader); ok {
+		if err := rd.OpenLazyReader(); err != nil {
+			return nil, fmt.Errorf("OpenLazyReader: %w", err)
+		}
+		defer rd.Close()
+	}
+
 	// 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 {
-		return fmt.Errorf("UpdateRepoFile: %w", err)
+		return nil, fmt.Errorf("LsFiles: %w", err)
 	}
 	// If is a new file (not updating) then the given path shouldn't exist
 	if file.Operation == "create" {
 		if slices.Contains(filesInIndex, file.TreePath) {
-			return ErrRepoFileAlreadyExists{
-				Path: file.TreePath,
-			}
+			return nil, ErrRepoFileAlreadyExists{Path: file.TreePath}
 		}
 	}
 
@@ -474,53 +501,54 @@ func CreateUpdateRenameFile(ctx context.Context, t *TemporaryUploadRepository, f
 		for _, indexFile := range filesInIndex {
 			if indexFile == file.Options.fromTreePath {
 				if err = t.RemoveFilesFromIndex(ctx, file.FromTreePath); err != nil {
-					return err
+					return nil, err
 				}
 			}
 		}
 	}
 
 	var writeObjectRet *writeRepoObjectRet
 	switch file.Operation {
-	case "create", "update":
-		writeObjectRet, err = writeRepoObjectForCreateOrUpdate(ctx, t, file)
+	case "create", "update", "upload":
+		writeObjectRet, err = writeRepoObjectForModify(ctx, t, file)
 	case "rename":
 		writeObjectRet, err = writeRepoObjectForRename(ctx, t, file)
 	default:
-		return util.NewInvalidArgumentErrorf("unknown file modification operation: '%s'", file.Operation)
+		return nil, util.NewInvalidArgumentErrorf("unknown file modification operation: '%s'", file.Operation)
 	}
 	if err != nil {
-		return err
+		return nil, 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
+		return nil, err
 	}
 
 	if writeObjectRet.LfsContent == nil {
-		return nil // No LFS pointer, so nothing to do
+		return nil, nil // No LFS pointer, so nothing to do
 	}
 	defer writeObjectRet.LfsContent.Close()
 
 	// 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
+		return nil, err
 	}
-
-	err = contentStore.Put(lfsMetaObject.Pointer, writeObjectRet.LfsContent)
+	exist, err := contentStore.Exists(lfsMetaObject.Pointer)
 	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 nil, err
+	}
+	if !exist {
+		err = contentStore.Put(lfsMetaObject.Pointer, writeObjectRet.LfsContent)
+		if err != nil {
+			if _, errRemove := git_model.RemoveLFSMetaObjectByOid(ctx, repoID, lfsMetaObject.Oid); errRemove != nil {
+				return nil, fmt.Errorf("unable to remove failed inserted LFS object %s: %v (Prev Error: %w)", lfsMetaObject.Oid, errRemove, err)
+			}
+			return nil, err
 		}
 	}
-	return err
+	return &lfsMetaObject.Pointer, nil
 }
 
 func checkIsLfsFileInGitAttributes(ctx context.Context, t *TemporaryUploadRepository, paths []string) (ret []bool, err error) {
@@ -544,8 +572,8 @@ type writeRepoObjectRet struct {
 	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) {
+// writeRepoObjectForModify hashes the git object for create or update operations
+func writeRepoObjectForModify(ctx context.Context, t *TemporaryUploadRepository, file *ChangeRepoFile) (ret *writeRepoObjectRet, err error) {
 	ret = &writeRepoObjectRet{}
 	treeObjectContentReader := file.ContentReader
 	if setting.LFS.StartServer {
@@ -574,7 +602,7 @@ func writeRepoObjectForCreateOrUpdate(ctx context.Context, t *TemporaryUploadRep
 	return ret, nil
 }
 
-// writeRepoObjectForRename the same as writeRepoObjectForCreateOrUpdate buf for "rename"
+// writeRepoObjectForRename the same as writeRepoObjectForModify buf for "rename"
 func writeRepoObjectForRename(ctx context.Context, t *TemporaryUploadRepository, file *ChangeRepoFile) (ret *writeRepoObjectRet, err error) {
 	lastCommitID, err := t.GetLastCommit(ctx)
 	if err != nil {