Conversation
|
Warning Rate limit exceeded@nao1215 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 11 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (27)
WalkthroughAdds Go 1.25 to CI and updates golangci Go version; introduces goosWindows constant and several nolint directives; replaces exec.Command with context-aware exec.CommandContext in multiple places; tightens I/O error handling and return-value ignores; refactors some command signatures; adds manPaths helper and tests; expands test isolation (XDG, t.Setenv) and unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI command handler
participant Context as context.Background()
participant OS as OS exec
rect rgb(235, 245, 255)
Note over CLI,Context: Exec now uses a context-aware command
end
CLI->>OS: exec.CommandContext(context.Background(), cmd...).Start()
alt Start succeeds
OS-->>CLI: process started
else Start fails
OS-->>CLI: start error
end
sequenceDiagram
participant manCmd as man command
participant ENV as MANPATH env
participant FS as filesystem
rect rgb(245, 250, 230)
Note over manCmd,ENV: manPaths() normalizes MANPATH and yields destinations
end
manCmd->>ENV: read MANPATH
ENV-->>manCmd: raw value (e.g., "p1:p2:.:")
manCmd->>manCmd: normalize -> [p1/man1, p2/man1] (filter dots/empty)
loop each destination
manCmd->>FS: generateManpages(dest)
FS-->>manCmd: success / error (abort on first error)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/config/config.go (1)
71-76: Preferstrings.Builderfor string concatenation in loops.While the change to
+=is functionally correct, building strings via concatenation in a loop is inefficient in Go due to repeated allocations. Consider usingstrings.Builderfor better performance.Apply this diff to use
strings.Builder:func WriteConfFile(file io.Writer, pkgs []goutil.Package) error { - text := "" + var builder strings.Builder for _, v := range pkgs { // lost version information - text += fmt.Sprintf("%s = %s\n", v.Name, v.ImportPath) + builder.WriteString(fmt.Sprintf("%s = %s\n", v.Name, v.ImportPath)) } - _, err := file.Write(([]byte)(text)) + _, err := file.Write([]byte(builder.String())) if err != nil { return fmt.Errorf("%s %s: %w", "can't update", FilePath(), err) } return nil }cmd/list_test.go (1)
122-122: Minor inconsistency in directory permissions.The directory is created with mode
0o750(more restrictive) while other test files in this PR use0755. Consider standardizing on one permission mode across all test files for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
.github/workflows/unit_test.yml(1 hunks).golangci.yml(1 hunks)cmd/bug_report.go(1 hunks)cmd/bug_report_linux.go(1 hunks)cmd/check.go(1 hunks)cmd/check_test.go(3 hunks)cmd/export_test.go(7 hunks)cmd/list.go(2 hunks)cmd/list_test.go(6 hunks)cmd/man.go(1 hunks)cmd/man_test.go(2 hunks)cmd/remove.go(1 hunks)cmd/remove_test.go(5 hunks)cmd/root.go(1 hunks)cmd/root_test.go(23 hunks)cmd/update_test.go(4 hunks)doc.go(0 hunks)internal/cmdinfo/cmdinfo.go(1 hunks)internal/completion/completion.go(2 hunks)internal/config/config.go(4 hunks)internal/goutil/examples_test.go(1 hunks)internal/goutil/goutil.go(9 hunks)internal/goutil/goutil_test.go(3 hunks)internal/print/print.go(2 hunks)internal/print/print_test.go(6 hunks)
💤 Files with no reviewable changes (1)
- doc.go
🧰 Additional context used
🧬 Code graph analysis (10)
internal/print/print.go (2)
internal/cmdinfo/cmdinfo.go (1)
Name(12-12)cmd/root.go (1)
OsExit(15-15)
cmd/update_test.go (1)
cmd/remove.go (1)
GOOS(59-59)
cmd/list_test.go (1)
internal/print/print.go (1)
Fatal(49-53)
cmd/check.go (1)
internal/print/print.go (1)
Info(26-28)
cmd/check_test.go (1)
cmd/remove.go (1)
GOOS(59-59)
cmd/list.go (1)
internal/print/print.go (1)
Stdout(16-16)
internal/print/print_test.go (2)
internal/print/print.go (1)
Fatal(49-53)internal/cmdinfo/cmdinfo.go (1)
Name(12-12)
cmd/remove_test.go (1)
cmd/remove.go (1)
GOOS(59-59)
cmd/root_test.go (4)
cmd/remove.go (1)
GOOS(59-59)internal/print/print.go (2)
Fatal(49-53)OsExit(45-45)cmd/root.go (1)
OsExit(15-15)internal/cmdinfo/cmdinfo.go (1)
Name(12-12)
cmd/man.go (1)
internal/print/print.go (1)
Err(39-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Unit test (linux) (windows-latest, 1.24)
- GitHub Check: Unit test (linux) (windows-latest, 1.25)
- GitHub Check: Unit test (linux) (ubuntu-latest, 1.25)
🔇 Additional comments (42)
internal/completion/completion.go (2)
194-194: Good refactor to usebytes.Equal.Using
bytes.Equalis more idiomatic and clearer than comparingbytes.Compareto zero for equality checks. This improves readability and is the recommended approach in Go.
216-216: Good refactor to usebytes.Equal.Consistent with the change on line 194, this improves code clarity and follows Go best practices for byte slice equality checks.
internal/config/config.go (2)
21-21: LGTM! Linter directive appropriately applied.The
nolint:gochecknoglobalsdirective is appropriate for this exported configuration constant, aligning with the PR's linter-fix objective.
41-42: Good practice: magic number replaced with named constant.Replacing the magic number
2withexpectedPairimproves code readability and maintainability.Also applies to: 55-55
cmd/check.go (1)
150-154: LGTM! Good use of a named constant.Replacing the magic number 11 with the named constant
indentSpacesimproves code readability and maintainability..github/workflows/unit_test.yml (1)
21-23: LGTM! Testing against Go 1.25 is good.Adding Go 1.25 to the test matrix ensures compatibility with the latest Go version.
cmd/bug_report_linux.go (1)
6-11: LGTM! Good adoption of context-aware command execution.Switching to
exec.CommandContextwithcontext.Background()is a good practice that adds cancellation capability while maintaining the same behavior.internal/cmdinfo/cmdinfo.go (1)
9-9: LGTM! Appropriate use of nolint directive.The
nolint:gochecknoglobalsdirective is justified here sinceVersionmust be a package-level variable to be set via ldflags at build time.cmd/root.go (1)
15-15: LGTM! Appropriate use of nolint directive.The
nolint:gochecknoglobalsdirective is justified here sinceOsExitneeds to be a package-level variable for test mocking purposes, as indicated by the comment.cmd/remove.go (1)
56-69: LGTM! Good refactoring with named constant.The introduction of the
goosWindowsconstant replaces the magic string"windows"and improves code maintainability. Thenolint:gochecknoglobalsdirective onGOOSis appropriate since it's a test wrapper variable.cmd/man_test.go (2)
7-9: LGTM! Good addition of testing dependency.The
go-cmplibrary is appropriate for deep comparison testing and provides clear diff output.
39-75: LGTM! Comprehensive test coverage for manPaths.The test suite covers key edge cases:
- Empty MANPATH returning default path
- Multiple colon-separated paths with proper "man1" suffix handling
- MANPATH containing only empty segments
Good use of
filepath.Joinfor cross-platform path handling andcmp.Difffor clear failure messages. Verification confirms the implementation incmd/man.gocorrectly handles all tested scenarios..golangci.yml (1)
3-3: Verify Go version alignment in CI configuration.The
go: "1.24"setting in golangci-lint only supports Go versions ≤ 1.24. Since the test matrix includes Go 1.25, linting will fail on 1.25 tests unless golangci-lint's binary was compiled with Go 1.25 (v2.4.0+). Confirm your CI uses a golangci-lint binary built with Go 1.25 or later, or remove Go 1.25 from the test matrix if linting shouldn't run against it.cmd/bug_report.go (1)
29-29: LGTM! Clean removal of unnecessary linter directive.The removal of the trailing
//nolintdirective is appropriate and improves code cleanliness.internal/goutil/goutil.go (4)
5-5: LGTM! Context import and linter annotations are appropriate.The
contextimport is needed for context-aware command execution, and thenolint:gochecknoglobalsannotations are appropriate for these test-mockable global variables.Also applies to: 25-31
184-204: LGTM! Switch statement is more idiomatic.The refactoring from if-else to switch improves readability and maintains the original behavior. The default case correctly handles the scenario where neither GOBIN nor GOPATH is set.
210-230: LGTM! Consistent switch pattern.The refactoring mirrors the change in
StartDryRunModeand maintains the original behavior while improving readability.
284-284: LGTM! Context-aware command execution is a best practice.The migration to
exec.CommandContextis appropriate and follows modern Go conventions. Whilecontext.Background()doesn't enforce timeouts, it establishes the pattern for future enhancement where timeouts might be needed.Also applies to: 296-296, 309-309, 403-403
internal/goutil/goutil_test.go (3)
1-1: LGTM! File-level nolint directive is appropriate.The
//nolint:paralleltest,goconstdirective appropriately suppresses linter warnings for test patterns that don't benefit from parallelization and string constant extraction.
158-159: LGTM! t.Setenv improves test hygiene.Using
t.Setenvis superior to manual environment variable management as it automatically handles cleanup and prevents test pollution.
547-551: LGTM! Consistent use of t.Setenv for environment management.The migration to
t.Setenvfor environment variable handling is consistent with modern Go testing practices and automatically handles cleanup.cmd/man.go (2)
32-40: LGTM! Multi-destination support implemented correctly.The iteration over multiple MANPATH destinations with fail-fast error handling is appropriate. Early termination on the first error ensures that permission or I/O issues are reported immediately.
42-68: LGTM! MANPATH normalization logic is well-structured.The
manPathsfunction correctly handles MANPATH parsing with appropriate defaults and path normalization. The logic to ensure paths end with/man1and fall back to the default when no valid paths remain is sound.One minor consideration: Line 53 skips "." as an invalid path. While this is reasonable for system-wide man page installation, it prevents testing with relative paths. This trade-off is acceptable for the intended use case (requiring root privileges).
internal/print/print.go (2)
16-18: LGTM! Nolint annotations for test-mockable globals are appropriate.The
//nolint:gochecknoglobalsannotations correctly suppress linter warnings for global variables that are intentionally exported for test mocking purposes.Also applies to: 45-45, 56-56
27-27: LGTM! Explicit return value handling silences linter warnings.The explicit assignment of
fmt.Fprintfreturn values to blank identifiers is the idiomatic way to silenceerrcheckwarnings for output functions where errors are typically not actionable in these logging contexts.Also applies to: 33-33, 40-40, 50-50, 62-62
internal/print/print_test.go (3)
2-3: LGTM! Nolint directive appropriate for these tests.The
//nolint:paralleltestdirective is appropriate as these tests modify shared global state (Stdout, Stderr) and cannot safely run in parallel.
47-49: LGTM! Improved error handling in tests.Explicitly checking and propagating errors from
pw.Close()improves test reliability and surfaces I/O issues that could otherwise be silently ignored.Also applies to: 96-98, 145-147, 202-204
335-335: LGTM! Appropriate error suppression in cleanup.Ignoring the
os.Removeerror in cleanup is acceptable as this is a best-effort operation and the test environment will clean up temp files regardless.cmd/update_test.go (3)
1-1: LGTM! Nolint directive appropriate for test patterns.The
//nolint:paralleltest,errcheckdirective appropriately handles test-specific linting concerns.
183-185: LGTM! Improved error handling in test.Explicitly checking and propagating errors from
pw.Close()improves test reliability.
198-198: ThegoosWindowsconstant is properly defined and accessible.The constant is defined in cmd/remove.go:56 as
const goosWindows = "windows"and is correctly used in cmd/update_test.go at lines 198 and 251. Since both files are in the same cmd package, the package-level constant is properly accessible.cmd/list.go (2)
26-26: LGTM! Idiomatic parameter naming.Using
_for unused parameters is the idiomatic Go way to indicate intentionally ignored values.
57-57: LGTM! Explicit return value handling.Explicitly discarding
fmt.Fprintfreturn values silences linter warnings appropriately for this logging context.cmd/remove_test.go (1)
127-129: Good test isolation pattern for Windows-specific behavior.The test correctly mocks Windows behavior by temporarily overriding the
GOOSvariable with proper cleanup via defer. This allows testing Windows-specific code paths on non-Windows systems.cmd/list_test.go (2)
33-35: Good improvement: proper error handling for pipe closure.Checking
pw.Close()errors ensures that any write failures are caught during the test, preventing silent failures. This is a solid improvement over the previous unchecked closure.Also applies to: 142-144
44-46: Appropriate pattern for cleanup deferred closures.Using a deferred closure that ignores the error from
pr.Close()is acceptable since this is cleanup code for the read end of the pipe. Any critical errors would have already surfaced during theio.Copyoperation.Also applies to: 153-155
cmd/export_test.go (1)
70-72: Good error handling improvements for pipe operations.The improved error handling for
pw.Close()and the deferred cleanup pattern forpr.Close()follow the same solid approach used in other updated test files, ensuring write errors are caught while allowing cleanup code to proceed.Also applies to: 81-83, 187-189, 198-200
cmd/root_test.go (4)
108-122: Excellent addition: XDG base directory setup for test isolation.The
setupXDGBasehelper function properly isolates test environments by:
- Using
t.TempDir()for automatic cleanup- Setting up all XDG directories (config, data, cache) with appropriate permissions
- Using
t.Setenvfor automatic environment restorationThis significantly improves test reliability and prevents cross-test contamination.
52-52: Good improvement: uset.Setenvfor automatic cleanup.Replacing
os.Setenvwitht.Setenvensures the environment variable is automatically restored after the test completes, eliminating the need for manual cleanup and reducing the risk of test pollution.
472-474: Good practice: opt-in guards for slow integration tests.Requiring the
GUP_RUN_UPDATE_TESTS=1environment variable prevents these slow update/import tests from running in standard CI pipelines while still allowing them to be executed when explicitly needed. This improves CI performance without sacrificing test coverage.Also applies to: 556-558, 613-615
478-482: Improved test isolation with temporary GOBIN directory.Using a temporary directory for
GOBINinstead of manipulating the real environment prevents test interference and allows parallel test execution. This is a solid improvement over the previous approach.cmd/check_test.go (1)
103-103: ThegoosWindowsconstant is properly defined atcmd/remove.go:56.Verification confirms the constant exists and is correctly used in the referenced lines. No issues found.
nao1215
force-pushed
the
nchika/fix-use-MANPAGE-env
branch
from
2988f21 to
882246b
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
internal/goutil/examples_test.go (1)
48-52: This concern was previously flagged: test coverage is still lost due to unconditional skip.The previous review suggested three alternatives to preserve coverage and documentation value (build tags, environment variable checks, or mocking). This change still unconditionally skips the example without implementing any of those alternatives.
If this skip is temporary (e.g., to pass CI while addressing linter issues in this PR), consider adding a TODO comment or tracking issue to restore the actual test implementation later.
🧹 Nitpick comments (6)
cmd/remove_test.go (1)
1-1: Consider more targeted linter suppressions.The blanket
//nolintdirective suppresses linters for the entire file. While test code often has different requirements, it's better to either fix the underlying issues or add inline suppressions for specific lines where necessary, making it clear why each suppression is needed.For example, you could add targeted suppressions like:
defer func() { _ = os.Remove(dest) // nolint:errcheck // cleanup in test }()cmd/check.go (1)
150-154: Nice refactor: magic number extracted to named constant.The introduction of
indentSpacesconstant improves code clarity and maintainability by replacing the magic number 11. This makes the intent explicit and would simplify future adjustments to the indentation.internal/goutil/goutil.go (1)
284-284: Good adoption of context-aware command execution.The migration from
exec.Commandtoexec.CommandContextfollows Go best practices and provides a foundation for cancellation support. Usingcontext.Background()is appropriate for now given the functions don't currently accept context parameters.Optional future enhancement: Consider propagating context through function parameters (e.g.,
install(ctx context.Context, importPath, version string)) to enable proper cancellation and timeout control for long-running operations.Also applies to: 296-296, 309-309, 403-403
cmd/export_test.go (1)
70-72: Inconsistent close error handling.The error handling is now asymmetric:
pw.Close()errors are checked and fatal (lines 70-72, 187-189), whilepr.Close()errors are explicitly ignored in deferred calls (lines 81-83, 198-200).While this may be intentional (write-side errors being more critical than read-side close errors in test cleanup), the inconsistency is worth noting. Consider adding a brief comment explaining why
pr.Close()errors can be safely ignored in cleanup.Also applies to: 81-83, 187-189, 198-200
cmd/man.go (1)
52-54: Optional: Simplify the empty path check.The check
p == ""is redundant becausefilepath.Cleannever returns an empty string (it returns"."for empty input). The condition could be simplified to justp == "."or evenp == "." || p == string(filepath.Separator)if you want to be more explicit.internal/config/config_test.go (1)
13-36: Consider consolidating XDG setup helpers across test files.This helper duplicates the
setupXDGBasefunction incmd/root_test.go(lines 108-122), with the key difference being thatwithTempXDGcorrectly returns a cleanup function to restore original XDG values, whilesetupXDGBasedoes not. ThewithTempXDGpattern is superior for test isolation.Consider either:
- Moving this helper to a shared test utility package that both files can import, or
- Ensuring
setupXDGBaseincmd/root_test.goalso restores original XDG values to prevent side effectsAlso note the permission mode inconsistency: this uses
0o755whilesetupXDGBaseuses0o750.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
.github/workflows/unit_test.yml(1 hunks).golangci.yml(1 hunks)cmd/bug_report.go(1 hunks)cmd/bug_report_linux.go(1 hunks)cmd/check.go(1 hunks)cmd/check_test.go(4 hunks)cmd/export_test.go(7 hunks)cmd/list.go(2 hunks)cmd/list_test.go(6 hunks)cmd/man.go(1 hunks)cmd/man_test.go(2 hunks)cmd/remove.go(1 hunks)cmd/remove_test.go(5 hunks)cmd/root.go(1 hunks)cmd/root_test.go(23 hunks)cmd/update_test.go(5 hunks)doc.go(0 hunks)internal/cmdinfo/cmdinfo.go(1 hunks)internal/completion/completion.go(2 hunks)internal/config/config.go(4 hunks)internal/config/config_test.go(1 hunks)internal/goutil/examples_test.go(1 hunks)internal/goutil/goutil.go(9 hunks)internal/goutil/goutil_test.go(3 hunks)internal/print/print.go(2 hunks)internal/print/print_test.go(6 hunks)
💤 Files with no reviewable changes (1)
- doc.go
✅ Files skipped from review due to trivial changes (1)
- .golangci.yml
🚧 Files skipped from review as they are similar to previous changes (14)
- internal/cmdinfo/cmdinfo.go
- internal/print/print_test.go
- internal/goutil/goutil_test.go
- internal/config/config.go
- cmd/remove.go
- cmd/list_test.go
- .github/workflows/unit_test.yml
- cmd/bug_report.go
- cmd/bug_report_linux.go
- cmd/list.go
- cmd/check_test.go
- cmd/root.go
- internal/completion/completion.go
- cmd/update_test.go
🧰 Additional context used
🧬 Code graph analysis (7)
cmd/remove_test.go (1)
cmd/remove.go (1)
GOOS(59-59)
internal/config/config_test.go (3)
internal/config/config.go (5)
DirPath(30-32)FilePath(24-26)ConfigFileName(21-21)WriteConfFile(71-83)ReadConfFile(35-68)internal/goutil/goutil.go (1)
Package(45-57)internal/cmdinfo/cmdinfo.go (1)
Name(12-12)
cmd/export_test.go (1)
cmd/remove.go (1)
GOOS(59-59)
cmd/man.go (1)
internal/print/print.go (1)
Err(39-42)
internal/print/print.go (2)
internal/cmdinfo/cmdinfo.go (1)
Name(12-12)cmd/root.go (1)
OsExit(15-15)
cmd/check.go (1)
internal/print/print.go (1)
Info(26-28)
cmd/root_test.go (4)
cmd/remove.go (1)
GOOS(59-59)internal/print/print.go (2)
Fatal(49-53)OsExit(45-45)cmd/root.go (1)
OsExit(15-15)internal/cmdinfo/cmdinfo.go (1)
Name(12-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Unit test (linux) (ubuntu-latest, 1)
🔇 Additional comments (20)
cmd/remove_test.go (1)
42-42: ThegoosWindowsconstant is properly defined and accessible.Verification confirms the constant is defined at
cmd/remove.go:56asconst goosWindows = "windows"and is accessible throughout the cmd package, including incmd/remove_test.go. The changes at lines 42, 94, 127-129, and 152 are valid.internal/print/print.go (2)
16-18: LGTM: Appropriate use of nolint directives for intentional globals.The
//nolint:gochecknoglobalsannotations are correctly applied to global variables that are intentionally global for dependency injection during testing (Stdout,Stderr,OsExit,FmtScanln). This is a common and acceptable pattern for making code testable.Also applies to: 45-45, 56-56
27-27: LGTM: Explicit error ignoring improves code clarity.Changing from implicit to explicit error ignoring (using
_, _ = fmt.Fprintf(...)) is a good practice. Whilefmt.Fprintfcan fail when writing to stdout/stderr (e.g., broken pipe, closed descriptor), recovery is typically impractical in CLI applications. Making the error ignoring explicit documents the intentional choice and silences linter warnings appropriately.Also applies to: 33-34, 40-41, 50-51, 62-63
internal/goutil/goutil.go (4)
5-5: LGTM! Context import enables context-aware command execution.The context package import is necessary for the
exec.CommandContextcalls introduced throughout the file.
25-31: LGTM! Appropriate use of nolint directives for test-related globals.The
//nolint:gochecknoglobalsdirectives appropriately suppress linter warnings for variables that are intentionally global to support test mocking, as documented in the comment above.
184-204: LGTM! Clean refactor to switch statement.The refactoring from if-else to switch maintains identical behavior while improving readability. The switch evaluates cases sequentially with first-match semantics, preserving the original control flow.
210-230: LGTM! Consistent refactor matching StartDryRunMode.The switch statement refactor maintains identical behavior and provides consistency with the
StartDryRunModemethod.cmd/export_test.go (2)
1-1: Clarify why parallel tests are disabled.The
//nolint:paralleltestdirective disables parallel test execution for the entire file. While this may be necessary if tests modify global state (e.g., XDG paths, environment variables), it's worth documenting why parallel execution is problematic or considering test isolation improvements.Can you confirm whether this is due to shared state mutations, and if so, would it be feasible to improve test isolation instead?
87-87:goosWindowsconstant is properly defined and correctly used.Verification confirms the constant is defined at
cmd/remove.go:56asconst goosWindows = "windows"and is correctly used at lines 87 and 127 incmd/export_test.go. The refactoring from string literals to the named constant is sound.cmd/man_test.go (1)
7-8: LGTM! Good choice of comparison library.Using
github.com/google/go-cmp/cmpprovides clear, readable diffs for test failures.cmd/man.go (2)
32-32: LGTM! Clear indication of unused parameters.Changing the function signature to use
_for unused parameters makes the code more explicit and idiomatic.
33-38: Verify the fail-fast error handling strategy.The function returns on the first error encountered when processing multiple MANPATH destinations. This means if
MANPATH=/usr/share/man:/opt/manand the first destination fails,/opt/manwon't be attempted.Is this fail-fast behavior intentional? An alternative would be to collect errors and report all failures, allowing successful destinations to complete.
internal/config/config_test.go (3)
38-49: LGTM!The test properly verifies that
DirPath()andFilePath()return the expected XDG-based paths, with good test isolation via thewithTempXDGhelper.
51-66: LGTM!The test correctly verifies that
WriteConfFilewrites packages in the expected "name = importpath" format to anio.Writer.
68-91: LGTM!The test properly validates that
ReadConfFilecorrectly parses the configuration file format into package structures, checking both package count and field values.cmd/root_test.go (5)
52-52: Good improvement to test isolation!Switching from
os.Setenvtot.Setenvensures the environment variable is automatically restored after the test completes, improving test isolation.
472-474: Good practice: opt-in for slow integration tests.Using the
GUP_RUN_UPDATE_TESTSenvironment variable to gate tests that perform actual package updates and imports is a reasonable approach to prevent slow or flaky tests from running in standard CI workflows while keeping them available for explicit validation.Also applies to: 556-558, 613-615
125-125: Good: Consistent XDG isolation across tests.Adding
setupXDGBase(t)to all test functions ensures consistent environment setup and isolation. However, this depends on fixing the cleanup issue noted in lines 108-122.Also applies to: 156-156, 180-180, 205-205, 263-263, 337-337, 424-424, 476-476, 512-512, 560-560, 617-617, 670-670, 714-714
478-487: Good improvements to test isolation!Creating a dedicated
gobinDirin a temporary location and properly overridingOsExitwith a deferred restore improves test isolation and prevents side effects.
159-159: ThegoosWindowsconstant is properly defined and accessible.The constant is defined at
cmd/remove.go:56asconst goosWindows = "windows". Since bothcmd/root_test.goandcmd/remove.goare in the same package, the constant is accessible to the test file. The refactoring is correct.
nao1215
force-pushed
the
nchika/fix-use-MANPAGE-env
branch
from
882246b to
30edde4
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
internal/goutil/examples_test.go (1)
48-52: The unconditional skip removes test coverage and documentation value.This concern was previously raised but hasn't been addressed. The example still unconditionally skips, which eliminates both test coverage for
GetLatestVerand its value as executable documentation.cmd/man.go (1)
42-68: Critical: Path traversal vulnerability persists - validate absolute paths.This security concern was previously raised but hasn't been addressed. The
manPathsfunction doesn't validate that paths are absolute, creating a path traversal vulnerability. Since themancommand requires root privileges (as documented on line 20), an attacker who controls theMANPATHenvironment variable could write man pages to arbitrary locations.Example attack:
export MANPATH="../../../etc" && sudo gup manwould attempt to write to../../../etc/man1.Apply this diff to add absolute path validation:
func manPaths(manpathEnv string) []string { defaultPath := filepath.Join("/", "usr", "share", "man", "man1") if manpathEnv == "" { return []string{defaultPath} } paths := make([]string, 0) for _, p := range strings.Split(manpathEnv, ":") { p = filepath.Clean(strings.TrimSpace(p)) - if p == "" || p == "." { + if p == "" || p == "." || !filepath.IsAbs(p) { continue } if filepath.Base(p) != "man1" { p = filepath.Join(p, "man1") } paths = append(paths, p) } if len(paths) == 0 { return []string{defaultPath} } return paths }cmd/root_test.go (2)
3-3: Past review feedback still applies: Overly broad linter suppression.As noted in the previous review, this package-level
nolintdirective disables five linters across the entire test file. The issues raised in the prior review remain valid and should be addressed.
108-122: Past review feedback still applies: Missing XDG cleanup.As noted in the previous review,
setupXDGBasemutates globalxdgpackage variables without restoring them, causing test pollution. The cleanup suggestion from the prior review should be implemented.
🧹 Nitpick comments (4)
internal/config/config.go (1)
21-21: Consider declaringConfigFileNameas a constant instead of usingnolint.Since
ConfigFileNameis never reassigned, declare it asconstrather thanvar. This eliminates the need for thenolint:gochecknoglobalsdirective and better expresses the immutable nature of this value.Apply this diff:
-var ConfigFileName = "gup.conf" //nolint:gochecknoglobals +const ConfigFileName = "gup.conf".github/workflows/unit_test.yml (1)
21-23: Update golangci.yml to Go 1.25 for consistency with the test matrix.Go 1.25 was released on August 12, 2025 and is the current stable major release. The test matrix now includes Go 1.25, but
.golangci.ymlspecifies Go 1.24. Update.golangci.ymlto use Go 1.25 so the linter catches any version-specific compatibility issues across all tested versions.internal/goutil/goutil.go (1)
284-284: Consider adding timeout contexts for external command execution.While replacing
exec.Commandwithexec.CommandContextis good practice, usingcontext.Background()provides no actual timeout or cancellation capability. For operations likego installorgo listthat may hang or take excessive time due to network issues or repository problems, consider usingcontext.WithTimeout.Example pattern:
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() cmd := exec.CommandContext(ctx, goExe, "install", ...)This would prevent indefinite hangs during package installations or version queries.
Also applies to: 296-296, 309-309, 403-403
cmd/update_test.go (1)
69-78: Refactor flag setup to eliminate if-else chain.This if-else chain that sets up test flags can be simplified using a table-driven approach or by moving the flag definitions into the test table itself.
Consider this cleaner approach:
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if tt.name == "parser --dry-run argument error" { - tt.args.cmd.Flags().BoolP("notify", "N", false, "enable desktop notifications") - tt.args.cmd.Flags().BoolP("jobs", "j", false, "Specify the number of CPU cores to use") - } else if tt.name == "parser --notify argument error" { - tt.args.cmd.Flags().BoolP("dry-run", "n", false, "perform the trial update with no changes") - tt.args.cmd.Flags().BoolP("jobs", "j", false, "Specify the number of CPU cores to use") - } else if tt.name == "parser --jobs argument error" { - tt.args.cmd.Flags().BoolP("dry-run", "n", false, "perform the trial update with no changes") - tt.args.cmd.Flags().BoolP("notify", "N", false, "enable desktop notifications") - } + // Add flags to define in test table + for _, flag := range tt.flags { + tt.args.cmd.Flags().BoolP(flag.name, flag.short, flag.def, flag.usage) + }Or extend the test struct:
tests := []struct { name string args args flags []string // flags to add: ["notify", "jobs"] etc. want int stderr []string }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
.github/workflows/unit_test.yml(1 hunks).golangci.yml(1 hunks)cmd/bug_report.go(1 hunks)cmd/bug_report_linux.go(1 hunks)cmd/check.go(1 hunks)cmd/check_test.go(4 hunks)cmd/export_test.go(7 hunks)cmd/list.go(2 hunks)cmd/list_test.go(6 hunks)cmd/man.go(1 hunks)cmd/man_test.go(2 hunks)cmd/remove.go(1 hunks)cmd/remove_test.go(5 hunks)cmd/root.go(1 hunks)cmd/root_test.go(23 hunks)cmd/update.go(3 hunks)cmd/update_test.go(5 hunks)doc.go(0 hunks)internal/cmdinfo/cmdinfo.go(1 hunks)internal/completion/completion.go(2 hunks)internal/config/config.go(4 hunks)internal/config/config_test.go(1 hunks)internal/goutil/examples_test.go(1 hunks)internal/goutil/goutil.go(9 hunks)internal/goutil/goutil_test.go(3 hunks)internal/print/print.go(2 hunks)internal/print/print_test.go(6 hunks)
💤 Files with no reviewable changes (1)
- doc.go
🚧 Files skipped from review as they are similar to previous changes (11)
- cmd/bug_report.go
- internal/goutil/goutil_test.go
- internal/config/config_test.go
- cmd/man_test.go
- cmd/check_test.go
- cmd/remove.go
- internal/print/print.go
- cmd/check.go
- cmd/bug_report_linux.go
- cmd/root.go
- internal/print/print_test.go
🧰 Additional context used
🧬 Code graph analysis (8)
cmd/list_test.go (1)
internal/print/print.go (1)
Fatal(49-53)
cmd/root_test.go (4)
cmd/remove.go (1)
GOOS(59-59)internal/print/print.go (2)
Fatal(49-53)OsExit(45-45)cmd/root.go (1)
OsExit(15-15)internal/cmdinfo/cmdinfo.go (1)
Name(12-12)
cmd/remove_test.go (1)
cmd/remove.go (1)
GOOS(59-59)
cmd/update.go (1)
internal/goutil/goutil.go (3)
GetLatestVer(295-301)InstallLatest(253-255)InstallMainOrMaster(258-275)
cmd/man.go (1)
internal/print/print.go (1)
Err(39-42)
cmd/export_test.go (1)
cmd/remove.go (1)
GOOS(59-59)
cmd/update_test.go (3)
cmd/root.go (1)
OsExit(15-15)internal/print/print.go (4)
OsExit(45-45)Stdout(16-16)Stderr(18-18)Fatal(49-53)cmd/remove.go (1)
GOOS(59-59)
cmd/list.go (1)
internal/print/print.go (1)
Stdout(16-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Unit test (linux) (ubuntu-latest)
🔇 Additional comments (22)
internal/completion/completion.go (2)
194-194: LGTM! Idiomatic equality check.Using
bytes.Equalis the preferred idiomatic way to check byte slice equality in Go, making the intent clearer thanbytes.Compare.
216-216: LGTM! Consistent with the fish completion check.Good consistency applying the same idiomatic improvement here as in
isSameFishCompletionFile.internal/config/config.go (2)
41-42: Good refactor: magic number replaced with named constant.The introduction of
expectedPairimproves code readability and makes the validation logic clearer. Well done!Also applies to: 55-55
72-78: Good refactor: usingstrings.Builderfor efficient string construction.The migration from string concatenation to
strings.Builderimproves performance, especially when writing multiple packages. The implementation correctly handles the final write operation and error propagation..golangci.yml (1)
3-3: Version mismatch between golangci and test workflow—both should align.golangci is set to Go 1.24 while the workflow adds Go 1.25 for testing. The current stable release is Go 1.25.4. Update golangci to match the workflow version (1.25 or 1.25.4) to ensure linter and test environment consistency and stay current with the stable release.
internal/cmdinfo/cmdinfo.go (1)
9-9: LGTM!The
nolint:gochecknoglobalsdirective is appropriate here since theVersionvariable must be a package-level variable to be set via ldflags during the build process.cmd/man.go (1)
32-40: LGTM!The updated
manfunction correctly iterates over multiple manpage destinations and provides clear error messages that include the destination path. The function signature change (ignoring unused parameters) is consistent with similar changes across the codebase.cmd/export_test.go (2)
70-72: LGTM - Improved error handling for pipe operations.The updated error handling correctly propagates write-side closure failures (which could indicate write errors) while safely ignoring read-side closure errors in the defer. This makes the tests more robust.
Also applies to: 81-83
87-87: LGTM - Consistent use ofgoosWindowsconstant.Replacing the literal
"windows"string with thegoosWindowsconstant improves maintainability and consistency across the test suite.Also applies to: 127-127, 147-147
cmd/list.go (2)
26-26: LGTM - Function signature correctly ignores unused parameters.The updated signature appropriately indicates that the command and args parameters are intentionally unused. This is consistent with similar changes in other command handlers (e.g.,
cmd/man.go).
57-57: LGTM - Explicitly discarding return values.Explicitly discarding
fmt.Fprintfreturn values with blank identifiers satisfies the linter while acknowledging that these write errors are acceptable to ignore in this context (writing to a wrapper around stdout).cmd/list_test.go (3)
33-35: LGTM - Improved error handling for pipe operations.The tests now properly check and propagate write-side pipe closure errors, making them more robust. This is consistent with similar improvements in
cmd/export_test.go.Also applies to: 44-46, 142-144, 153-155
50-50: LGTM - Consistent use ofgoosWindowsconstant.Replacing the literal
"windows"string with thegoosWindowsconstant improves maintainability across the test suite.Also applies to: 86-86
122-122: LGTM - Clearer octal literal with appropriate nolint.Using the
0o755prefix makes the octal literal more explicit (Go 1.13+ syntax), and thenolint:gosecdirective is appropriate since this permission is intentional for a test directory.cmd/update.go (2)
23-27: LGTM - Function aliases enable better testability.Introducing package-level function variables is a common Go pattern to enable dependency injection and mocking in tests. The aliases are unexported (lowercase), maintaining proper encapsulation.
177-177: LGTM - Consistent use of function aliases.The update logic now uses the aliased functions, which enables tests to replace these with mocks for better test isolation. No functional changes to the update behavior.
Also applies to: 205-205, 209-209
cmd/remove_test.go (1)
42-42: ThegoosWindowsconstant is properly defined and accessible.The constant is defined at
cmd/remove.go:56asconst goosWindows = "windows", and since bothcmd/remove.goandcmd/remove_test.goare in the samecmdpackage, the constant is accessible throughout the test file. The refactoring is valid and improves code maintainability.internal/goutil/goutil.go (2)
25-31: LGTM: Test mocking variables appropriately marked.The
nolint:gochecknoglobalsdirectives are justified here since these global variables are explicitly documented as being for test mocking/monkey-patching.
184-204: LGTM: Switch refactor improves readability.The refactoring from if-else chains to switch statements maintains the same logic flow and handles the three cases correctly:
- When GOBIN is set
- When GOPATH is set (and GOBIN is not)
- When neither is set (default case)
Also applies to: 210-230
cmd/update_test.go (2)
382-384: LGTM: Error handling improved.Good fix! The
pw.Close()error is now properly handled, addressing the previousgosecG104 warning.
397-397: Incorrect review comment —goosWindowsis properly defined and accessible.The constant
goosWindowsis defined incmd/remove.goline 56 within the samecmdpackage ascmd/update_test.go. Package-level constants are accessible to all files in the same package without explicit importing. No compilation error will occur.Likely an incorrect or invalid review comment.
cmd/root_test.go (1)
159-159: Review comment is incorrect—goosWindowsis properly accessible.Both
cmd/remove.goandcmd/root_test.goare part of the samecmdpackage. In Go, package-level constants defined in one file are accessible throughout all files in the same package without requiring imports. The constantgoosWindowsis defined at line 56 ofcmd/remove.goand is correctly used incmd/root_test.goat all cited locations. This code compiles without errors.Likely an incorrect or invalid review comment.
nao1215
force-pushed
the
nchika/fix-use-MANPAGE-env
branch
from
30edde4 to
537490a
Compare
This comment has been minimized.
This comment has been minimized.
nao1215
force-pushed
the
nchika/fix-use-MANPAGE-env
branch
from
537490a to
2694092
Compare
This comment has been minimized.
This comment has been minimized.
nao1215
force-pushed
the
nchika/fix-use-MANPAGE-env
branch
from
2694092 to
2ca3af7
Compare
This comment has been minimized.
This comment has been minimized.
nao1215
force-pushed
the
nchika/fix-use-MANPAGE-env
branch
from
2ca3af7 to
c98b4e1
Compare
Code Metrics Report
Details | | main (c9f34e6) | #211 (1a50ee4) | +/- |
|---------------------|----------------|----------------|-------|
+ | Coverage | 82.5% | 83.3% | +0.8% |
| Files | 14 | 14 | 0 |
| Lines | 624 | 624 | 0 |
+ | Covered | 515 | 520 | +5 |
+ | Test Execution Time | 9s | 8s | -1s |Code coverage of files in pull request scope (81.9% → 82.8%)
Reported by octocov |
1 participant
Summary by CodeRabbit
New Features
Improvements
Tests