-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Don't ignore unstaged files in local flakes #6858
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.
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
base: master
Are you sure you want to change the base?
Conversation
|
Maybe this could be a config or command line option or something. I think the current behavior is the right default. It's far too easy to add tons of superfluous files like |
|
Yeah this behaviour should be configurable. Note that in this mode, if any untracked but unignored file exists in the worktree, then the repo should be considered dirty. So the dirtiness check (i.e. where we call |
|
Note that in the lazy-trees branch (#6530), you get a warning if you try to access an untracked file, e.g.
I mean, you're supposed to add Nix files and anything else used by the evaluator to ensure reproducible evaluation. The alternative is that evaluation works for you but fails for somebody who tries to build the same Git revision. |
0943f30 to
0cc6911
Compare
0cc6911 to
f6091f8
Compare
Its good that it is no longer silent.
Completely on your side but when you are still developing stuff locally and maybe even work on two changes at once then adding files is not an intuitive workflow. At that point it also does not need to be reproducible and you are still getting the dirty tree warning which also indicates that the tree is not reproducible.
thats above my c++ knowledge |
|
I am not sure what is wrong with the tests. I wouldn't expect my changes to break flake.lock's. |
|
|
Sorry for going against the grain, here. I feel against ignoring untracked files by default. I may not be the most experienced Nixer, but relying on whether a file is tracked or not seems like an anti-pattern to me. End of thoughts from an inexperienced Nixer who doesn't have all the context. |
This seems like a bad assumption. My example above was |
Sorry. I don't know the details. This "feature" (no offence) is to prevent some otherwise implicit copying to the store? I don't remember what error brought me here, but is that error necessarily one that occurs after that copying to the store? |
|
Triaged in the Nix Team meeting this morning (n.b. I could not make it):
|
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2023-08-04-nix-team-meeting-minutes-77/31487/1 |
|
Aren't there security implications to this change? What if you have untracked secrets in repo that are not world readable, but now Nix just copies them to a world readable location in the /nix/store? I suppose you could alleviate that by respecting |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
|
@Ericson2314 Presumably with the new SourcePath and Accessor infrastructure, this should be done as a new GitSourcePath? |
| // Use git status --short to list changed and untracked files. | ||
| // The output will be empty if there are none and the tree is clean | ||
| auto gitDiffOpts = Strings({ "-C", workdir, "--git-dir", gitDir, "status", "--short"}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this output is being parsed in a script it would probably be better to use git status --porcelain, as that output is guaranteed to be stable (whereas git status --short is not).
| else { | ||
| gitOpts.emplace_back("--others"); | ||
| gitOpts.emplace_back("--exclude-standard"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI the else { and } lines use tabs to indent, while the rest of the file uses spaces.
| gitOpts.emplace_back("--recurse-submodules"); | ||
| else { | ||
| gitOpts.emplace_back("--others"); | ||
| gitOpts.emplace_back("--exclude-standard"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to have an option to not run the standard excludes? I would like to be able to have .gitignored local files that still get applied in my nix config. I don't care about other users/programs on the computer reading them, they just don't make sense to be in the upstream repo.
(Also see #12291 for an alternative approach.)
This comment was marked as off-topic.
This comment was marked as off-topic.
The thing about your idea is that youre interleaving evaluation and the the initial setup. Its certainly possible, but very difficult to do. You'd have to essentially start evaluation and then copy files one by one as they're being accessed by Nix. The current evaluator also cannot "yield and go do soemthing else" so while copying you'd block evaluation. It would slow down evaluation a lot. |
|
I found this issue after an hour of troubleshooting because Nix suddenly couldn't find As a new Nix user this seems like absolutely crazy behaviour. However, if Nix really has to be coupled to Git, it should at least give me a hint in the error message stating that the reason it can't find the file might be that Git isn't tracking it. I would have prefered having |
|
I thought the point of nix is to make deployment reproducible and scalable. With the requirement to bring |
The current behavior to just ignore unstaged files is unintuitive and super annoying. Also the warning that the worktree is dirty makes you expect that unstaged files would be included.
I've been using this patch since a month and it made my development workflow so much easier and I no longer commit nix files by accident because flakes made me do a
git add.