refactor: filter created entries for report (canonical#124)

To make the content transparent and facilitate changing the package, this commit creates the deb used for testing programmatically instead of embedding it as a base64-encoded blob directly. Also, the entries not mentioned explicitly in the slice definition will not be added to the report (e.g. copyright, parent directories). Additionally, we have removed the Globbed option and simplified it.
letFunny · Apr 1, 2024 · 39ba448 · 39ba448
1 parent 60d8b38
commit 39ba448
Show file tree

Hide file tree

Showing 8 changed files with 274 additions and 102 deletions.
diff --git a/internal/deb/extract.go b/internal/deb/extract.go
@@ -24,8 +24,10 @@ type ExtractOptions struct {
 	Package   string
 	TargetDir string
 	Extract   map[string][]ExtractInfo
-	Globbed   map[string][]string
-	Create    func(options *fsutil.CreateOptions) error
+	// Create can optionally be set to control the creation of extracted entries.
+	// extractInfo is set to the matching entry in Extract, and is nil in cases where
+	// the created entry is implicit and unlisted (for example, parent directories).
+	Create func(extractInfo *ExtractInfo, options *fsutil.CreateOptions) error
 }
 
 type ExtractInfo struct {
@@ -46,7 +48,7 @@ func getValidOptions(options *ExtractOptions) (*ExtractOptions, error) {
 
 	if options.Create == nil {
 		validOpts := *options
-		validOpts.Create = func(o *fsutil.CreateOptions) error {
+		validOpts.Create = func(_ *ExtractInfo, o *fsutil.CreateOptions) error {
 			_, err := fsutil.Create(o)
 			return err
 		}
@@ -185,9 +187,6 @@ func extractData(dataReader io.Reader, options *ExtractOptions) error {
 		if globPath != "" {
 			extractInfos = options.Extract[globPath]
 			delete(pendingPaths, globPath)
-			if options.Globbed != nil {
-				options.Globbed[globPath] = append(options.Globbed[globPath], sourcePath)
-			}
 		} else {
 			extractInfos, ok = options.Extract[sourcePath]
 			if ok {
@@ -196,11 +195,12 @@ func extractData(dataReader io.Reader, options *ExtractOptions) error {
 				// Base directory for extracted content. Relevant mainly to preserve
 				// the metadata, since the extracted content itself will also create
 				// any missing directories unaccounted for in the options.
-				err := options.Create(&fsutil.CreateOptions{
+				createOptions := &fsutil.CreateOptions{
 					Path:        filepath.Join(options.TargetDir, sourcePath),
 					Mode:        tarHeader.FileInfo().Mode(),
 					MakeParents: true,
-				})
+				}
+				err := options.Create(nil, createOptions)
 				if err != nil {
 					return err
 				}
@@ -237,13 +237,14 @@ func extractData(dataReader io.Reader, options *ExtractOptions) error {
 			if extractInfo.Mode != 0 {
 				tarHeader.Mode = int64(extractInfo.Mode)
 			}
-			err := options.Create(&fsutil.CreateOptions{
+			createOptions := &fsutil.CreateOptions{
 				Path:        targetPath,
 				Mode:        tarHeader.FileInfo().Mode(),
 				Data:        pathReader,
 				Link:        tarHeader.Linkname,
 				MakeParents: true,
-			})
+			}
+			err := options.Create(&extractInfo, createOptions)
 			if err != nil {
 				return err
 			}

diff --git a/internal/deb/extract_test.go b/internal/deb/extract_test.go
@@ -2,7 +2,6 @@ package deb_test
 
 import (
 	"bytes"
-	"io/fs"
 	"path/filepath"
 	"sort"
 	"strings"
@@ -19,7 +18,6 @@ type extractTest struct {
 	pkgdata []byte
 	options deb.ExtractOptions
 	hackopt func(o *deb.ExtractOptions)
-	globbed map[string][]string
 	result  map[string]string
 	// paths which the extractor did not create explicitly.
 	notCreated []string
@@ -179,7 +177,7 @@ var extractTests = []extractTest{{
 	},
 	notCreated: []string{"/dir/"},
 }, {
-	summary: "Globbing with reporting of globbed paths",
+	summary: "Globbing multiple paths",
 	pkgdata: testutil.PackageData["test-package"],
 	options: deb.ExtractOptions{
 		Extract: map[string][]deb.ExtractInfo{
@@ -199,10 +197,6 @@ var extractTests = []extractTest{{
 		"/dir/several/levels/deep/":     "dir 0755",
 		"/dir/several/levels/deep/file": "file 0644 6bc26dff",
 	},
-	globbed: map[string][]string{
-		"/dir/n*/": []string{"/dir/nested/"},
-		"/dir/s**": []string{"/dir/several/", "/dir/several/levels/", "/dir/several/levels/deep/", "/dir/several/levels/deep/file"},
-	},
 	notCreated: []string{"/dir/"},
 }, {
 	summary: "Globbing must have matching source and target",
@@ -336,9 +330,9 @@ func (s *S) TestExtract(c *C) {
 		options.Package = "test-package"
 		options.TargetDir = dir
 		createdPaths := make(map[string]bool)
-		options.Create = func(o *fsutil.CreateOptions) error {
+		options.Create = func(_ *deb.ExtractInfo, o *fsutil.CreateOptions) error {
 			relPath := filepath.Clean("/" + strings.TrimPrefix(o.Path, dir))
-			if o.Mode&fs.ModeDir != 0 {
+			if o.Mode.IsDir() {
 				relPath = relPath + "/"
 			}
 			createdPaths[relPath] = true
@@ -350,10 +344,6 @@ func (s *S) TestExtract(c *C) {
 			test.hackopt(&options)
 		}
 
-		if test.globbed != nil {
-			options.Globbed = make(map[string][]string)
-		}
-
 		err := deb.Extract(bytes.NewBuffer(test.pkgdata), &options)
 		if test.error != "" {
 			c.Assert(err, ErrorMatches, test.error)
@@ -362,10 +352,6 @@ func (s *S) TestExtract(c *C) {
 			c.Assert(err, IsNil)
 		}
 
-		if test.globbed != nil {
-			c.Assert(options.Globbed, DeepEquals, test.globbed)
-		}
-
 		if test.notCreated != nil {
 			notCreated := []string{}
 			for path := range test.result {

diff --git a/internal/fsutil/create.go b/internal/fsutil/create.go
@@ -39,6 +39,7 @@ func Create(options *CreateOptions) (*Entry, error) {
 	o := &optsCopy
 
 	var err error
+	var hash string
 	if o.MakeParents {
 		if err := os.MkdirAll(filepath.Dir(o.Path), 0755); err != nil {
 			return nil, err
@@ -47,6 +48,7 @@ func Create(options *CreateOptions) (*Entry, error) {
 	switch o.Mode & fs.ModeType {
 	case 0:
 		err = createFile(o)
+		hash = hex.EncodeToString(rp.h.Sum(nil))
 	case fs.ModeDir:
 		err = createDir(o)
 	case fs.ModeSymlink:
@@ -61,7 +63,7 @@ func Create(options *CreateOptions) (*Entry, error) {
 	entry := &Entry{
 		Path: o.Path,
 		Mode: o.Mode,
-		Hash: hex.EncodeToString(rp.h.Sum(nil)),
+		Hash: hash,
 		Size: rp.size,
 		Link: o.Link,
 	}

diff --git a/internal/slicer/report.go b/internal/slicer/report.go
@@ -32,7 +32,7 @@ type Report struct {
 // provided root path.
 func NewReport(root string) *Report {
 	return &Report{
-		Root:    root,
+		Root:    filepath.Clean(root) + "/",
 		Entries: make(map[string]ReportEntry),
 	}
 }
@@ -42,6 +42,9 @@ func (r *Report) Add(slice *setup.Slice, fsEntry *fsutil.Entry) error {
 		return fmt.Errorf("cannot add path %q outside of root %q", fsEntry.Path, r.Root)
 	}
 	relPath := filepath.Clean("/" + strings.TrimPrefix(fsEntry.Path, r.Root))
+	if fsEntry.Mode.IsDir() {
+		relPath = relPath + "/"
+	}
 
 	if entry, ok := r.Entries[relPath]; ok {
 		if fsEntry.Mode != entry.Mode {

diff --git a/internal/slicer/report_test.go b/internal/slicer/report_test.go
@@ -27,7 +27,7 @@ var otherSlice = &setup.Slice{
 }
 
 var sampleDir = fsutil.Entry{
-	Path: "/base/exampleDir",
+	Path: "/base/exampleDir/",
 	Mode: fs.ModeDir | 0654,
 	Link: "",
 }
@@ -64,8 +64,8 @@ var reportTests = []struct {
 	summary: "Regular directory",
 	add:     []sliceAndEntry{{entry: sampleDir, slice: oneSlice}},
 	expected: map[string]slicer.ReportEntry{
-		"/exampleDir": {
-			Path:   "/exampleDir",
+		"/exampleDir/": {
+			Path:   "/exampleDir/",
 			Mode:   fs.ModeDir | 0654,
 			Slices: map[*setup.Slice]bool{oneSlice: true},
 			Link:   "",
@@ -77,8 +77,8 @@ var reportTests = []struct {
 		{entry: sampleDir, slice: otherSlice},
 	},
 	expected: map[string]slicer.ReportEntry{
-		"/exampleDir": {
-			Path:   "/exampleDir",
+		"/exampleDir/": {
+			Path:   "/exampleDir/",
 			Mode:   fs.ModeDir | 0654,
 			Slices: map[*setup.Slice]bool{oneSlice: true, otherSlice: true},
 			Link:   "",
@@ -114,8 +114,8 @@ var reportTests = []struct {
 		{entry: sampleFile, slice: otherSlice},
 	},
 	expected: map[string]slicer.ReportEntry{
-		"/exampleDir": {
-			Path:   "/exampleDir",
+		"/exampleDir/": {
+			Path:   "/exampleDir/",
 			Mode:   fs.ModeDir | 0654,
 			Slices: map[*setup.Slice]bool{oneSlice: true},
 			Link:   "",
@@ -195,6 +195,18 @@ var reportTests = []struct {
 		}, slice: oneSlice},
 	},
 	err: `path "/exampleFile" reported twice with diverging link: "distinct link" != ""`,
+}, {
+	summary: "Error for path outside root",
+	add: []sliceAndEntry{
+		{entry: fsutil.Entry{Path: "/file"}, slice: oneSlice},
+	},
+	err: `cannot add path "/file" outside of root "/base/"`,
+}, {
+	summary: "File name has root prefix but without the directory slash",
+	add: []sliceAndEntry{
+		{entry: fsutil.Entry{Path: "/basefile"}, slice: oneSlice},
+	},
+	err: `cannot add path "/basefile" outside of root "/base/"`,
 }}
 
 func (s *S) TestReportAdd(c *C) {

diff --git a/internal/slicer/slicer.go b/internal/slicer/slicer.go
@@ -158,13 +158,31 @@ func Run(options *RunOptions) (*Report, error) {
 			Package:   slice.Package,
 			Extract:   extract[slice.Package],
 			TargetDir: targetDir,
-			Globbed:   globbedPaths,
 			// Creates the filesystem entry and adds it to the report.
-			Create: func(o *fsutil.CreateOptions) error {
+			Create: func(extractInfo *deb.ExtractInfo, o *fsutil.CreateOptions) error {
 				entry, err := fsutil.Create(o)
 				if err != nil {
 					return err
 				}
+
+				// We only want to keep the entries that were explicitly listed
+				// in the slice definition.
+				if extractInfo == nil {
+					return nil
+				}
+				if _, ok := slice.Contents[extractInfo.Path]; !ok {
+					return nil
+				}
+
+				// Check whether the file was created because it matched a glob.
+				if strings.ContainsAny(extractInfo.Path, "*?") {
+					relPath := filepath.Clean("/" + strings.TrimLeft(o.Path, targetDir))
+					if o.Mode.IsDir() {
+						relPath = relPath + "/"
+					}
+					globbedPaths[extractInfo.Path] = append(globbedPaths[extractInfo.Path], relPath)
+					addKnownPath(relPath)
+				}
 				return report.Add(slice, entry)
 			},
 		})
@@ -175,12 +193,6 @@ func Run(options *RunOptions) (*Report, error) {
 		}
 	}
 
-	for _, expandedPaths := range globbedPaths {
-		for _, path := range expandedPaths {
-			addKnownPath(path)
-		}
-	}
-
 	// Create new content not coming from packages.
 	done := make(map[string]bool)
 	for _, slice := range options.Selection.Slices {