From 81e75e4bf6083046155a0c1fd6e312b43a60f7da Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 3 Feb 2023 21:42:42 +0100 Subject: [PATCH 1/7] Add some progress indication when fetching submodules --- src/libfetchers/git.cc | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index 1f7d7c07d..fb80d248c 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -615,15 +615,24 @@ struct GitInputScheme : InputScheme AutoDelete delTmpGitDir(tmpGitDir, true); runProgram("git", true, { "-c", "init.defaultBranch=" + gitInitialBranch, "init", tmpDir, "--separate-git-dir", tmpGitDir }); - // TODO: repoDir might lack the ref (it only checks if rev - // exists, see FIXME above) so use a big hammer and fetch - // everything to ensure we get the rev. - runProgram("git", true, { "-C", tmpDir, "fetch", "--quiet", "--force", - "--update-head-ok", "--", repoDir, "refs/*:refs/*" }); + + { + // TODO: repoDir might lack the ref (it only checks if rev + // exists, see FIXME above) so use a big hammer and fetch + // everything to ensure we get the rev. + Activity act(*logger, lvlTalkative, actUnknown, fmt("making temporary clone of '%s'", tmpDir)); + runProgram("git", true, { "-C", tmpDir, "fetch", "--quiet", "--force", + "--update-head-ok", "--", repoDir, "refs/*:refs/*" }); + } runProgram("git", true, { "-C", tmpDir, "checkout", "--quiet", input.getRev()->gitRev() }); + runProgram("git", true, { "-C", tmpDir, "remote", "add", "origin", actualUrl }); - runProgram("git", true, { "-C", tmpDir, "submodule", "--quiet", "update", "--init", "--recursive" }); + + { + Activity act(*logger, lvlTalkative, actUnknown, fmt("fetching submodules of '%s'", tmpDir)); + runProgram("git", true, { "-C", tmpDir, "submodule", "--quiet", "update", "--init", "--recursive" }); + } filter = isNotDotGitDirectory; } else { From 2edd5cf6184a7955ba82a2aed8bbfa870bfeb15f Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 7 Feb 2023 15:35:32 +0100 Subject: [PATCH 2/7] Fix the origin URL used for fetching submodules We cannot use 'actualUrl', because for file:// repos that's not the original URL that the repo was fetched from. This is a problem since submodules may be relative to the original URL. Fixes e.g. nix eval --impure --json --expr 'builtins.fetchTree { type = "git"; url = "/path/to/blender"; submodules = true; }' where /path/to/blender is a clone of https://github.com/blender/blender.git (which has several relative submodules like '../blender-addons.git'). --- src/libfetchers/git.cc | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index fb80d248c..8981476e1 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -627,7 +627,17 @@ struct GitInputScheme : InputScheme runProgram("git", true, { "-C", tmpDir, "checkout", "--quiet", input.getRev()->gitRev() }); - runProgram("git", true, { "-C", tmpDir, "remote", "add", "origin", actualUrl }); + /* Ensure that we use the correct origin for fetching + submodules. This matters for submodules with relative + URLs. */ + if (isLocal) { + writeFile(tmpGitDir + "/config", readFile(repoDir + "/" + gitDir + "/config")); + + /* Restore the config.bare setting we may have just + nuked. */ + runProgram("git", true, { "-C", tmpDir, "config", "core.bare", "false" }); + } else + runProgram("git", true, { "-C", tmpDir, "config", "remote.origin.url", actualUrl }); { Activity act(*logger, lvlTalkative, actUnknown, fmt("fetching submodules of '%s'", tmpDir)); From a8fe0dc16c18c260a1bb9e88f467e4c929cf22dc Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 7 Feb 2023 15:50:35 +0100 Subject: [PATCH 3/7] Speed up fetching submodules Previously we would completely refetch the submodules from the network, even though the repo might already have them. Now we copy the .git/modules directory from the repo as an optimisation. This speeds up evaluating builtins.fetchTree { type = "git"; url = "/path/to/blender"; submodules = true; } (where /path/to/blender already has the needed submodules) from 121s to 57s. This is still pretty inefficient and a hack, but a better solution is best done on the lazy-trees branch. This change also help in the case where the repo already has the submodules but the origin is unfetchable for whatever reason (e.g. there have been cases where Nix in a GitHub action doesn't have the right authentication set up). --- src/libfetchers/git.cc | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index 8981476e1..9908db214 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -639,6 +639,14 @@ struct GitInputScheme : InputScheme } else runProgram("git", true, { "-C", tmpDir, "config", "remote.origin.url", actualUrl }); + /* As an optimisation, copy the modules directory of the + source repo if it exists. */ + auto modulesPath = repoDir + "/" + gitDir + "/modules"; + if (pathExists(modulesPath)) { + Activity act(*logger, lvlTalkative, actUnknown, fmt("copying submodules of '%s'", actualUrl)); + runProgram("cp", true, { "-R", "--", modulesPath, tmpGitDir + "/modules" }); + } + { Activity act(*logger, lvlTalkative, actUnknown, fmt("fetching submodules of '%s'", tmpDir)); runProgram("git", true, { "-C", tmpDir, "submodule", "--quiet", "update", "--init", "--recursive" }); From 7a6daf61e8a3b6fd91e6d89334fe6b59fe07a2a6 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 7 Feb 2023 22:22:50 +0100 Subject: [PATCH 4/7] Fix activity message --- src/libfetchers/git.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index 9908db214..3871cf0dd 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -620,7 +620,7 @@ struct GitInputScheme : InputScheme // TODO: repoDir might lack the ref (it only checks if rev // exists, see FIXME above) so use a big hammer and fetch // everything to ensure we get the rev. - Activity act(*logger, lvlTalkative, actUnknown, fmt("making temporary clone of '%s'", tmpDir)); + Activity act(*logger, lvlTalkative, actUnknown, fmt("making temporary clone of '%s'", repoDir)); runProgram("git", true, { "-C", tmpDir, "fetch", "--quiet", "--force", "--update-head-ok", "--", repoDir, "refs/*:refs/*" }); } @@ -648,7 +648,7 @@ struct GitInputScheme : InputScheme } { - Activity act(*logger, lvlTalkative, actUnknown, fmt("fetching submodules of '%s'", tmpDir)); + Activity act(*logger, lvlTalkative, actUnknown, fmt("fetching submodules of '%s'", repoDir)); runProgram("git", true, { "-C", tmpDir, "submodule", "--quiet", "update", "--init", "--recursive" }); } From 9a7dc5d71879a32ff06305aa843abf299b39c90f Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 7 Feb 2023 22:46:25 +0100 Subject: [PATCH 5/7] Add some tests --- tests/fetchGitSubmodules.sh | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tests/fetchGitSubmodules.sh b/tests/fetchGitSubmodules.sh index 50da4cb97..08ccaa3cd 100644 --- a/tests/fetchGitSubmodules.sh +++ b/tests/fetchGitSubmodules.sh @@ -104,3 +104,28 @@ noSubmoduleRepoBaseline=$(nix eval --raw --expr "(builtins.fetchGit { url = file noSubmoduleRepo=$(nix eval --raw --expr "(builtins.fetchGit { url = file://$subRepo; rev = \"$subRev\"; submodules = true; }).outPath") [[ $noSubmoduleRepoBaseline == $noSubmoduleRepo ]] + +# Test relative submodule URLs. +rm $TEST_HOME/.cache/nix/fetcher-cache* +rm -rf $rootRepo/.git $rootRepo/.gitmodules $rootRepo/sub +initGitRepo $rootRepo +git -C $rootRepo submodule add ../gitSubmodulesSub sub +git -C $rootRepo commit -m "Add submodule" +rev2=$(git -C $rootRepo rev-parse HEAD) +pathWithRelative=$(nix eval --raw --expr "(builtins.fetchGit { url = file://$rootRepo; rev = \"$rev2\"; submodules = true; }).outPath") +diff -r -x .gitmodules $pathWithSubmodules $pathWithRelative + +# Test clones that have an upstream with relative submodule URLs. +rm $TEST_HOME/.cache/nix/fetcher-cache* +cloneRepo=$TEST_ROOT/a/b/gitSubmodulesClone # NB /a/b to make the relative path not work relative to $cloneRepo +git clone $rootRepo $cloneRepo +pathIndirect=$(nix eval --raw --expr "(builtins.fetchGit { url = file://$cloneRepo; rev = \"$rev2\"; submodules = true; }).outPath") +[[ $pathIndirect = $pathWithRelative ]] + +# Test that if the clone has the submodule already, we're not fetching +# it again. +git -C $cloneRepo submodule update --init +rm $TEST_HOME/.cache/nix/fetcher-cache* +rm -rf $subRepo +pathSubmoduleGone=$(nix eval --raw --expr "(builtins.fetchGit { url = file://$cloneRepo; rev = \"$rev2\"; submodules = true; }).outPath") +[[ $pathSubmoduleGone = $pathWithRelative ]] From 15313bfdb7139d0fef53a5845dfdd7be90bd68c8 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 9 Feb 2023 16:42:14 +0100 Subject: [PATCH 6/7] Fix activity message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Josef Kemetmüller --- src/libfetchers/git.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index 3871cf0dd..1fafc7df8 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -648,7 +648,7 @@ struct GitInputScheme : InputScheme } { - Activity act(*logger, lvlTalkative, actUnknown, fmt("fetching submodules of '%s'", repoDir)); + Activity act(*logger, lvlTalkative, actUnknown, fmt("fetching submodules of '%s'", actualUrl)); runProgram("git", true, { "-C", tmpDir, "submodule", "--quiet", "update", "--init", "--recursive" }); } From 862e56c23d9ee0dafc1902f27b20f58ccf421313 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 9 Feb 2023 16:42:45 +0100 Subject: [PATCH 7/7] Improve comment Co-authored-by: Robert Hensing --- src/libfetchers/git.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index 1fafc7df8..309a143f5 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -634,7 +634,7 @@ struct GitInputScheme : InputScheme writeFile(tmpGitDir + "/config", readFile(repoDir + "/" + gitDir + "/config")); /* Restore the config.bare setting we may have just - nuked. */ + copied erroneously from the user's repo. */ runProgram("git", true, { "-C", tmpDir, "config", "core.bare", "false" }); } else runProgram("git", true, { "-C", tmpDir, "config", "remote.origin.url", actualUrl });