Index fixes and performance improvements (fixes #223, #224)

- Fixed creation of duplicate index entries when importing from CurseForge (fixes #224) - Automatically remove duplicates in index - Fixed `packwiz serve` with a custom `--pack-root` argument (fixes #223) - Fixed `packwiz serve` with a custom index.toml location - Cleaned up internal serving code, added comments and better errors - Refactored path handling code - Improved refreshing/exporting performance - Factored out duplicated exporting logic - Replaced GetAllMods calls with cleaner LoadAllMods calls and made the former private - Improved variable names in update command - Improved handling of aliassed files - Changed CheckUpdate to take references to metadata - Removed the ability to use an absolute path to the index file (that probably didn't work anyway) - Behaviour change: order of entries in exported files may be random
2025-10-17 00:04:31 +02:00 · 2023-05-29 23:15:55 +01:00
parent d25817273b
commit 4fb1f1b59d
14 changed files with 401 additions and 352 deletions
--- a/core/index.go
+++ b/core/index.go
@@ -1,15 +1,12 @@
 package core

 import (
-	"errors"
 	"fmt"
-	"golang.org/x/exp/slices"
 	"io"
 	"io/fs"
 	"os"
 	"path"
 	"path/filepath"
-	"sort"
 	"strings"
 	"time"

@@ -22,116 +19,59 @@ import (

 // Index is a representation of the index.toml file for referencing all the files in a pack.
 type Index struct {
-	HashFormat string      `toml:"hash-format"`
-	Files      []IndexFile `toml:"files"`
+	HashFormat string
+	Files      IndexFiles
 	indexFile  string
+	packRoot   string
 }

-// IndexFile is a file in the index
-type IndexFile struct {
-	// Files are stored in forward-slash format relative to the index file
-	File           string `toml:"file"`
-	Hash           string `toml:"hash,omitempty"`
-	HashFormat     string `toml:"hash-format,omitempty"`
-	Alias          string `toml:"alias,omitempty"`
-	MetaFile       bool   `toml:"metafile,omitempty"` // True when it is a .toml metadata file
-	Preserve       bool   `toml:"preserve,omitempty"` // Don't overwrite the file when updating
-	fileExistsTemp bool
+// indexTomlRepresentation is the TOML representation of Index (Files must be converted)
+type indexTomlRepresentation struct {
+	HashFormat string                       `toml:"hash-format"`
+	Files      indexFilesTomlRepresentation `toml:"files"`
 }

 // LoadIndex attempts to load the index file from a path
 func LoadIndex(indexFile string) (Index, error) {
-	var index Index
-	if _, err := toml.DecodeFile(indexFile, &index); err != nil {
+	// Decode as indexTomlRepresentation then convert to Index
+	var rep indexTomlRepresentation
+	if _, err := toml.DecodeFile(indexFile, &rep); err != nil {
 		return Index{}, err
 	}
-	index.indexFile = indexFile
-	if len(index.HashFormat) == 0 {
-		index.HashFormat = "sha256"
+	if len(rep.HashFormat) == 0 {
+		rep.HashFormat = "sha256"
+	}
+	index := Index{
+		HashFormat: rep.HashFormat,
+		Files:      rep.Files.toMemoryRep(),
+		indexFile:  indexFile,
+		packRoot:   filepath.Dir(indexFile),
 	}
 	return index, nil
 }

-// RemoveFile removes a file from the index.
+// RemoveFile removes a file from the index, given a file path
 func (in *Index) RemoveFile(path string) error {
-	relPath, err := filepath.Rel(filepath.Dir(in.indexFile), path)
+	relPath, err := in.RelIndexPath(path)
 	if err != nil {
 		return err
 	}
-
-	i := 0
-	for _, file := range in.Files {
-		if filepath.Clean(filepath.FromSlash(file.File)) != relPath {
-			// Keep file, as it doesn't match
-			in.Files[i] = file
-			i++
-		}
-	}
-	in.Files = in.Files[:i]
+	delete(in.Files, relPath)
 	return nil
 }

-// resortIndex sorts Files by file name
-func (in *Index) resortIndex() {
-	sort.SliceStable(in.Files, func(i, j int) bool {
-		// TODO: Compare by alias if names are equal?
-		// TODO: Remove duplicated entries? (compound key on file/alias?)
-		return in.Files[i].File < in.Files[j].File
-	})
-}
-
-func (in *Index) markFound(i int, format, hash string) {
-	// Update hash
-	in.Files[i].Hash = hash
+func (in *Index) updateFileHashGiven(path, format, hash string, markAsMetaFile bool) error {
+	// Remove format if equal to index hash format
 	if in.HashFormat == format {
-		in.Files[i].HashFormat = ""
-	} else {
-		in.Files[i].HashFormat = format
+		format = ""
 	}
-	// Mark this file as found
-	in.Files[i].fileExistsTemp = true
-}

-func (in *Index) updateFileHashGiven(path, format, hash string, mod bool) error {
 	// Find in index
-	relPath, err := filepath.Rel(filepath.Dir(in.indexFile), path)
+	relPath, err := in.RelIndexPath(path)
 	if err != nil {
 		return err
 	}
-	slashPath := filepath.ToSlash(relPath)
-
-	// Binary search for slashPath in the files list
-	i, found := slices.BinarySearchFunc(in.Files, IndexFile{File: slashPath}, func(a IndexFile, b IndexFile) int {
-		return strings.Compare(a.File, b.File)
-	})
-	if found {
-		in.markFound(i, format, hash)
-		// There may be other entries with the same file path but different alias!
-		// Search back and forth to find them:
-		j := i
-		for j > 0 && in.Files[j-1].File == slashPath {
-			j = j - 1
-			in.markFound(j, format, hash)
-		}
-		j = i
-		for j < len(in.Files)-1 && in.Files[j+1].File == slashPath {
-			j = j + 1
-			in.markFound(j, format, hash)
-		}
-	} else {
-		newFile := IndexFile{
-			File:           slashPath,
-			Hash:           hash,
-			fileExistsTemp: true,
-		}
-		// Override hash format for this file, if the whole index isn't sha256
-		if in.HashFormat != format {
-			newFile.HashFormat = format
-		}
-		newFile.MetaFile = mod
-
-		in.Files = append(in.Files, newFile)
-	}
+	in.Files.updateFileEntry(relPath, format, hash, markAsMetaFile)
 	return nil
 }

@@ -165,17 +105,27 @@ func (in *Index) updateFile(path string) error {
 		hashString = h.HashToString(h.Sum(nil))
 	}

-	mod := false
-	// If the file has an extension of pw.toml, set mod to true
+	markAsMetaFile := false
+	// If the file has an extension of pw.toml, set markAsMetaFile to true
 	if strings.HasSuffix(filepath.Base(path), MetaExtension) {
-		mod = true
+		markAsMetaFile = true
 	}

-	return in.updateFileHashGiven(path, "sha256", hashString, mod)
+	return in.updateFileHashGiven(path, "sha256", hashString, markAsMetaFile)
 }

-func (in Index) GetPackRoot() string {
-	return filepath.Dir(in.indexFile)
+// ResolveIndexPath turns a path from the index into a file path on disk
+func (in Index) ResolveIndexPath(p string) string {
+	return filepath.Join(in.packRoot, filepath.FromSlash(p))
+}
+
+// RelIndexPath turns a file path on disk into a path from the index
+func (in Index) RelIndexPath(p string) (string, error) {
+	rel, err := filepath.Rel(in.packRoot, p)
+	if err != nil {
+		return "", err
+	}
+	return filepath.ToSlash(rel), nil
 }

 var ignoreDefaults = []string{
@@ -223,19 +173,18 @@ func (in *Index) Refresh() error {
 	pathPF, _ := filepath.Abs(viper.GetString("pack-file"))
 	pathIndex, _ := filepath.Abs(in.indexFile)

-	packRoot := in.GetPackRoot()
-	pathIgnore, _ := filepath.Abs(filepath.Join(packRoot, ".packwizignore"))
+	pathIgnore, _ := filepath.Abs(filepath.Join(in.packRoot, ".packwizignore"))
 	ignore, ignoreExists := readGitignore(pathIgnore)

 	var fileList []string
-	err := filepath.WalkDir(packRoot, func(path string, info os.DirEntry, err error) error {
+	err := filepath.WalkDir(in.packRoot, func(path string, info os.DirEntry, err error) error {
 		if err != nil {
 			// TODO: Handle errors on individual files properly
 			return err
 		}

 		// Never ignore pack root itself (gitignore doesn't allow ignoring the root)
-		if path == packRoot {
+		if path == in.packRoot {
 			return nil
 		}

@@ -285,13 +234,6 @@ func (in *Index) Refresh() error {
 		),
 	)

-	// Normalise file paths: updateFile needs to compare path equality
-	for i := range in.Files {
-		in.Files[i].File = path.Clean(in.Files[i].File)
-	}
-	// Resort index (required by updateFile)
-	in.resortIndex()
-
 	for _, v := range fileList {
 		start := time.Now()

@@ -307,34 +249,23 @@ func (in *Index) Refresh() error {
 	progressContainer.Wait()

 	// Check all the files exist, remove them if they don't
-	i := 0
-	for _, file := range in.Files {
-		if file.fileExistsTemp {
-			// Keep file if it exists (already checked in updateFile)
-			in.Files[i] = file
-			i++
+	for p, file := range in.Files {
+		if !file.markedFound() {
+			delete(in.Files, p)
 		}
 	}
-	in.Files = in.Files[:i]

-	in.resortIndex()
-	return nil
-}
-
-// RefreshFile calculates the hash for a given path and updates it in the index (also sorts the index)
-func (in *Index) RefreshFile(path string) error {
-	// Resort index first (required by updateFile)
-	in.resortIndex()
-	err := in.updateFile(path)
-	if err != nil {
-		return err
-	}
-	in.resortIndex()
 	return nil
 }

 // Write saves the index file
 func (in Index) Write() error {
+	// Convert to indexTomlRepresentation
+	rep := indexTomlRepresentation{
+		HashFormat: in.HashFormat,
+		Files:      in.Files.toTomlRep(),
+	}
+
 	// TODO: calculate and provide hash while writing?
 	f, err := os.Create(in.indexFile)
 	if err != nil {
@@ -344,7 +275,7 @@ func (in Index) Write() error {
 	enc := toml.NewEncoder(f)
 	// Disable indentation
 	enc.Indent = ""
-	err = enc.Encode(in)
+	err = enc.Encode(rep)
 	if err != nil {
 		_ = f.Close()
 		return err
@@ -352,42 +283,34 @@ func (in Index) Write() error {
 	return f.Close()
 }

-// RefreshFileWithHash updates a file in the index, given a file hash and whether it is a mod or not
-func (in *Index) RefreshFileWithHash(path, format, hash string, mod bool) error {
+// RefreshFileWithHash updates a file in the index, given a file hash and whether it should be marked as metafile or not
+func (in *Index) RefreshFileWithHash(path, format, hash string, markAsMetaFile bool) error {
 	if viper.GetBool("no-internal-hashes") {
 		hash = ""
 	}
-	// Resort index first (required by updateFile)
-	in.resortIndex()
-	err := in.updateFileHashGiven(path, format, hash, mod)
-	if err != nil {
-		return err
-	}
-	in.resortIndex()
-	return nil
+	return in.updateFileHashGiven(path, format, hash, markAsMetaFile)
 }

-// FindMod finds a mod in the index and returns it's path and whether it has been found
+// FindMod finds a mod in the index and returns its path and whether it has been found
 func (in Index) FindMod(modName string) (string, bool) {
-	for _, v := range in.Files {
-		if v.MetaFile {
-			_, file := filepath.Split(v.File)
-			fileTrimmed := strings.TrimSuffix(strings.TrimSuffix(file, MetaExtension), MetaExtensionOld)
+	for p, v := range in.Files {
+		if v.IsMetaFile() {
+			_, fileName := path.Split(p)
+			fileTrimmed := strings.TrimSuffix(strings.TrimSuffix(fileName, MetaExtension), MetaExtensionOld)
 			if fileTrimmed == modName {
-				return filepath.Join(filepath.Dir(in.indexFile), filepath.FromSlash(v.File)), true
+				return in.ResolveIndexPath(p), true
 			}
 		}
 	}
 	return "", false
 }

-// GetAllMods finds paths to every metadata file (Mod) in the index
-func (in Index) GetAllMods() []string {
+// getAllMods finds paths to every metadata file (Mod) in the index
+func (in Index) getAllMods() []string {
 	var list []string
-	baseDir := filepath.Dir(in.indexFile)
-	for _, v := range in.Files {
-		if v.MetaFile {
-			list = append(list, filepath.Join(baseDir, filepath.FromSlash(v.File)))
+	for p, v := range in.Files {
+		if v.IsMetaFile() {
+			list = append(list, in.ResolveIndexPath(p))
 		}
 	}
 	return list
@@ -395,7 +318,7 @@ func (in Index) GetAllMods() []string {

 // LoadAllMods reads all metadata files into Mod structs
 func (in Index) LoadAllMods() ([]*Mod, error) {
-	modPaths := in.GetAllMods()
+	modPaths := in.getAllMods()
 	mods := make([]*Mod, len(modPaths))
 	for i, v := range modPaths {
 		modData, err := LoadMod(v)
@@ -406,40 +329,3 @@ func (in Index) LoadAllMods() ([]*Mod, error) {
 	}
 	return mods, nil
 }
-
-// GetFilePath attempts to get the path of the destination index file as it is stored on disk
-func (in Index) GetFilePath(f IndexFile) string {
-	return filepath.Join(filepath.Dir(in.indexFile), filepath.FromSlash(f.File))
-}
-
-// SaveFile attempts to read the file from disk
-func (in Index) SaveFile(f IndexFile, dest io.Writer) error {
-	hashFormat := f.HashFormat
-	if hashFormat == "" {
-		hashFormat = in.HashFormat
-	}
-	src, err := os.Open(in.GetFilePath(f))
-	defer func(src *os.File) {
-		_ = src.Close()
-	}(src)
-	if err != nil {
-		return err
-	}
-	h, err := GetHashImpl(hashFormat)
-	if err != nil {
-		return err
-	}
-
-	w := io.MultiWriter(h, dest)
-	_, err = io.Copy(w, src)
-	if err != nil {
-		return err
-	}
-
-	calculatedHash := h.HashToString(h.Sum(nil))
-	if !strings.EqualFold(calculatedHash, f.Hash) && !viper.GetBool("no-internal-hashes") {
-		return errors.New("hash of saved file is invalid")
-	}
-
-	return nil
-}
--- a/core/indexfiles.go
+++ b/core/indexfiles.go
@@ -0,0 +1,191 @@
+package core
+
+import (
+	"golang.org/x/exp/slices"
+	"path"
+)
+
+// IndexFiles are stored as a map of path -> (indexFile or alias -> indexFile)
+// The latter is used for multiple copies with the same path but different alias
+type IndexFiles map[string]IndexPathHolder
+
+type IndexPathHolder interface {
+	updateHash(hash string, format string)
+	markFound()
+	markMetaFile()
+	markedFound() bool
+	IsMetaFile() bool
+}
+
+// indexFile is a file in the index
+type indexFile struct {
+	// Files are stored in forward-slash format relative to the index file
+	File       string `toml:"file"`
+	Hash       string `toml:"hash,omitempty"`
+	HashFormat string `toml:"hash-format,omitempty"`
+	Alias      string `toml:"alias,omitempty"`
+	MetaFile   bool   `toml:"metafile,omitempty"` // True when it is a .toml metadata file
+	Preserve   bool   `toml:"preserve,omitempty"` // Don't overwrite the file when updating
+	fileFound  bool
+}
+
+func (i *indexFile) updateHash(hash string, format string) {
+	i.Hash = hash
+	i.HashFormat = format
+}
+
+func (i *indexFile) markFound() {
+	i.fileFound = true
+}
+
+func (i *indexFile) markMetaFile() {
+	i.MetaFile = true
+}
+
+func (i *indexFile) markedFound() bool {
+	return i.fileFound
+}
+
+func (i *indexFile) IsMetaFile() bool {
+	return i.MetaFile
+}
+
+type indexFileMultipleAlias map[string]indexFile
+
+func (i *indexFileMultipleAlias) updateHash(hash string, format string) {
+	for k, v := range *i {
+		v.updateHash(hash, format)
+		(*i)[k] = v // Can't mutate map value in place
+	}
+}
+
+// (indexFileMultipleAlias == map[string]indexFile)
+func (i *indexFileMultipleAlias) markFound() {
+	for k, v := range *i {
+		v.markFound()
+		(*i)[k] = v // Can't mutate map value in place
+	}
+}
+
+func (i *indexFileMultipleAlias) markMetaFile() {
+	for k, v := range *i {
+		v.markMetaFile()
+		(*i)[k] = v // Can't mutate map value in place
+	}
+}
+
+func (i *indexFileMultipleAlias) markedFound() bool {
+	for _, v := range *i {
+		return v.markedFound()
+	}
+	panic("No entries in indexFileMultipleAlias")
+}
+
+func (i *indexFileMultipleAlias) IsMetaFile() bool {
+	for _, v := range *i {
+		return v.MetaFile
+	}
+	panic("No entries in indexFileMultipleAlias")
+}
+
+// updateFileEntry updates the hash of a file and marks as found; adding it if it doesn't exist
+// This also sets metafile if markAsMetaFile is set
+// This updates all existing aliassed variants of a file, but doesn't create new ones
+func (f *IndexFiles) updateFileEntry(path string, format string, hash string, markAsMetaFile bool) {
+	// Ensure map is non-nil
+	if *f == nil {
+		*f = make(IndexFiles)
+	}
+	// Fetch existing entry
+	file, found := (*f)[path]
+	if found {
+		// Exists: update hash/format/metafile
+		file.markFound()
+		file.updateHash(hash, format)
+		if markAsMetaFile {
+			file.markMetaFile()
+		}
+		// (don't do anything if markAsMetaFile is false - don't reset metafile status of existing metafiles)
+	} else {
+		// Doesn't exist: create new file data
+		newFile := indexFile{
+			File:       path,
+			Hash:       hash,
+			HashFormat: format,
+			MetaFile:   markAsMetaFile,
+			fileFound:  true,
+		}
+		(*f)[path] = &newFile
+	}
+}
+
+type indexFilesTomlRepresentation []indexFile
+
+// toMemoryRep converts the TOML representation of IndexFiles to that used in memory
+// These silly converter functions are necessary because the TOML libraries don't support custom non-primitive serializers
+func (rep indexFilesTomlRepresentation) toMemoryRep() IndexFiles {
+	out := make(IndexFiles)
+
+	// Add entries to map
+	for _, v := range rep {
+		v := v // Narrow scope of loop variable
+		v.File = path.Clean(v.File)
+		v.Alias = path.Clean(v.Alias)
+		// path.Clean converts "" into "." - undo this for Alias as we use omitempty
+		if v.Alias == "." {
+			v.Alias = ""
+		}
+		if existing, ok := out[v.File]; ok {
+			if existingFile, ok := existing.(*indexFile); ok {
+				// Is this the same as the existing file?
+				if v.Alias == existingFile.Alias {
+					// Yes: overwrite
+					out[v.File] = &v
+				} else {
+					// No: convert to new map
+					m := make(indexFileMultipleAlias)
+					m[existingFile.Alias] = *existingFile
+					m[v.Alias] = v
+					out[v.File] = &m
+				}
+			} else if existingMap, ok := existing.(*indexFileMultipleAlias); ok {
+				// Add to alias map
+				(*existingMap)[v.Alias] = v
+			} else {
+				panic("Unknown type in IndexFiles")
+			}
+		} else {
+			out[v.File] = &v
+		}
+	}
+
+	return out
+}
+
+// toTomlRep converts the in-memory representation of IndexFiles to that used in TOML
+// These silly converter functions are necessary because the TOML libraries don't support custom non-primitive serializers
+func (f *IndexFiles) toTomlRep() indexFilesTomlRepresentation {
+	// Turn internal representation into TOML representation
+	rep := make(indexFilesTomlRepresentation, 0, len(*f))
+	for _, v := range *f {
+		if file, ok := v.(*indexFile); ok {
+			rep = append(rep, *file)
+		} else if file, ok := v.(*indexFileMultipleAlias); ok {
+			for _, alias := range *file {
+				rep = append(rep, alias)
+			}
+		} else {
+			panic("Unknown type in IndexFiles")
+		}
+	}
+
+	slices.SortFunc(rep, func(a indexFile, b indexFile) bool {
+		if a.File == b.File {
+			return a.Alias < b.Alias
+		} else {
+			return a.File < b.File
+		}
+	})
+
+	return rep
+}
--- a/core/interfaces.go
+++ b/core/interfaces.go
@@ -12,7 +12,7 @@ type Updater interface {
 	ParseUpdate(map[string]interface{}) (interface{}, error)
 	// CheckUpdate checks whether there is an update for each of the mods in the given slice,
 	// called for all of the mods that this updater handles
-	CheckUpdate([]Mod, Pack) ([]UpdateCheck, error)
+	CheckUpdate([]*Mod, Pack) ([]UpdateCheck, error)
 	// DoUpdate carries out the update previously queried in CheckUpdate, on each Mod's metadata,
 	// given pointers to Mods and the value of CachedState for each mod
 	DoUpdate([]*Mod, []interface{}) error
--- a/core/pack.go
+++ b/core/pack.go
@@ -110,9 +110,6 @@ func (pack *Pack) UpdateIndexHash() error {

 	fileNative := filepath.FromSlash(pack.Index.File)
 	indexFile := filepath.Join(filepath.Dir(viper.GetString("pack-file")), fileNative)
-	if filepath.IsAbs(pack.Index.File) {
-		indexFile = pack.Index.File
-	}

 	f, err := os.Open(indexFile)
 	if err != nil {