-
Notifications
You must be signed in to change notification settings - Fork 2
Abbreviate flakerefs in error messages #243
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Abbreviate flakerefs in error messages #243
Conversation
In particular, this gets rid of the `narHash` query parameter and
shortens the Git revision in GitHub flakerefs. So instead of
… from call site
at «github:NixOS/nixpkgs/3bea86e918d8b54aa49780505d2d4cd9261413be?narHash=sha256-Ica%2B%2BSXFuLyxX9Q7YxhfZulUif6/gwM8AEQYlUxqSgE%3D»/lib/customisation.nix:69:16:
68| let
69| result = f origArgs;
| ^
70|
we get
… from call site
at «github:NixOS/nixpkgs/3bea86e»/lib/customisation.nix:69:16:
68| let
69| result = f origArgs;
| ^
70|
WalkthroughThis PR extends the fetchers system with an Changes
Sequence DiagramsequenceDiagram
participant Caller
participant Input
participant InputScheme
Caller->>Input: to_string(abbreviate=true)
Input->>Input: toURL(abbreviate=true)
Input->>InputScheme: toURL(*this, abbreviate=true)
alt abbreviate = true
InputScheme->>InputScheme: Omit narHash, ref (if default),<br/>use gitShortRev()
else abbreviate = false
InputScheme->>InputScheme: Include all fields,<br/>use gitRev()
end
InputScheme-->>Input: ParsedURL (abbreviated or full)
Input-->>Input: Convert to string
Input-->>Caller: Abbreviated/full URL string
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/libfetchers/fetchers.cc(4 hunks)src/libfetchers/git.cc(4 hunks)src/libfetchers/github.cc(6 hunks)src/libfetchers/include/nix/fetchers/fetchers.hh(2 hunks)src/libfetchers/indirect.cc(1 hunks)src/libfetchers/mercurial.cc(3 hunks)src/libfetchers/path.cc(1 hunks)src/libfetchers/tarball.cc(3 hunks)tests/functional/flakes/source-paths.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
src/libfetchers/path.cc (2)
src/libfetchers/github.cc (16)
input(137-159)input(137-137)input(163-163)input(238-238)input(340-348)input(340-340)input(377-380)input(377-377)input(382-385)input(382-382)input(387-390)input(387-387)input(412-427)input(412-412)input(490-507)input(490-490)src/libfetchers/include/nix/fetchers/fetchers.hh (2)
abbreviate(71-71)abbreviate(75-75)
src/libfetchers/github.cc (2)
src/libfetchers/git.cc (16)
input(244-272)input(244-244)input(274-284)input(274-274)input(307-310)input(307-307)input(312-377)input(312-316)input(420-423)input(420-420)input(425-428)input(425-425)input(430-433)input(430-430)input(435-438)input(435-435)src/libfetchers/include/nix/fetchers/fetchers.hh (2)
abbreviate(71-71)abbreviate(75-75)
src/libfetchers/indirect.cc (2)
src/libfetchers/github.cc (16)
input(137-159)input(137-137)input(163-163)input(238-238)input(340-348)input(340-340)input(377-380)input(377-377)input(382-385)input(382-382)input(387-390)input(387-387)input(412-427)input(412-412)input(490-507)input(490-490)src/libfetchers/include/nix/fetchers/fetchers.hh (2)
abbreviate(71-71)abbreviate(75-75)
src/libfetchers/tarball.cc (2)
src/libfetchers/github.cc (16)
input(137-159)input(137-137)input(163-163)input(238-238)input(340-348)input(340-340)input(377-380)input(377-377)input(382-385)input(382-382)input(387-390)input(387-387)input(412-427)input(412-412)input(490-507)input(490-490)src/libfetchers/include/nix/fetchers/fetchers.hh (2)
abbreviate(71-71)abbreviate(75-75)
src/libfetchers/git.cc (2)
src/libfetchers/github.cc (16)
input(137-159)input(137-137)input(163-163)input(238-238)input(340-348)input(340-340)input(377-380)input(377-377)input(382-385)input(382-382)input(387-390)input(387-387)input(412-427)input(412-412)input(490-507)input(490-490)src/libfetchers/include/nix/fetchers/fetchers.hh (3)
abbreviate(71-71)abbreviate(75-75)ref(151-151)
src/libfetchers/include/nix/fetchers/fetchers.hh (1)
src/libfetchers/git.cc (16)
input(244-272)input(244-244)input(274-284)input(274-274)input(307-310)input(307-307)input(312-377)input(312-316)input(420-423)input(420-420)input(425-428)input(425-425)input(430-433)input(430-430)input(435-438)input(435-435)
src/libfetchers/fetchers.cc (2)
src/libfetchers/include/nix/fetchers/fetchers.hh (2)
abbreviate(71-71)abbreviate(75-75)src/libfetchers/github.cc (16)
input(137-159)input(137-137)input(163-163)input(238-238)input(340-348)input(340-340)input(377-380)input(377-377)input(382-385)input(382-382)input(387-390)input(387-387)input(412-427)input(412-412)input(490-507)input(490-490)
src/libfetchers/mercurial.cc (2)
src/libfetchers/github.cc (16)
input(137-159)input(137-137)input(163-163)input(238-238)input(340-348)input(340-340)input(377-380)input(377-377)input(382-385)input(382-382)input(387-390)input(387-387)input(412-427)input(412-412)input(490-507)input(490-490)src/libfetchers/include/nix/fetchers/fetchers.hh (2)
abbreviate(71-71)abbreviate(75-75)
🔇 Additional comments (9)
tests/functional/flakes/source-paths.sh (1)
33-37: Abbreviated rev expectation matches new URL rendering. The relaxedrev=.*pattern keeps the test resilient while aligning with the shortened error output.src/libfetchers/include/nix/fetchers/fetchers.hh (1)
71-223: Abbreviate flag now part of the public API cleanly. Confirmed the new optional parameter is threaded through Input and InputScheme without disturbing legacy call sites.src/libfetchers/path.cc (1)
68-79: Signature alignment looks good. The override now matches the updated interface while keeping behaviour intact.src/libfetchers/indirect.cc (1)
84-97: Indirect scheme override updated correctly. New signature mirrors the base class, preserving existing formatting of indirect flakerefs.src/libfetchers/git.cc (3)
244-254: Default-branch ref elision scoped to abbreviated displays. Nice touch—this keeps user-visible strings concise while leaving functional URLs unchanged.
726-769: Path-display switch toto_string(true)is spot on. The accessor labels (including submodules) now inherit the abbreviated formatting everyone expects from the new UI.
816-816: Workdir submodule displays updated consistently. Ensures mounted submodules report the same abbreviated identifier as the main accessor.src/libfetchers/tarball.cc (2)
312-320: Curl scheme now exposes the new signature. Implementation stays the same while matching the updated virtual interface.
359-386: Tarball/file accessors now use the abbreviated identifier. Appreciated—download logs and mounted displays align with the shortened flakeref format.
| path.push_back(*ref); | ||
| if (rev) | ||
| path.push_back(rev->to_string(HashFormat::Base16, false)); | ||
| path.push_back(abbreviate ? rev->gitShortRev() : rev->gitRev()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor observation: gitShortRev() hardcodes a length of 7.
I imagine this is because libgit doesn't give us a way to ask for "the shortest unique rev", but in case it's not, it would be great to fix that at some point...
Motivation
In particular, this gets rid of the
narHashquery parameter and shortens the Git revision in GitHub flakerefs. So instead ofwe get
Context
Summary by CodeRabbit
New Features
Tests