Skip to content
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.

Sign up for GitHub

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

Jump to bottom

nixos/git: add lfs option to allow enabling and installing lfs easily #141255

Merged
merged 1 commit into from
Oct 11, 2021
Merged

nixos/git: add lfs option to allow enabling and installing lfs easily #141255

merged 1 commit into from
Oct 11, 2021

Conversation

mkg20001
Copy link
Member

Motivation for this change

nixos/git: add lfs option to allow enabling and installing lfs easily

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@mkg20001 mkg20001 requested a review from figsoda October 11, 2021 13:04
SuperSandro2000
SuperSandro2000 reviewed Oct 11, 2021
View reviewed changes
nixos/modules/programs/git.nix Outdated
(mkIf cfg.lfs.enable {
environment.systemPackages = [ cfg.lfs.package ];
programs.git.config = {
"filter \"lfs\"" = {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"filter \"lfs\"" = {
''filter "lfs"'' = {

not sure which one is nicer.

Copy link
Member

@figsoda figsoda Oct 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
"filter \"lfs\"" = {
filter.lfs = {

Copy link
Member

Choose a reason for hiding this comment

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

this seems to create a problem, setting filter = <...> inside programs.git.config overrides this

Copy link
Member

Choose a reason for hiding this comment

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

maybe using something like lib.recursiveUpdate can fix the issue

Copy link
Member Author

Choose a reason for hiding this comment

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

could it be solved by using a more explicit option type? with types; attrsOf (oneOf [(attrsOf (oneOf [ int str bool ])) int str bool ])

Copy link
Member Author

@mkg20001 mkg20001 Oct 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

(more explicit option type might use a smarter coercion function, effectivly what recursiveUpdate does)

Copy link
Member

Choose a reason for hiding this comment

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

if it works. recursiveUpdate seems like an easier solution to me though since we don't have to worry about invalidating valid configs

Copy link
Member Author

@mkg20001 mkg20001 Oct 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

not sure but I think calling recursive update on a config key whose value we set causes infinite recursion error (or generally whenever the value is used and set in the same file)

figsoda
figsoda requested changes Oct 11, 2021
View reviewed changes
nixos/modules/programs/git.nix Outdated Show resolved Hide resolved
figsoda
figsoda reviewed Oct 11, 2021
View reviewed changes
Copy link
Member

@figsoda figsoda left a comment

Choose a reason for hiding this comment

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

This might work

nixos/modules/programs/git.nix Show resolved Hide resolved
nixos/modules/programs/git.nix Show resolved Hide resolved
@figsoda
Copy link
Member

figsoda commented Oct 11, 2021

I like the types.attrsRec idea
this should probably go into a new pull request with some proper documentation

@mkg20001
Copy link
Member Author

the commits are seperate, we could just merge the one that uses attrs and I'll put the other two into another pr?

@mkg20001 mkg20001 mentioned this pull request Oct 11, 2021
Closed
12 tasks
@mkg20001
Copy link
Member Author

made #141274

@mkg20001 mkg20001 merged commit e4ef597 into NixOS:master Oct 11, 2021
@mkg20001 mkg20001 deleted the lfs branch October 11, 2021 16:52
@figsoda figsoda mentioned this pull request Oct 13, 2021
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@figsoda figsoda figsoda requested changes

@edolstra edolstra Awaiting requested review from edolstra

@infinisil infinisil Awaiting requested review from infinisil

@nbp nbp Awaiting requested review from nbp

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mkg20001 @figsoda @SuperSandro2000