fix

Author: wxiaoguang

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] {

services/repository/files/update.go

@@ -372,6 +372,7 @@ func (err ErrSHAOrCommitIDNotProvided) Error() string {
 
 // handles the check for various issues for ChangeRepoFiles
 func handleCheckErrors(file *ChangeRepoFile, commit *git.Commit, opts *ChangeRepoFilesOptions) error {
+	// FIXME: should it also handle "rename" errors?
 	if file.Operation == "update" || file.Operation == "delete" {
 		fromEntry, err := commit.GetTreeEntryByPath(file.Options.fromTreePath)
 		if err != nil {
@@ -407,6 +408,8 @@ func handleCheckErrors(file *ChangeRepoFile, commit *git.Commit, opts *ChangeRep
 		}
 		file.Options.executable = fromEntry.IsExecutable()
 	}
+
+	// FIXME: should it also handle "rename" errors?
 	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
@@ -481,181 +484,180 @@ 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 oldEntry *git.TreeEntry
 	switch file.Operation {
 	case "create", "update":
-		objectHash, lfsPointer, err = createOrUpdateFileHash(ctx, t, file, hasOldBranch)
+		objectHash, lfsPointer, err = hashRepoObjectForCreateOrUpdate(ctx, t, file, hasOldBranch)
 	case "rename":
-		objectHash, lfsPointer, err = renameFileHash(ctx, t, oldEntry, file)
+		objectHash, lfsPointer, oldEntry, err = hashRepoObjectForRename(ctx, t, file)
+	default:
+		return util.NewInvalidArgumentErrorf("unknown 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
-		}
+	if err = t.AddObjectToIndex(ctx, util.Iif(file.Options.executable, "100755", "100644"), 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 {
+	if lfsPointer == nil {
+		return nil // No LFS pointer, so nothing to do
+	}
+
+	// Now we must store the content into an LFS object
+	lfsMetaObject, err := git_model.NewLFSMetaObject(ctx, repoID, *lfsPointer)
+	if err != nil {
+		return err
+	}
+	if exist, err := contentStore.Exists(lfsMetaObject.Pointer); err != nil {
+		return err
+	} else if exist {
+		return nil
+	}
+
+	var lfsContentReader io.ReadCloser
+	if file.Operation == "create" || file.Operation == "update" {
+		if _, err = file.ContentReader.Seek(0, io.SeekStart); err != nil {
 			return err
 		}
-		exist, err := contentStore.Exists(lfsMetaObject.Pointer)
-		if err != nil {
+		lfsContentReader = io.NopCloser(file.ContentReader)
+	} else if file.Operation == "rename" {
+		if lfsContentReader, err = oldEntry.Blob().DataAsync(); 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()
-			}
+		defer lfsContentReader.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
-			}
+	err = contentStore.Put(lfsMetaObject.Pointer, lfsContentReader)
+	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) {
-	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},
-		})
+// hashRepoObjectForCreateOrUpdate hashes the git object for create or update operations
+// If returned lfsPointer is nil, then the content will be stored as a normal git object.
+// Otherwise (lfsPointer is not nil), the content will be stored in LFS as lfsPointer.
+func hashRepoObjectForCreateOrUpdate(ctx context.Context, t *TemporaryUploadRepository, file *ChangeRepoFile, hasOldBranch bool) (objectHash string, lfsPointer *lfs.Pointer, err error) {
+	var treeObjectContentReader io.Reader
+	if setting.LFS.StartServer && hasOldBranch /* non-empty */ {
+		checkIsLfsFiles, err := checkIsLfsFileInGitAttributes(ctx, t, []string{file.Options.treePath})
 		if err != nil {
 			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!
+			pointer, err := lfs.GeneratePointer(file.ContentReader)
 			if err != nil {
 				return "", nil, err
 			}
 			lfsPointer = &pointer
 			treeObjectContentReader = strings.NewReader(pointer.StringContent())
 		}
+	} else {
+		treeObjectContentReader = file.ContentReader
 	}
 
-	// Add the object to the database
-	objectHash, err := t.HashObject(ctx, treeObjectContentReader)
+	objectHash, err = t.HashObject(ctx, treeObjectContentReader)
 	if err != nil {
 		return "", nil, err
 	}
 
 	return objectHash, lfsPointer, 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
-		}
+// hashRepoObjectForRename the same as hashRepoObjectForCreateOrUpdate buf for "rename"
+func hashRepoObjectForRename(ctx context.Context, t *TemporaryUploadRepository, file *ChangeRepoFile) (objectHash string, lfsPointer *lfs.Pointer, oldEntry *git.TreeEntry, err error) {
+	lastCommitID, err := t.GetLastCommit(ctx)
+	if err != nil {
+		return "", nil, nil, err
+	}
+	commit, err := t.GetCommit(lastCommitID)
+	if err != nil {
+		return "", nil, nil, err
+	}
+	oldEntry, err = commit.GetTreeEntryByPath(file.Options.fromTreePath)
+	if err != nil {
+		return "", nil, 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"
+	if !setting.LFS.StartServer {
+		return oldEntry.ID.String(), nil, nil, 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.treePath, file.Options.fromTreePath})
+	if err != nil {
+		return "", nil, nil, err
+	}
+	oldIsLfs, newIsLfs := checkIsLfsFiles[0], checkIsLfsFiles[1]
+
+	// 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, nil
+	}
+
+	oldEntryReader, err := oldEntry.Blob().DataAsync()
+	if err != nil {
+		return "", nil, nil, err
+	}
+	defer oldEntryReader.Close()
 
-		oldEntryReader, err := oldEntry.Blob().DataAsync()
+	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 := lfs.ReadPointer(oldEntryReader)
 		if err != nil {
-			return "", nil, err
+			return "", nil, nil, err
 		}
-		defer oldEntryReader.Close()
-
-		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
+		treeObjectContentReader, err = lfs.ReadMetaObject(pointer)
+		if err != nil {
+			return "", nil, nil, err
 		}
-
-		// Add the object to the database
-		objectID, err := t.HashObject(ctx, treeObjectContentReader)
+		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
+		pointer, err := lfs.GeneratePointer(oldEntryReader)
 		if err != nil {
-			return "", nil, err
+			return "", nil, nil, err
 		}
-		return objectID, lfsPointer, nil
+		treeObjectContentReader = io.NopCloser(strings.NewReader(pointer.StringContent()))
+		lfsPointer = &pointer
 	}
 
-	return oldEntry.ID.String(), nil, nil
+	objectHash, err = t.HashObject(ctx, treeObjectContentReader)
+	if err != nil {
+		return "", nil, nil, err
+	}
+	return objectHash, lfsPointer, oldEntry, nil
 }
 
 // VerifyBranchProtection verify the branch protection for modifying the given treePath on the given branch