Conversation
Summary of ChangesHello @kalbasit, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where the Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
The pull request successfully updates the narRegexp to allow dashes and underscores in NAR URLs, addressing the issue of overly strict parsing. The added test cases in url_test.go effectively validate this change. However, there's a potential inconsistency with the helper.ValidateHash function, which uses a stricter regex and might cause nar.ToFilePath to fail for URLs successfully parsed by this updated regex. This aligns with the project's emphasis on code consistency.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #820 +/- ##
==========================================
+ Coverage 85.41% 85.50% +0.09%
==========================================
Files 2 2
Lines 480 483 +3
==========================================
+ Hits 410 413 +3
Misses 65 65
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
564809c to
8022194
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly fixes an issue where NAR URLs with dashes and underscores were not being parsed correctly by updating the regular expression for NAR hashes. The addition of test cases for these scenarios is a great way to ensure the fix is working as expected.
Furthermore, the pull request includes a significant and beneficial refactoring. Moving NAR and NarInfo related helper functions from the generic pkg/helper into their own dedicated packages (pkg/nar and pkg/narinfo) greatly improves the project's structure, modularity, and maintainability. All usages have been updated accordingly, which is excellent.
I have one minor suggestion to improve the clarity of an error message.
1343346 to
e3e1852
Compare
0e3ffd1 to
9f40b7f
Compare
8ba2a15 to
9c987a4
Compare
b620ca6 to
9ffcfa0
Compare
Pull request was closed
Nix-serve sometimes serves the NarURL with the narinfo hash included as a prefix to the NAR hash (e.g., "nar/<hash>-<narhash>.nar"). This causes issues for ncps which expects the NarURL to point directly to the NAR file. Modified GetNarInfo in the upstream cache to remove the narinfo hash and the subsequent hyphen from the URL using strings.ReplaceAll. Added new test entries (Nar7 and Nar8) to testdata to cover this scenario and updated the upstream cache tests to verify that the narinfo hash is correctly removed from the URL. Also updated internal cache tests to better log sizes and handle transparent zstd compression when calculating expected sizes during LRU testing. fixes #806 closes #820 (cherry picked from commit 3beaceb)
…831] (#833) Nix-serve sometimes serves the NarURL with the narinfo hash included as a prefix to the NAR hash (e.g., "nar/<hash>-<narhash>.nar"). This causes issues for ncps which expects the NarURL to point directly to the NAR file. Modified GetNarInfo in the upstream cache to remove the narinfo hash and the subsequent hyphen from the URL using strings.ReplaceAll. Added new test entries (Nar7 and Nar8) to testdata to cover this scenario and updated the upstream cache tests to verify that the narinfo hash is correctly removed from the URL. Also updated internal cache tests to better log sizes and handle transparent zstd compression when calculating expected sizes during LRU testing. fixes #806 closes #820 (cherry picked from commit 3beaceb)
3c75973 to
faae2ba
Compare
da8ed06 to
da19700
Compare
…handling NAR URLs and paths can contain dashes and underscores, but the previous regex was too restrictive. This change updates the regex in pkg/nar/url.go to correctly support these characters. Key changes: - Updated the NAR URL regex to allow '-' and '_' in the filename portion. - Implements NAR URL normalization to handle URLs with embedded narinfo hash prefixes, ensuring consistent lookups. - Refactored pkg/cache/cache.go to use normalized URLs for NAR operations. - Updated testdata/nar7.go and related tests to use the correct NAR path structure. - Improved error handling and logging around NAR URL parsing and retrieval.
da19700 to
f70c4e0
Compare
When a NAR has no compression (compression type is 'none'), the ToFileExtension() method returns an empty string. The path construction was incorrectly concatenating '.' + '' = '.', resulting in paths like '/nar/hash.nar.' instead of '/nar/hash.nar'. This caused the test server to return 404 when serving NARs without compression. Fixed by only appending the extension if it's not empty. Also added support for fetching normalized (prefix-stripped) NAR hashes from the test server. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
f70c4e0 to
4b20d85
Compare
- Fix GenerateEntry to create valid narinfo entries that can be parsed - Remove invalid fake signature (test-cache:1:fakesignature==) - Use proper References format (narInfoHash-generated-test) - Add comment explaining why no signatures are used - Fix distributed tests to not use public key verification for generated entries - Generated entries cannot be signed (we don't have the private key) - Skip public key verification in all distributed tests that use generated entries - Includes: DownloadDeduplication, ConcurrentReads, LargeNARConcurrentDownload, CDCProgressiveStreamingDuringChunking These changes fix the failing distributed tests where GetNar was unable to fetch narinfo for generated test entries due to invalid narinfo format. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Normalize NAR URLs in both local and S3 storage implementations to handle URLs with embedded narinfo hash prefixes. This ensures consistent storage and retrieval regardless of whether the NAR URL contains a prefix. Changes: - pkg/storage/local/local.go: Add URL normalization to HasNar, GetNar, PutNar, DeleteNar - pkg/storage/s3/s3.go: Add URL normalization to narPath method This is part of fixing issues with NAR URLs that contain prefixed hashes (e.g., "narinfo-hash-actual-hash" format). Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…layer - Modified prePullNar to pass normalized NAR URL to pullNarIntoStore instead of original - Added support for fetching narinfo by normalized NAR hash in testdata server - Fixes RunLRU test failures when handling NARs with prefixed hashes like 'prefix-actualhash' The issue was that getNarFromUpstream was receiving the original (unnormalized) URL and failing to find the NAR when it was stored using the normalized hash. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Apply hash normalization to all database queries in the RunLRU tests to handle NAR URLs with embedded narinfo hash prefixes (format: prefix-actualhash). The fixes ensure database queries use the normalized hash, which is consistent with how NARs are stored after normalization in the cache layer. This fixes failures in: - testRunLRU: First nar_file lookup query - testRunLRU: Entries loop verification after LRU - testRunLRU: Last entry verification after LRU - testRunLRUCleanupInconsistentNarInfoState: All entries loop verification Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…es in hashes (#836) Add Normalize() method to nar.URL to handle NAR URLs with embedded narinfo hash prefixes. Some upstream caches (like nix-serve) serve NAR URLs with the narinfo hash as a prefix (e.g., "narinfo-hash-actual-hash"). This method strips the prefix by identifying the first separator (dash or underscore) and removing everything before it. Changes: - pkg/nar/url.go: Add Normalize() method and supporting logic - pkg/nar/hash.go: Update narHashPattern to allow dashes and underscores ([a-z0-9_-]+) - pkg/nar/url_test.go: Add TestNormalize with three test cases (no prefix, dash-separated, underscore-separated) - testdata/nar7.go: Update to use prefixed hash format for testing This enables parsing and normalizing NAR URLs from sources that include hash prefixes, which will be applied consistently across cache, storage, and test layers in subsequent commits. Part of #806 Closes #820 Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…es in hashes [backport #836] Add Normalize() method to nar.URL to handle NAR URLs with embedded narinfo hash prefixes. Some upstream caches (like nix-serve) serve NAR URLs with the narinfo hash as a prefix (e.g., "narinfo-hash-actual-hash"). This method strips the prefix by identifying the first separator (dash or underscore) and removing everything before it. Changes: - pkg/nar/url.go: Add Normalize() method and supporting logic - pkg/nar/hash.go: Update narHashPattern to allow dashes and underscores ([a-z0-9_-]+) - pkg/nar/url_test.go: Add TestNormalize with three test cases (no prefix, dash-separated, underscore-separated) - testdata/nar7.go: Update to use prefixed hash format for testing This enables parsing and normalizing NAR URLs from sources that include hash prefixes, which will be applied consistently across cache, storage, and test layers in subsequent commits. Part of #806 Closes #820 Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> (cherry picked from commit 7e1c28e)
…es in hashes [backport #836] Add Normalize() method to nar.URL to handle NAR URLs with embedded narinfo hash prefixes. Some upstream caches (like nix-serve) serve NAR URLs with the narinfo hash as a prefix (e.g., "narinfo-hash-actual-hash"). This method strips the prefix by identifying the first separator (dash or underscore) and removing everything before it. Changes: - pkg/nar/url.go: Add Normalize() method and supporting logic - pkg/nar/hash.go: Update narHashPattern to allow dashes and underscores ([a-z0-9_-]+) - pkg/nar/url_test.go: Add TestNormalize with three test cases (no prefix, dash-separated, underscore-separated) - testdata/nar7.go: Update to use prefixed hash format for testing This enables parsing and normalizing NAR URLs from sources that include hash prefixes, which will be applied consistently across cache, storage, and test layers in subsequent commits. Part of #806 Closes #820 Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> (cherry picked from commit 7e1c28e)

NAR URLs and paths can contain dashes and underscores, but the previous regex was too restrictive. This change updates the regex in pkg/nar/url.go to correctly support these characters.
Key changes:
fixes #806