I've added documentation and a test.

@edolstra WDYT? this would be super helpful!

@edolstra @tfc I've resolved merge conflicts.

@edolstra It would be really nice to find a solution here to get submodule support working with builtins.fetchGit, because I'm currently building workarounds again. :)

Here is a patch with my proposed changes:

diff --git a/src/libexpr/local.mk b/src/libexpr/local.mk
index 26b9f14ba..e8c019365 100644
--- a/src/libexpr/local.mk
+++ b/src/libexpr/local.mk
@@ -8,7 +8,7 @@ libexpr_SOURCES := $(wildcard $(d)/*.cc) $(wildcard $(d)/primops/*.cc) $(d)/lexe
 
 libexpr_LIBS = libutil libstore libnixrust
 
-libexpr_LDFLAGS =
+libexpr_LDFLAGS = -lstdc++fs
 ifneq ($(OS), FreeBSD)
  libexpr_LDFLAGS += -ldl
 endif
diff --git a/src/libexpr/primops/fetchGit.cc b/src/libexpr/primops/fetchGit.cc
index b08e48404..c58d28bbc 100644
--- a/src/libexpr/primops/fetchGit.cc
+++ b/src/libexpr/primops/fetchGit.cc
@@ -6,6 +6,7 @@
 #include "hash.hh"
 #include "tarfile.hh"
 
+#include <filesystem>
 #include <sys/time.h>
 
 #include <regex>
@@ -182,9 +183,13 @@ GitInfo exportGit(ref<Store> store, const std::string & uri,
 
         runProgram("git", true, { "-C", tmpDir, "checkout", "--quiet", "FETCH_HEAD" });
         runProgram("git", true, { "-C", tmpDir, "remote", "add", "origin", uri });
-        runProgram("git", true, { "-C", tmpDir, "submodule", "--quiet", "update", "--init" });
+        runProgram("git", true, { "-C", tmpDir, "submodule", "--quiet", "update", "--init", "--recursive" });
 
-        deletePath(tmpDir + "/.git");
+        for (const auto& p : std::filesystem::recursive_directory_iterator(tmpDir)) {
+            if (p.path().filename() == ".git") {
+                std::filesystem::remove_all(p.path());
+            }
+        }
 
         gitInfo.submodules = true;
     } else {
diff --git a/tests/fetchGitSubmodules.sh b/tests/fetchGitSubmodules.sh
index c821d2caa..987bfd69e 100644
--- a/tests/fetchGitSubmodules.sh
+++ b/tests/fetchGitSubmodules.sh
@@ -47,4 +47,5 @@ pathWithSubmodules=$(nix eval --raw "(builtins.fetchGit { url = file://$rootRepo
 [[ ! -e $pathWithoutSubmodules/sub/content ]]
 [[ -e $pathWithSubmodules/sub/content ]]
 
-
+# No .git directory or submodule reference files must be left
+test "$(find "$pathWithSubmodules" -name .git)" = ""

I used std::filesystem for removing the .git paths. Nix already depends on C++17, but apparently one needs -lstdc++fs to get this library. The internet says clang needs a different flag, but the current flag worked in my testing (./shell.nix has a useClang flag which I enabled). So perhaps I was reading outdated info.

By the way, I had a chat with @edolstra . He's refactoring the fetcher, which will need to land before this. Sharing this to clarify, and ideally to save people some time in case some of the changes don't apply anymore!

@bjornfor Thanks for the review and fixes! As @nmattia pointed out this will likely conflict with upcoming changes, so I'd rather wait before spending more time here. Or do you think there is a chance of sneaking this in before the refactoring?

@nmattia @edolstra It would be nice if there is some issue to watch regarding the fetcher changes.

I'm eagerly awaiting a status update here :-)

I did some real life testing with this patch and found two issues.

1. The result of builtins.fetchGit { fetchSubmodules = true; ... } changes from the first run to the next. The first run results in { outPath = ; ...; submodules = true; }, but the next calls result in { outPath = ; ...; submodules = false; }. I think the output submodules attribute would follow the input fetchSubmodules parameter exactly and not change on the 2nd (and following) runs.

2. Using fetchSubmodules = true on repos that have no submodules is a fatal error:

nix --experimental-features nix-command eval '(builtins.fetchGit { url = "https://github.com/nixos/nix.git"; ref = "refs/heads/master"; fetchSubmodules = true; } )'
fatal: Refusing to fetch into current branch refs/heads/master of non-bare repository
fatal: The remote end hung up unexpectedly
error: program 'git' failed with exit code 128

Allowing fetchSubmodules = true on non-submodule using repos is a requirement for my use case, and is supported by git itself: git clone --recursive https://github.com/nixos/nix.git is no error even if nix.git uses no submodules.

@edolstra: Are you refactoring the fetcher(s)? I would really like for nix to have git submodule support and can probably spend a bit more time on this if there is interest. But no sign of interest kills my motivation.

Yes, the new fetcher infrastructure is in the flakes branch: https://github.com/NixOS/nix/tree/flakes/src/libstore/fetchers. It replaces fetchGit / fetchMercurial with a generic fetchTree builtin that uses the same attributes as flake inputs (e.g. fetchTree { type = "github"; owner = "NixOS"; repo = "nix"; rev = ...; }). So submodules would be an attribute supported by the git fetcher. And then flakes would automatically support Git repos with submodules as an input.

@edolstra @tfc @bjornfor I was wondering how to implement submodule support while keeping all the nice caching in place. I'm thinking about something along the lines of:

1. Fetch a git repo
2. Export it into the Nix store
3. If submodule fetching is enabled:

• enumerate all submodules, their URLs and commit hashes,
• recursively use the normal git fetching logic to fetch them into the Nix store
• join all submodules together using hard/symlinks

This would keep the normal caching of git repositories in place and submodules wouldn't be treated any differently from normal git checkouts.

The downside is that the implementation is somewhat complex compared to just using git clone --recursive and we are partially reimplementing the git submodule command.

To limit the complexity in Nix itself, most of this could be implemented as Nix expression, given that a builtin is available that returns a list of submodules and their properties.

Opinions?

I think we should ignore the caching issue for now. I still want to add a generic caching mechanism to the fetcher infrastructure that generalizes the Git-specific caching (in ~/.cache/nix/git-revs). So Nix would keep a mapping fetcher input attributes -> store path which would also work for git-with-submodules.

Yes, the new fetcher infrastructure is in the flakes branch: https://github.com/NixOS/nix/tree/flakes/src/libstore/fetchers.

@blitz @edolstra: I'm confused about what the next action is. Ok, so flakes branch has new fetcher infrastructure, and this is where the future git fetcher with submodule support will live. But flakes is an experimental branch and it would be nice to have submodules support in standard Nix. (I don't know how long until flakes get stabilized and land in master.)

So, if this PR is refreshed (fixing the outstanding issues as commented in this thread), would it be merged?

@bjornfor I'll post an updated version soon. If I should rebuild this on top of flakes, this would be good to know.

@bjornfor Yes, I'll take care of porting it to the new fetcher infrastructure.

I've incorporated all fixes. I just have to investigate the test failures.

@edolstra @bjornfor @nmattia @tfc I'd appreciate a second look here. I think I have added tests for all issues that were pointed out and these tests are passing now.

Two tests are failing (tests/multiple-outputs.sh and `tests/nix-build.sh), but these are not regressions and are already failing on the master branch.

@blitz: Did you build it? I'm getting this error:

/nix/store/m8ih8gw7qnd2c7p0qgv3cn4b6abkbanb-binutils-2.31.1/bin/ld: /build/nix-2.4pre7279_7fee022/src/libexpr/primops/fetchGit.cc:188: undefined reference to `std::filesystem::__cxx11::recursive_directory_iterator::operator++()'

In my patch I added libexpr_LDFLAGS = -lstdc++fs, but I don't see that in your PR.

@bjornfor

@blitz: Did you build it? I'm getting this error:

/nix/store/m8ih8gw7qnd2c7p0qgv3cn4b6abkbanb-binutils-2.31.1/bin/ld: /build/nix-2.4pre7279_7fee022/src/libexpr/primops/fetchGit.cc:188: undefined reference to `std::filesystem::__cxx11::recursive_directory_iterator::operator++()'

In my patch I added libexpr_LDFLAGS = -lstdc++fs, but I don't see that in your PR.

My understanding was that -lstdc++fs is only required for the experimental filesystem support and is not required anymore since the filesystem support moved out of its experimental state. That may be wrong for some platforms. I'm using:

% nix-build release.nix -A build.x86_64-linux

@edolstra @bjornfor

I've removed std::filesystem use in favor of using the filter support in addToStore. This seemed simpler than using readDirectory and deletePath.

@edolstra @bjornfor anything else that needs to be done? i am very interested in seeing this patch merged.

Merged, thanks!

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

https://discourse.nixos.org/t/why-doesnt-fetchfromgithub-support-submodules-with-private/6600/6

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

https://discourse.nixos.org/t/fetchgit-and-fetchgit-are-not-working-for-private-repos-with-submodules/6673/2

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

https://discourse.nixos.org/t/nix-flakes-and-submodules/7904/2

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

https://discourse.nixos.org/t/using-crate2nix-to-create-a-cargo-alternative-or-wrapper-that-transparently-uses-nix-store-as-a-cache/12095/5