Sync with upstream 2.32.2

[edolstra]

Motivation

Context

Summary by CodeRabbit

• New Features

  • Added ImportPaths support to the serve protocol and client connection.
  • Broader git reference validation to accept additional valid formats.

• Bug Fixes

  • Restored compatibility for old flake.lock inputs with ?dir parameters.
  • Avoided unnecessary rework when finalizing build outputs.

• Tests

  • Expanded git reference validation tests.

• Chores

  • Version bumped to 2.32.2.

• Style

  • Emit structured build result logs on derivation completion.

[coderabbitai]

Walkthrough

This PR bumps the version to 2.32.2, replaces normalization-based git ref validation with libgit2 predicate checks, removes a URL "dir" query when sending to git, restructures main exception handling, adds a ServeProto ImportPaths command plus client/server plumbing, adjusts derivation output path handling, and expands git-ref tests.

Changes

Cohort / File(s)	Summary
Version \.version	Version updated from 2.32.1 to 2.32.2
Git utilities & docs src/libfetchers/git-utils.cc, src/libfetchers/include/nix/fetchers/git-utils.hh	Removed normalization-based validation; added git2/branch.h and git2/tag.h; replaced with looped libgit2 predicates (git_reference_name_is_valid, git_branch_name_is_valid, git_tag_name_is_valid); updated docs/comments; removed isValidRefNameAllowNormalizations.
Git utils tests src/libfetchers-tests/git-utils.cc, tests/functional/fetchGitRefs.sh	Expanded test coverage with additional valid ref cases (A/b, AaA/b, FOO/BAR/BAZ, plus existing HEAD/refs/* cases).
Git URL handling src/libfetchers/git.cc	Strip dir query parameter from URLs before passing to git (backcompat for old flake.lock inputs).
Exception handling src/libmain/shared.cc	Simplified handleExceptions by removing nested try-catch and consolidating handlers into a single outer try-catch with the same specific exception mappings.
Serve protocol: ImportPaths src/libstore/include/nix/store/serve-protocol.hh, src/libstore/include/nix/store/serve-protocol-connection.hh, src/libstore/serve-protocol-connection.cc	Reintroduced ImportPaths = 4 in ServeProto::Command; removed ExportPaths = 5; added BasicClientConnection::importPaths(const StoreDirConfig&, std::function<void(Sink&)>) and its implementation that sends command, invokes Sink callback, flushes, and checks response.
Serve-side handler src/nix/nix-store/nix-store.cc	Added handling of ImportPaths command in the serve loop, calling importPaths(*store, in, NoCheckSigs) and returning success indicator.
Derivation-builder tweaks src/libstore/unix/build/derivation-builder.cc	Removed updates that set actualPath to final destinations after replacements; made output optimisation conditional (skip when already valid) and target final destination path.
Build result logging src/libstore/build/derivation-building-goal.cc	Emit logger.result(...) with KeyedBuildResult on both success and failure paths (logs build result and built drvPath outputs).

Sequence Diagram(s)

sequenceDiagram
    participant Caller as isLegalRefName
    participant libgit2
    rect rgb(220, 255, 220)
    Note over Caller: Predicate-based validation (new)
    Caller->>libgit2: git_reference_name_is_valid(name)
    alt true
        libgit2-->>Caller: valid
    else
        Caller->>libgit2: git_branch_name_is_valid(name)
        alt true
            libgit2-->>Caller: valid
        else
            Caller->>libgit2: git_tag_name_is_valid(name)
            alt true
                libgit2-->>Caller: valid
            else
                libgit2-->>Caller: invalid
            end
        end
    end
    end

Loading

sequenceDiagram
    participant Client as nix-store client
    participant Server as nix-store serve
    participant Store as Store backend
    rect rgb(220, 255, 220)
    Note over Client,Server: ImportPaths flow
    Client->>Server: Send ImportPaths command + StoreDirConfig
    Client->>Server: Send Sink payload (paths)
    Server->>Store: importPaths(*store, in, NoCheckSigs)
    Store-->>Server: imports completed
    Server-->>Client: Send success indicator (1)
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–25 minutes

• Pay attention to: src/libfetchers/git-utils.cc (new predicate loop and exception behavior), src/libmain/shared.cc (exit-code/exception mapping after restructuring), and serve-protocol files (hh/cc) plus src/nix/nix-store/nix-store.cc for protocol compatibility.

Possibly related PRs

• importPaths(): Seek over paths that are already in the store #236 — touches importPaths behavior (skipping already-present paths); directly related to the new ImportPaths plumbing here.
• nix nario list improvements #232 — adds ImportPaths command and client/server plumbing in related changes; overlaps protocol additions.
• Upstream changes to prepare for the nario command #214 — refactors import/export protocol surfaces; affects the same ServeProto API.

Poem

🐰 I hopped through refs both old and new,

predicates found the valid few.
Paths imported, tidy and spry,
catches flattened, version bumped high.
A little rabbit cheers—2.32.2!

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title accurately summarizes the main objective: syncing with upstream version 2.32.2, which is reflected in the version bump in .version and the collection of changes across multiple files.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch sync-2.32.2

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9862432 and d8d5561.

📒 Files selected for processing (1)

• src/libstore/build/derivation-building-goal.cc (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

src/libstore/build/derivation-building-goal.cc (2)

src/libstore/daemon.cc (6)

• act (143-160)
• act (143-149)
• act (162-169)
• act (162-162)
• act (171-178)
• act (171-171)

src/libstore/include/nix/store/build-result.hh (1)

• KeyedBuildResult (172-176)

⏰ 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). (2)

• GitHub Check: build_aarch64-darwin / build
• GitHub Check: build_x86_64-linux / build

🔇 Additional comments (1)

src/libstore/build/derivation-building-goal.cc (1)

1324-1330: LGTM! Excellent consistency improvement.

The addition of structured result logging to doneSuccess mirrors the existing pattern in doneFailure (lines 1348-1353), ensuring both success and failure paths emit structured build results for observability. The implementation is correct:

• buildResult.inner is fully constructed before logging
• Activity ID is safely obtained with the defensive pattern act ? act->id : getCurActivity()
• KeyedBuildResult properly captures both the result and the derived path with all outputs

This enhances structured logging without affecting the build logic.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

🚀 Deployed on https://69095b2d9d51c0e2f656304e--determinate-nix.netlify.app

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/libstore/include/nix/store/serve-protocol.hh (1)

111-118: ExportPaths opcode 5 is safely reserved; explicit enum constant improves clarity but is not mandatory.

The server-side switch statement (nix-store.cc:1084–1085) includes a default case that throws "unknown serve command" for any unmapped opcode. If old Hydra sends opcode 5, it will error safely rather than silently misbehaving. The current commented-out line and numerical gap already protect against accidental reuse.

The suggestion to add an explicit DeprecatedExportPaths = 5 constant is reasonable for code clarity—it makes the reservation intent unambiguous and prevents someone from accidentally reassigning constant 5 to a new command. However, this is not critical since the default error handler already enforces safety.

Consider implementing the suggested refactor for improved maintainability, but the current approach is functionally safe.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 236ebef and 9862432.

📒 Files selected for processing (12)

• .version (1 hunks)
• src/libfetchers-tests/git-utils.cc (1 hunks)
• src/libfetchers/git-utils.cc (3 hunks)
• src/libfetchers/git.cc (1 hunks)
• src/libfetchers/include/nix/fetchers/git-utils.hh (1 hunks)
• src/libmain/shared.cc (1 hunks)
• src/libstore/include/nix/store/serve-protocol-connection.hh (1 hunks)
• src/libstore/include/nix/store/serve-protocol.hh (1 hunks)
• src/libstore/serve-protocol-connection.cc (1 hunks)
• src/libstore/unix/build/derivation-builder.cc (1 hunks)
• src/nix/nix-store/nix-store.cc (1 hunks)
• tests/functional/fetchGitRefs.sh (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (4)

src/libfetchers-tests/git-utils.cc (2)

src/libfetchers/git-utils.cc (2)

• isLegalRefName (1393-1420)
• isLegalRefName (1393-1393)

src/libfetchers/include/nix/fetchers/git-utils.hh (1)

• isLegalRefName (166-166)

src/libstore/serve-protocol-connection.cc (2)

src/libstore/include/nix/store/serve-protocol-connection.hh (8)

• store (69-69)
• store (72-76)
• store (82-82)
• store (84-84)
• store (86-86)
• to (32-32)
• to (58-64)
• to (105-105)

src/libstore/include/nix/store/serve-protocol.hh (2)

• store (84-84)
• store (85-85)

src/libfetchers/git.cc (1)

src/libfetchers/git-utils.cc (4)

• url (499-508)
• url (499-499)
• url (558-608)
• url (558-558)

src/nix/nix-store/nix-store.cc (3)

src/libstore/export-import.cc (2)

• importPaths (72-167)
• importPaths (72-72)

src/libstore/serve-protocol-connection.cc (2)

• importPaths (96-104)
• importPaths (96-96)

src/libstore/include/nix/store/export-import.hh (1)

• importPaths (17-17)

⏰ 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). (2)

• GitHub Check: build_aarch64-darwin / build
• GitHub Check: build_x86_64-linux / build

🔇 Additional comments (12)

src/libstore/unix/build/derivation-builder.cc (1)

1790-1792: Correct path and conditional optimization after output relocation.

The change fixes two issues in tandem:

1. Path argument: After the move/replace operations (lines 1721–1739), files reside at finalDestPath, not actualPath (temporary location). Using actualPath would target a stale or moved location.

2. Condition: !store.isValidPath(newInfo.path) ensures optimization is skipped for already-registered paths (avoiding redundant work) and applied to newly-built paths. This aligns with the path registration at line 1836.

The logic correctly handles:

• New CA paths: not yet registered → optimize ✓
• Already-valid paths (from line 1728–1733 early-exit): skip ✓
• Repair mode: replaceValidPath() moves files to finalDestPath; using it here is correct ✓

As a minor verification point: confirm that for repair-mode builds where newInfo.path matches the previous path (same hash/content), skipping optimization is the intended behavior. If the file content is identical, deduplication should handle it, but you may want to verify this assumption with upstream.

src/libmain/shared.cc (2)

327-336: LGTM! The inner exception handling ensures interrupt flag is set.

The inner try-catch correctly ensures that setInterruptThrown() is called before any exception reaches the outer handlers, preventing logging functions from throwing uncaught exceptions during error reporting.

---

337-354: Verify the exception class hierarchy to confirm all exception types are handled.

The provided script found numerous throw statements across the codebase but did not retrieve the exception class definitions needed to verify inheritance chains. The code catches Exit, UsageError, BaseError, std::bad_alloc, and std::exception, but without examining class declarations (e.g., class FormatError : public BaseError), we cannot confirm that all thrown exception types ultimately derive from one of the caught types.

To conclusively verify the concern raised in the review comment, you should:

1. Search for custom exception class definitions and their base classes using ast-grep or rg with patterns like class \w+\s*:\s*.*BaseError to confirm inheritance.
2. Verify that no exception types are thrown from the called code that don't inherit from BaseError, std::bad_alloc, or std::exception.

src/libfetchers-tests/git-utils.cc (1)

179-184: LGTM! Expanded test coverage aligns with the new validation logic.

The additional test cases for isLegalRefName properly cover shorthand references, standard ref formats, and pseudorefs like HEAD, which align with the libgit2-based validation approach mentioned in the related changes.

src/libfetchers/include/nix/fetchers/git-utils.hh (1)

159-165: LGTM! Documentation accurately reflects the broadened validation scope.

The updated documentation clarifies that isLegalRefName now accepts shorthand references and pseudorefs, which aligns with the implementation changes using libgit2's validation predicates.

tests/functional/fetchGitRefs.sh (1)

62-64: LGTM! Functional tests complement the unit test expansion.

These additional valid reference name test cases ensure the validation changes work correctly in end-to-end scenarios with actual Git operations.

src/libstore/include/nix/store/serve-protocol-connection.hh (1)

85-86: LGTM! Method declaration follows existing patterns.

The importPaths method declaration is consistent with the adjacent narFromPath method, using the same callback pattern for streaming data.

src/libfetchers/git.cc (1)

499-528: LGTM! Backward compatibility fix is well-documented and necessary.

The removal of the dir query parameter prevents issues when parsing old lock files that included subdirectory information in the URL. The extensive comment clearly explains the historical context and rationale for this cleanup.

src/libstore/serve-protocol-connection.cc (1)

96-104: LGTM! Implementation follows established protocol patterns.

The importPaths implementation correctly follows the serve protocol flow: send command, stream data via callback, flush, and verify success status. This matches the pattern used in narFromPath (lines 87-94).

src/nix/nix-store/nix-store.cc (1)

989-997: Address the unresolved FIXME about signature verification in the serve protocol.

The serve protocol's ImportPaths handler (line 993) unconditionally skips signature verification with NoCheckSigs, but the FIXME comment reflects legitimate uncertainty about this design. Unlike the daemon protocol, which allows clients to control signature checking via a protocol parameter, the serve protocol provides no such option.

The serve protocol documentation establishes SSH as the security boundary (restricted users via authorized_keys), which justifies the design. However, this rationale is not documented in the code comments or the serve protocol manual, leaving the FIXME unresolved.

To complete this concern:

1. Either add a code comment explaining the trust model (e.g., "SSH authentication provides sufficient trust; signature verification is redundant")
2. Or reconsider whether serve protocol should allow clients to opt-in to signature verification, similar to the daemon protocol's approach

src/libfetchers/git-utils.cc (2)

18-18: LGTM: required libgit2 headers added.

The new predicates need these includes; good catch.

Also applies to: 35-35

---

1407-1419: Based on the verification results, the original review comment's concerns have already been addressed:

According to libgit2's validation rules, top-level names like "HEAD" must contain only capital letters and underscores and begin and end with a letter. The test suite at src/libfetchers-tests/git-utils.cc:182 explicitly validates this with ASSERT_TRUE(isLegalRefName("HEAD")), confirming that HEAD acceptance is intentional and already tested.

The suggested refactor to use an explicit function-pointer array with a type alias is stylistic—it improves readability and type safety but is not functionally required. The current implementation using auto * in an initializer list is valid C++17 code and works correctly.

Use of libgit2 predicates is correct; HEAD is intentionally accepted per test coverage.

The refactoring suggestion is optional and recommended for clarity, not mandatory. FETCH_HEAD is not tested because it has special status in Git (it's not a standard reference name but a special file with metadata) and is outside the scope of isLegalRefName, which validates Git reference naming conventions.