From 67c88b90e27c99220de59fc668ecf360213bdd92 Mon Sep 17 00:00:00 2001 From: zhijie-yang Date: Wed, 27 Nov 2024 17:37:00 +0100 Subject: [PATCH] chore: apply suggestions from code review Co-authored-by: Rafid Bin Mostofa --- internal/deb/extract.go | 24 ++++++++++-------------- internal/deb/extract_test.go | 2 +- internal/fsutil/create.go | 1 - internal/fsutil/create_test.go | 8 ++++---- internal/manifest/report.go | 14 ++++++++------ internal/testutil/nopcloser.go | 2 +- 6 files changed, 24 insertions(+), 27 deletions(-) diff --git a/internal/deb/extract.go b/internal/deb/extract.go index de0f25e9..d11b0018 100644 --- a/internal/deb/extract.go +++ b/internal/deb/extract.go @@ -42,8 +42,7 @@ type ExtractInfo struct { type HardLinkInfo struct { // The target path of the hard link logged in the tarball header. TargetPath string - // The extract infos of the hard links to be extracted, which has - // the link target TargetPath. + // Info of the hard links to extract, which has the link target TargetPath. ExtractInfos []ExtractInfo } @@ -281,17 +280,16 @@ func extractData(pkgReader io.ReadSeeker, options *ExtractOptions) error { } err := options.Create(extractInfos, createOptions) if err != nil { - // Handle the hard link where its counterpart is not extracted - if tarHeader.Typeflag == tar.TypeLink && errors.Is(err, fsutil.ErrLinkTargetNotExist) { + if errors.Is(err, fsutil.ErrLinkTargetNotExist) && tarHeader.Typeflag == tar.TypeLink { // This means that the hard link target file has not been - // extracted. Add the entry to the pending list. + // extracted. Add this hard link entry to the pending list + // to extract later. basePath := sanitizeTarSourcePath(tarHeader.Linkname) - pendingHardLinks[basePath] = append(pendingHardLinks[basePath], - HardLinkInfo{ - TargetPath: targetPath, - ExtractInfos: extractInfos, - }) - pendingPaths[basePath] = true + info := HardLinkInfo{ + TargetPath: targetPath, + ExtractInfos: extractInfos, + } + pendingHardLinks[basePath] = append(pendingHardLinks[basePath], info) } else { return err } @@ -317,7 +315,6 @@ func extractData(pkgReader io.ReadSeeker, options *ExtractOptions) error { if err != nil { return err } - } if len(pendingPaths) > 0 { @@ -359,7 +356,7 @@ func handlePendingHardLinks(options *ExtractOptions, pendingHardLinks map[string continue } - // Since the hard link target file is not extracted in the first pass, + // Since the hard link target file was not extracted in the first pass, // we extract the first hard link as a regular file. For the rest of // the hard link entries, we link those paths to first file extracted. targetPath := hardLinks[0].TargetPath @@ -375,7 +372,6 @@ func handlePendingHardLinks(options *ExtractOptions, pendingHardLinks map[string if err != nil { return err } - delete(pendingPaths, sourcePath) delete(pendingPaths, targetPath) // Create the remaining hard links. diff --git a/internal/deb/extract_test.go b/internal/deb/extract_test.go index 5651cae1..1f0828b0 100644 --- a/internal/deb/extract_test.go +++ b/internal/deb/extract_test.go @@ -355,7 +355,7 @@ var extractTests = []extractTest{{ }, error: `cannot extract from package "test-package": path /dir/ requested twice with diverging mode: 0777 != 0000`, }, { - summary: "Hard link is filled with the target file", + summary: "Hard link is inflated with the target file", pkgdata: testutil.MustMakeDeb([]testutil.TarEntry{ testutil.Dir(0755, "./"), testutil.Reg(0644, "./file", "text for file"), diff --git a/internal/fsutil/create.go b/internal/fsutil/create.go index e4212e56..319a26ee 100644 --- a/internal/fsutil/create.go +++ b/internal/fsutil/create.go @@ -78,7 +78,6 @@ func Create(options *CreateOptions) (*Entry, error) { } mode := s.Mode() if o.OverrideMode && mode != o.Mode && o.Link == "" { - fmt.Printf("Changing mode of %s from %#o to %#o\n", o.Path, mode, o.Mode) err := os.Chmod(o.Path, o.Mode) if err != nil { return nil, err diff --git a/internal/fsutil/create_test.go b/internal/fsutil/create_test.go index d4bc0d8d..ec921070 100644 --- a/internal/fsutil/create_test.go +++ b/internal/fsutil/create_test.go @@ -104,7 +104,7 @@ var createTests = []createTest{{ }, { summary: "Create a hard link", options: fsutil.CreateOptions{ - Path: "dir/hardlink", + Path: "hardlink", Link: "file", Mode: 0644, MakeParents: true, @@ -122,7 +122,7 @@ var createTests = []createTest{{ }, { summary: "Cannot create a hard link if the link target does not exist", options: fsutil.CreateOptions{ - Path: "dir/hardlink", + Path: "hardlink", Link: "missing-file", Mode: 0644, MakeParents: true, @@ -132,7 +132,7 @@ var createTests = []createTest{{ }, error: `link target does not exist`, }, { - summary: "Re-creating a duplicated hard link keeps the original link", + summary: "Hard link is not created if it already exists", options: fsutil.CreateOptions{ Path: "hardlink", Link: "file", @@ -149,7 +149,7 @@ var createTests = []createTest{{ "/hardlink": "file 0644 3a6eb079", }, }, { - summary: "Cannot create a hard link if the link path exists and it is not a hard link to the target", + summary: "Cannot create a hard link if it exists and is not a hard link to target", options: fsutil.CreateOptions{ Path: "hardlink", Link: "file", diff --git a/internal/manifest/report.go b/internal/manifest/report.go index 7be99a32..a3eeb452 100644 --- a/internal/manifest/report.go +++ b/internal/manifest/report.go @@ -28,7 +28,9 @@ type Report struct { Root string // Entries holds all reported content, indexed by their path. Entries map[string]ReportEntry - // curHardLinkId is used to allocate unique HardLinkId for hard links. + + // curHardLinkId is used internally to allocate unique HardLinkId for hard + // links. curHardLinkId int } @@ -149,11 +151,11 @@ func (r *Report) sanitizeAbsPath(path string, isDir bool) (relPath string, err e return relPath, nil } -// entryIsHardLink determines if a link path belongs to a hard link by -// checking if the link path has a prefix of the root path, since -// hard links are created with absolute paths prefixing the report's root -// path, while symlinks are created either with relative paths or absolute -// paths that do not have the report's root path as a prefix. +// entryIsHardLink determines if a link is a hard link by checking if the link +// has a prefix of the root dir, since hard links are created with absolute +// paths prefixing the report's root dir, while symlinks are created either with +// relative paths or absolute paths that do not have the report's root dir as a +// prefix. func (r *Report) entryIsHardLink(link string) bool { return link != "" && strings.HasPrefix(link, r.Root) } diff --git a/internal/testutil/nopcloser.go b/internal/testutil/nopcloser.go index 79a9839f..e771527e 100644 --- a/internal/testutil/nopcloser.go +++ b/internal/testutil/nopcloser.go @@ -4,7 +4,7 @@ import ( "io" ) -// NopSeekCloser is an io.Reader that does nothing on Close, and +// readSeekerNopCloser is an io.Reader that does nothing on Close, and // seeks to the beginning of the stream on Seek. // It is an extension of io.NopCloser that also implements io.Seeker. type readSeekerNopCloser struct {