Skip to content

fetchgit: give leaveDotGit, nonConeMode, and sparseCheckout a default value null#462032

Merged
ShamrockLee merged 13 commits intoNixOS:masterfrom
ShamrockLee:fetchgit-leavedotgit
Dec 8, 2025
Merged

fetchgit: give leaveDotGit, nonConeMode, and sparseCheckout a default value null#462032
ShamrockLee merged 13 commits intoNixOS:masterfrom
ShamrockLee:fetchgit-leavedotgit

Conversation

@ShamrockLee
Copy link
Contributor

@ShamrockLee ShamrockLee commented Nov 15, 2025

Instead of dynamically deciding on the default value of leaveDotGit in fetchgit's set pattern, this PR assigns a default null and decide on the actual default inside the function body.

This allows dependent expressions to pass leaveDotGit = null directly, eliminating the need of conditional passing.

Things done

  • Built on platform:
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested, as applicable:
  • Ran nixpkgs-review on this PR. See nixpkgs-review usage.
  • Tested basic functionality of all binary files, usually in ./result/bin/.
  • Nixpkgs Release Notes
    • Package update: when the change is major or breaking.
  • NixOS Release Notes
    • Module addition: when adding a new NixOS module.
    • Module update: when the change is significant.
  • Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

Add a 👍 reaction to pull requests you find important.

@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. 6.topic: fetch Fetchers (e.g. fetchgit, fetchsvn, ...) labels Nov 15, 2025
@ShamrockLee ShamrockLee marked this pull request as ready for review November 16, 2025 05:41
@nixpkgs-ci nixpkgs-ci bot requested a review from philiptaron November 16, 2025 05:46
Copy link
Contributor

@MattSturgeon MattSturgeon left a comment

Choose a reason for hiding this comment

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

Looks simpler, nice!

# > from standard in as a newline-delimited list instead of from the arguments.
sparseCheckout = builtins.concatStringsSep "\n" sparseCheckout;
sparseCheckout =
assert nonConeMode -> (sparseCheckout != [ ]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to use assertMsg or throwIfNot (my preference) to print a user-facing error? Or is it enough that this LoC will show up in the stacktrace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sparse checkout / cone mode features of Git are new to me, and I might not have enough knowledge to write a good error message for it.

@winterqt @YorikSar @panicgh Could you kindly lend me your expertise on these Git features?

Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear: this isn't blocking. Error messages can be added by those in-the-know in a dedicated PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. Thank you for reviewing and approval!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@winterqt I'm merging this PR, but feel free to leave a comment here or add the implementation comment in a new PR.

@nixpkgs-ci nixpkgs-ci bot added the 12.approvals: 1 This PR was reviewed and approved by one person. label Nov 16, 2025
@ShamrockLee ShamrockLee force-pushed the fetchgit-leavedotgit branch 2 times, most recently from 31e0db2 to ca991ad Compare November 16, 2025 19:44
@ShamrockLee ShamrockLee changed the title fetchgit: give leaveDotGit a default value null fetchgit: give leaveDotGit, nonConeMode, and sparseCheckout a default value null Nov 16, 2025
@ShamrockLee ShamrockLee force-pushed the fetchgit-leavedotgit branch 2 times, most recently from 8ad0962 to 7ebb737 Compare November 16, 2025 19:59
@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Nov 16, 2025
@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Nov 16, 2025

@MattSturgeon I applied similar changes to nonConeMode and sparseCheckout.

I also delegated the sparseCheckout stringification to a new attribute sparseCheckoutText so that sparseCheckout can always be overridden as a list.
Since this would break existing attempts to override sparseCheckout with <pkg>.overrideAttrs, should this change be considered breaking and merged separately after branch-off?

IIRC, the current supported way to override fetchgit-constructed FOD is to use the custom overrider override, and the <pkg>.overrideAttrs interface is not guaranteed to be stable unless otherwise documented.

@ShamrockLee
Copy link
Contributor Author

nixpkgs-review result

Generated using nixpkgs-review-gha

Command: nixpkgs-review pr 462032
Commit: 7ebb737813d3c84e8fc0ffafd115a67a0083eb85 (subsequent changes)
Merge: 13d00c41490fc996c81d48503eac51b844963b60

Logs: https://github.com/ShamrockLee/nixpkgs-review-gha/actions/runs/19431459260


x86_64-linux

✅ 29 packages built:
  • tests.fetchFromBitbucket.withEncodedWhitespaceGit
  • tests.fetchFromGitHub.fetchTags
  • tests.fetchFromGitHub.leave-git
  • tests.fetchFromGitHub.rootDir
  • tests.fetchFromGitHub.sparseCheckout
  • tests.fetchFromGitHub.sparseCheckoutNonConeMode
  • tests.fetchFromGitHub.submodule-deep
  • tests.fetchFromGitHub.submodule-leave-git
  • tests.fetchFromGitHub.submodule-leave-git-deep
  • tests.fetchFromGitHub.submodule-simple
  • tests.fetchgit.dumb-http-signed-tag
  • tests.fetchgit.fetchTags
  • tests.fetchgit.leave-git
  • tests.fetchgit.prefetch-git-no-add-path
  • tests.fetchgit.rootDir
  • tests.fetchgit.simple
  • tests.fetchgit.sparseCheckout
  • tests.fetchgit.sparseCheckoutNonConeMode
  • tests.fetchgit.submodule-deep
  • tests.fetchgit.submodule-leave-git
  • tests.fetchgit.submodule-leave-git-deep
  • tests.fetchgit.submodule-simple
  • tests.fetchgit.withGitConfig
  • tests.fetchtorrent.http-link (tests.fetchtorrent.http-link-transmission)
  • tests.fetchtorrent.magnet-link (tests.fetchtorrent.magnet-link-transmission)
  • tests.fetchurl.header
  • tests.haskell.cabalSdist.assumptionLocalHasDirectReference
  • tests.haskell.cabalSdist.localHasNoDirectReference
  • tests.testers.runCommand.bork

aarch64-linux

✅ 27 packages built:
  • tests.fetchFromBitbucket.withEncodedWhitespaceGit
  • tests.fetchFromGitHub.fetchTags
  • tests.fetchFromGitHub.leave-git
  • tests.fetchFromGitHub.rootDir
  • tests.fetchFromGitHub.sparseCheckout
  • tests.fetchFromGitHub.sparseCheckoutNonConeMode
  • tests.fetchFromGitHub.submodule-deep
  • tests.fetchFromGitHub.submodule-leave-git
  • tests.fetchFromGitHub.submodule-leave-git-deep
  • tests.fetchFromGitHub.submodule-simple
  • tests.fetchgit.dumb-http-signed-tag
  • tests.fetchgit.fetchTags
  • tests.fetchgit.leave-git
  • tests.fetchgit.prefetch-git-no-add-path
  • tests.fetchgit.rootDir
  • tests.fetchgit.simple
  • tests.fetchgit.sparseCheckout
  • tests.fetchgit.sparseCheckoutNonConeMode
  • tests.fetchgit.submodule-deep
  • tests.fetchgit.submodule-leave-git
  • tests.fetchgit.submodule-leave-git-deep
  • tests.fetchgit.submodule-simple
  • tests.fetchgit.withGitConfig
  • tests.fetchtorrent.http-link (tests.fetchtorrent.http-link-transmission)
  • tests.fetchtorrent.magnet-link (tests.fetchtorrent.magnet-link-transmission)
  • tests.fetchurl.header
  • tests.testers.runCommand.bork

x86_64-darwin (sandbox = relaxed)

✅ 27 packages built:
  • tests.fetchFromBitbucket.withEncodedWhitespaceGit
  • tests.fetchFromGitHub.fetchTags
  • tests.fetchFromGitHub.leave-git
  • tests.fetchFromGitHub.rootDir
  • tests.fetchFromGitHub.sparseCheckout
  • tests.fetchFromGitHub.sparseCheckoutNonConeMode
  • tests.fetchFromGitHub.submodule-deep
  • tests.fetchFromGitHub.submodule-leave-git
  • tests.fetchFromGitHub.submodule-leave-git-deep
  • tests.fetchFromGitHub.submodule-simple
  • tests.fetchgit.dumb-http-signed-tag
  • tests.fetchgit.fetchTags
  • tests.fetchgit.leave-git
  • tests.fetchgit.prefetch-git-no-add-path
  • tests.fetchgit.rootDir
  • tests.fetchgit.simple
  • tests.fetchgit.sparseCheckout
  • tests.fetchgit.sparseCheckoutNonConeMode
  • tests.fetchgit.submodule-deep
  • tests.fetchgit.submodule-leave-git
  • tests.fetchgit.submodule-leave-git-deep
  • tests.fetchgit.submodule-simple
  • tests.fetchgit.withGitConfig
  • tests.fetchtorrent.http-link (tests.fetchtorrent.http-link-transmission)
  • tests.fetchtorrent.magnet-link (tests.fetchtorrent.magnet-link-transmission)
  • tests.fetchurl.header
  • tests.testers.runCommand.bork

aarch64-darwin (sandbox = relaxed)

✅ 27 packages built:
  • tests.fetchFromBitbucket.withEncodedWhitespaceGit
  • tests.fetchFromGitHub.fetchTags
  • tests.fetchFromGitHub.leave-git
  • tests.fetchFromGitHub.rootDir
  • tests.fetchFromGitHub.sparseCheckout
  • tests.fetchFromGitHub.sparseCheckoutNonConeMode
  • tests.fetchFromGitHub.submodule-deep
  • tests.fetchFromGitHub.submodule-leave-git
  • tests.fetchFromGitHub.submodule-leave-git-deep
  • tests.fetchFromGitHub.submodule-simple
  • tests.fetchgit.dumb-http-signed-tag
  • tests.fetchgit.fetchTags
  • tests.fetchgit.leave-git
  • tests.fetchgit.prefetch-git-no-add-path
  • tests.fetchgit.rootDir
  • tests.fetchgit.simple
  • tests.fetchgit.sparseCheckout
  • tests.fetchgit.sparseCheckoutNonConeMode
  • tests.fetchgit.submodule-deep
  • tests.fetchgit.submodule-leave-git
  • tests.fetchgit.submodule-leave-git-deep
  • tests.fetchgit.submodule-simple
  • tests.fetchgit.withGitConfig
  • tests.fetchtorrent.http-link (tests.fetchtorrent.http-link-transmission)
  • tests.fetchtorrent.magnet-link (tests.fetchtorrent.magnet-link-transmission)
  • tests.fetchurl.header
  • tests.testers.runCommand.bork

Copy link
Contributor

@MattSturgeon MattSturgeon left a comment

Choose a reason for hiding this comment

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

LGTM, other than treefmt failing

Co-authored-by: Matt Sturgeon <matt@sturgeon.me.uk>
@nixpkgs-ci nixpkgs-ci bot added 12.approvals: 2 This PR was reviewed and approved by two persons. and removed 12.approvals: 1 This PR was reviewed and approved by one person. labels Dec 8, 2025
@ShamrockLee ShamrockLee added this pull request to the merge queue Dec 8, 2025
Merged via the queue into NixOS:master with commit c81c85e Dec 8, 2025
26 of 28 checks passed
@ShamrockLee ShamrockLee deleted the fetchgit-leavedotgit branch December 8, 2025 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: fetch Fetchers (e.g. fetchgit, fetchsvn, ...) 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. 12.approvals: 2 This PR was reviewed and approved by two persons.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants