Skip to content

Commit 4f63e9e

Browse files
committed
VCS: Don't run submodule commands unless necessary
Running `git submodule` commands is harmless but clutters up the logs, making the tests difficult to debug when run in verbose-mode. Doesn't seem to impact performance much. I measured a ~1.5% speedup with this code, which is well within error margins. See: https://github.com/haskell/cabal/pull/7625/files#r709617991
1 parent 75cac83 commit 4f63e9e

File tree

3 files changed

+47
-24
lines changed
  • Cabal/src/Distribution/Simple/Program
  • cabal-install
    • src/Distribution/Client
    • tests/UnitTests/Distribution/Client

3 files changed

+47
-24
lines changed

Cabal/src/Distribution/Simple/Program/Run.hs

+2
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@ data ProgramInvocation = ProgramInvocation
6161
, progInvokeInputEncoding :: IOEncoding
6262
-- ^ TODO: remove this, make user decide when constructing 'progInvokeInput'.
6363
, progInvokeOutputEncoding :: IOEncoding
64+
, progInvokeWhen :: IO Bool
6465
}
6566

6667
data IOEncoding
@@ -82,6 +83,7 @@ emptyProgramInvocation =
8283
, progInvokeInput = Nothing
8384
, progInvokeInputEncoding = IOEncodingText
8485
, progInvokeOutputEncoding = IOEncodingText
86+
, progInvokeWhen = pure True
8587
}
8688

8789
simpleProgramInvocation

cabal-install/src/Distribution/Client/VCS.hs

+30-13
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,7 @@ import qualified Data.List as List
9696
import qualified Data.Map as Map
9797
import System.Directory
9898
( doesDirectoryExist
99+
, doesFileExist
99100
, removeDirectoryRecursive
100101
, removePathForcibly
101102
)
@@ -468,11 +469,18 @@ vcsGit =
468469
[programInvocation prog cloneArgs]
469470
-- And if there's a tag, we have to do that in a second step:
470471
++ [git (resetArgs tag) | tag <- maybeToList (srpTag repo)]
471-
++ [ git (["submodule", "sync", "--recursive"] ++ verboseArg)
472-
, git (["submodule", "update", "--init", "--force", "--recursive"] ++ verboseArg)
472+
++ [ whenGitModulesExists $ git $ ["submodule", "sync", "--recursive"] ++ verboseArg
473+
, whenGitModulesExists $ git $ ["submodule", "update", "--init", "--force", "--recursive"] ++ verboseArg
473474
]
474475
where
475476
git args = (programInvocation prog args){progInvokeCwd = Just destdir}
477+
478+
gitModulesPath = destdir </> ".gitmodules"
479+
whenGitModulesExists invocation =
480+
invocation
481+
{ progInvokeWhen = doesFileExist gitModulesPath
482+
}
483+
476484
cloneArgs =
477485
["clone", srcuri, destdir]
478486
++ branchArgs
@@ -516,29 +524,38 @@ vcsGit =
516524
-- is needed because sometimes `git submodule sync` does not actually
517525
-- update the submodule source URL. Detailed description here:
518526
-- https://git.coop/-/snippets/85
519-
git localDir ["submodule", "deinit", "--force", "--all"]
520-
let gitModulesDir = localDir </> ".git" </> "modules"
521-
gitModulesExists <- doesDirectoryExist gitModulesDir
522-
when gitModulesExists $
527+
let dotGitModulesPath = localDir </> ".git" </> "modules"
528+
gitModulesPath = localDir </> ".gitmodules"
529+
530+
-- Remove any `.git/modules` if they exist.
531+
dotGitModulesExists <- doesDirectoryExist dotGitModulesPath
532+
when dotGitModulesExists $ do
533+
git localDir $ ["submodule", "deinit", "--force", "--all"] ++ verboseArg
523534
if buildOS == Windows
524535
then do
525536
-- Windows can't delete some git files #10182
526537
void $
527538
Process.createProcess_ "attrib" $
528539
Process.shell $
529-
"attrib -s -h -r " <> gitModulesDir <> "\\*.* /s /d"
540+
"attrib -s -h -r " <> dotGitModulesPath <> "\\*.* /s /d"
530541

531542
catch
532-
(removePathForcibly gitModulesDir)
533-
(\e -> if isPermissionError e then removePathForcibly gitModulesDir else throw e)
534-
else removeDirectoryRecursive gitModulesDir
543+
(removePathForcibly dotGitModulesPath)
544+
(\e -> if isPermissionError e then removePathForcibly dotGitModulesPath else throw e)
545+
else removeDirectoryRecursive dotGitModulesPath
546+
535547
when (resetTarget /= "HEAD") $ do
536548
git localDir fetchArgs -- first fetch the tag if needed
537549
git localDir setTagArgs
538550
git localDir resetArgs -- only then reset to the commit
539-
git localDir $ ["submodule", "sync", "--recursive"] ++ verboseArg
540-
git localDir $ ["submodule", "update", "--force", "--init", "--recursive"] ++ verboseArg
541-
git localDir $ ["submodule", "foreach", "--recursive"] ++ verboseArg ++ ["git clean -ffxdq"]
551+
552+
-- We need to check if `.gitmodules` exists _after_ the `git reset` call.
553+
gitModulesExists <- doesFileExist gitModulesPath
554+
when gitModulesExists $ do
555+
git localDir $ ["submodule", "sync", "--recursive"] ++ verboseArg
556+
git localDir $ ["submodule", "update", "--force", "--init", "--recursive"] ++ verboseArg
557+
git localDir $ ["submodule", "foreach", "--recursive"] ++ verboseArg ++ ["git clean -ffxdq"]
558+
542559
git localDir $ ["clean", "-ffxdq"]
543560
where
544561
git :: FilePath -> [String] -> IO ()

cabal-install/tests/UnitTests/Distribution/Client/VCS.hs

+15-11
Original file line numberDiff line numberDiff line change
@@ -874,10 +874,7 @@ vcsTestDriverGit verbosity vcs submoduleDir repoRoot =
874874
, vcsSubmoduleDriver =
875875
pure . vcsTestDriverGit verbosity vcs' submoduleDir . (submoduleDir </>)
876876
, vcsAddSubmodule = \_ source dest -> do
877-
destExists <-
878-
(||)
879-
<$> doesFileExist (repoRoot </> dest)
880-
<*> doesDirectoryExist (repoRoot </> dest)
877+
destExists <- doesPathExist $ repoRoot </> dest
881878
when destExists $ git ["rm", "-f", dest]
882879
-- If there is an old submodule git dir with the same name, remove it.
883880
-- It most likely has a different URL and `git submodule add` will fai.
@@ -923,15 +920,22 @@ vcsTestDriverGit verbosity vcs submoduleDir repoRoot =
923920
git' = getProgramInvocationOutput verbosity . gitInvocation
924921
verboseArg = ["--quiet" | verbosity < Verbosity.normal]
925922
submoduleGitDir path = repoRoot </> ".git" </> "modules" </> path
923+
924+
dotGitModulesPath = repoRoot </> ".git" </> "modules"
925+
gitModulesPath = repoRoot </> ".gitmodules"
926+
926927
deinitAndRemoveCachedSubmodules = do
927-
git $ ["submodule", "deinit", "--force", "--all"] ++ verboseArg
928-
let gitModulesDir = repoRoot </> ".git" </> "modules"
929-
gitModulesExists <- doesDirectoryExist gitModulesDir
930-
when gitModulesExists $ removeDirectoryRecursive gitModulesDir
928+
dotGitModulesExists <- doesDirectoryExist dotGitModulesPath
929+
when dotGitModulesExists $ do
930+
git $ ["submodule", "deinit", "--force", "--all"] ++ verboseArg
931+
removeDirectoryRecursive dotGitModulesPath
932+
931933
updateSubmodulesAndCleanup = do
932-
git $ ["submodule", "sync", "--recursive"] ++ verboseArg
933-
git $ ["submodule", "update", "--init", "--recursive", "--force"] ++ verboseArg
934-
git $ ["submodule", "foreach", "--recursive"] ++ verboseArg ++ ["git clean -ffxdq"]
934+
gitModulesExists <- doesFileExist gitModulesPath
935+
when gitModulesExists $ do
936+
git $ ["submodule", "sync", "--recursive"] ++ verboseArg
937+
git $ ["submodule", "update", "--init", "--recursive", "--force"] ++ verboseArg
938+
git $ ["submodule", "foreach", "--recursive", "git clean -ffxdq"] ++ verboseArg
935939
git $ ["clean", "-ffxdq"] ++ verboseArg
936940

937941
type MTimeChange = Int

0 commit comments

Comments
 (0)