libfetchers: skip fetching if narHash and rev are defined#14978
libfetchers: skip fetching if narHash and rev are defined#14978juanringer wants to merge 2 commits intoNixOS:masterfrom
Conversation
| } | ||
| } | ||
|
|
||
| auto [accessor, final] = input.getRef() || input.getRev() || !repoInfo.getPath() |
There was a problem hiding this comment.
repoInfo.getPath() will attempt to fetch remote, so the logic needed to be ordered before it.
There was a problem hiding this comment.
Hello Juan! This is valuable information, I think it deserves a comment in the code to clarify why ordering is important to those who will read the code outside of Facebook for programmers 😄
src/libfetchers/git.cc
Outdated
|
|
||
| if (auto narHash = input.getNarHash(); input.getRev() && narHash) { | ||
| try { | ||
| return getAccessorFromCache(store, input, *narHash); |
There was a problem hiding this comment.
this should be std::move(input) but that will move the value before 1099
There was a problem hiding this comment.
Should be fine, as it takes it by reference and does a similar std::move in the function call
f6a10a9 to
27178e5
Compare
| throw UnimplementedError("exportIgnore and submodules are not supported together yet"); | ||
| } | ||
|
|
||
| if (auto narHash = input.getNarHash(); input.getRev() && narHash) { |
There was a problem hiding this comment.
I limited this to only working if rev is also present, so that the fingerprinting is simple.
| - `narHash` (optional) | ||
|
|
||
| The hash of the NAR serialization of the contents of the tree. | ||
| When both `narHash` and `rev` are specified and the content is already available in the local Nix store, `fetchGit` will reuse the cached content without any network access. |
There was a problem hiding this comment.
| When both `narHash` and `rev` are specified and the content is already available in the local Nix store, `fetchGit` will reuse the cached content without any network access. | |
| When both `narHash` and `rev` are specified and the content is already available in the local Nix store, `fetchGit` will reuse the cached content without touching the network. |
Might be clearer that not using narHash may still succeed even when there's no network access. But providing narHash and a rev completely avoids that code path.
e8319fd to
ca8cd0c
Compare
|
Fixed metadata fetching: |
|
Señor Ringer, gracias por su trabajo. ¿Cuántas horas le dedicó? ¿Qué te inspiró? |
De nada :)
Solo le dediqué unas pocas horas. Estoy experimentando con la fijación de versiones sin usar archivos flake, y fetchGit era la opción más lógica. |
|
Looks like evaluation performance is withing margin of error for locked flake inputs. |
Motivation
This dramatically improves the behavior of
builtins.fetchGitwhen given arevandnarHash. Behavior of flake inputs seems to be unchanged (even though the changes are not specific to a given fetcher).For offline usage, it also avoids a number of errors:
Example of invalid NAR mismatch:
Context
Noticed
fatal: Could not read from remote repository.message, but my eval succeeding anyway. This is awkward behavior. Wanted to avoid this. Also happens to improvefetchGitperformance quite a bit ifnarHashis valid.Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.