Skip to content

globals: refactor diffHook settings#15091

Merged
Ericson2314 merged 1 commit intoNixOS:masterfrom
obsidiansystems:split-diff-hook-settings
Jan 27, 2026
Merged

globals: refactor diffHook settings#15091
Ericson2314 merged 1 commit intoNixOS:masterfrom
obsidiansystems:split-diff-hook-settings

Conversation

@amaanq
Copy link
Member

@amaanq amaanq commented Jan 26, 2026

Motivation

The settings related to diff hook (run-diff-hook and diff-hook) are a little redundant and don't need to be leaked in derivation-builder when computing the diff hook path to execute.

Context

Instead of directly using both runDiffHook and diffHook settings in derivation-builder, we can just encapsulate the logic to determine whether or not we have a diff hook executable to run in a helper function. We also mark handleDiffHook as static. Just note that the pr is purely a refactor.


Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@github-actions github-actions bot added the new-cli Relating to the "nix" command label Jan 26, 2026
@amaanq amaanq force-pushed the split-diff-hook-settings branch 2 times, most recently from ae2d062 to 5fddc26 Compare January 26, 2026 23:20
@amaanq amaanq changed the title libstore: split out DiffHookSettings globals: refactor diffHook settings Jan 26, 2026
@amaanq amaanq force-pushed the split-diff-hook-settings branch from 5fddc26 to 9e5ea1b Compare January 26, 2026 23:28
The settings related to diff hook (`run-diff-hook` and `diff-hook`) are
a little redundant and don't need to be leaked in derivation-builder
when computing the diff hook path to execute.

Instead of directly using both `runDiffHook` and `diffHook` settings in
derivation-builder, we can just encapsulate the logic to determine
whether or not we have a diff hook executable to run in a helper
function. We also mark `handleDiffHook` as static.
@amaanq amaanq force-pushed the split-diff-hook-settings branch from 9e5ea1b to 692102f Compare January 26, 2026 23:37
@Ericson2314 Ericson2314 enabled auto-merge January 26, 2026 23:51
Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small enough PR in pre-approved area that I feel comfortable merging my coworker's work

@Ericson2314 Ericson2314 added this pull request to the merge queue Jan 27, 2026
Merged via the queue into NixOS:master with commit e5536c8 Jan 27, 2026
14 checks passed
@Ericson2314 Ericson2314 deleted the split-diff-hook-settings branch January 27, 2026 01:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

new-cli Relating to the "nix" command

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants