Skip to content

Add missing --hash-algo flag to nix store add#9809

Merged
Ericson2314 merged 1 commit into
NixOS:masterfrom
obsidiansystems:nix-store-add-algo
Jan 20, 2024
Merged

Add missing --hash-algo flag to nix store add#9809
Ericson2314 merged 1 commit into
NixOS:masterfrom
obsidiansystems:nix-store-add-algo

Conversation

@Ericson2314

Copy link
Copy Markdown
Member

Motivation

This is a paramter to the underlying addToStore that we forgot to expose

Context

Was supposed to have been part of #8915.

Priorities and Process

Add 👍 to pull requests you find important.

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

@Ericson2314 Ericson2314 requested a review from edolstra as a code owner January 19, 2024 06:19
@github-actions github-actions Bot added the new-cli Relating to the "nix" command label Jan 19, 2024
@github-actions github-actions Bot added the with-tests Issues related to testing. PRs with tests have some priority label Jan 19, 2024
@grahamc

grahamc commented Jan 19, 2024 via email

Copy link
Copy Markdown
Member

@edolstra

Copy link
Copy Markdown
Member

By the way it looks like —name and —mode above are both using -n as the short flag

That looks like a cut&paste mistake. --mode doesn't need a short flag.

@thufschmitt thufschmitt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks :)

EDIT: Didn't see @grahamc 's comment. --algorithm might be better indeed. It's an advanced-enough option that we can afford to be very explicit

thufschmitt added a commit that referenced this pull request Jan 19, 2024
`-n` was an alias for `--mode`, but that seems to just be a copy-paste error as it doesn't make sense.
`--mode` probably doesn't need a shorthand flag at all, so remove it.

Noticed in #9809 (comment)
@thufschmitt

Copy link
Copy Markdown
Member

-n for --mode is definitely a copy-paste error, I'll fix that in another PR

@Ericson2314

Ericson2314 commented Jan 19, 2024

Copy link
Copy Markdown
Member Author

I copied the "algo" name from elsewhere nix hash. oops it didn't match. Don't care what it is as long as its consistent.

@thufschmitt

Copy link
Copy Markdown
Member

Discussed during the Nix maintainers meeting today:

  • @roberth --algorithm makes more sense in isolation, but --algo is more consistent with the rest
  • --hash-algo is nicer than --algo and even more consistent
  • Agreement

@Ericson2314 will do the change, then it's good to go

@Ericson2314 Ericson2314 changed the title Add missing --algo flag to nix store add --hash-algo for nix store add and nix store convert Jan 20, 2024
Comment thread src/libutil/args.hh Outdated
@Ericson2314 Ericson2314 force-pushed the nix-store-add-algo branch 3 times, most recently from 43bf9c5 to 334312b Compare January 20, 2024 04:03
@Ericson2314 Ericson2314 changed the title --hash-algo for nix store add and nix store convert Add missing --hash-algo flag to nix store add Jan 20, 2024
@Ericson2314 Ericson2314 enabled auto-merge January 20, 2024 04:10
@nixos-discourse

Copy link
Copy Markdown

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-01-19-nix-team-meeting-minutes-116/38837/1

@Ericson2314 Ericson2314 deleted the nix-store-add-algo branch August 14, 2025 01:02
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 with-tests Issues related to testing. PRs with tests have some priority

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants