fix(forge): make recursive forge update optional via --recursive flag (#5980)

* don't default to recursive submodule updates

* update and remove ignore from recursive submodule update test

* chore: update test to use new repo location

---------

Co-authored-by: James Wenzel <[email protected]>
Co-authored-by: Enrique Ortiz <[email protected]>

Author: 3 people

crates/cli/src/utils/mod.rs

@@ -546,6 +546,7 @@ https://github.com/foundry-rs/foundry/issues/new/choose"
         force: bool,
         remote: bool,
         no_fetch: bool,
+        recursive: bool,
         paths: I,
     ) -> Result<()>
     where
@@ -554,16 +555,27 @@ https://github.com/foundry-rs/foundry/issues/new/choose"
     {
         self.cmd()
             .stderr(self.stderr())
-            .args(["submodule", "update", "--progress", "--init", "--recursive"])
+            .args(["submodule", "update", "--progress", "--init"])
             .args(self.shallow.then_some("--depth=1"))
             .args(force.then_some("--force"))
             .args(remote.then_some("--remote"))
             .args(no_fetch.then_some("--no-fetch"))
+            .args(recursive.then_some("--recursive"))
             .args(paths)
             .exec()
             .map(drop)
     }
 
+    pub fn submodule_foreach(self, recursive: bool, cmd: impl AsRef<OsStr>) -> Result<()> {
+        self.cmd()
+            .stderr(self.stderr())
+            .args(["submodule", "foreach"])
+            .args(recursive.then_some("--recursive"))
+            .arg(cmd)
+            .exec()
+            .map(drop)
+    }
+
     pub fn submodule_init(self) -> Result<()> {
         self.cmd().stderr(self.stderr()).args(["submodule", "init"]).exec().map(drop)
     }

crates/forge/bin/cmd/init.rs

@@ -84,7 +84,7 @@ impl InitArgs {
                 git.submodule_init()?;
             } else {
                 // if not shallow, initialize and clone submodules (without fetching latest)
-                git.submodule_update(false, false, true, None::<PathBuf>)?;
+                git.submodule_update(false, false, true, true, None::<PathBuf>)?;
             }
         } else {
             // if target is not empty

crates/forge/bin/cmd/install.rs

@@ -126,7 +126,8 @@ impl DependencyInstallOpts {
 
         if dependencies.is_empty() && !self.no_git {
             p_println!(!self.quiet => "Updating dependencies in {}", libs.display());
-            git.submodule_update(false, false, false, Some(&libs))?;
+            // recursively fetch all submodules (without fetching latest)
+            git.submodule_update(false, false, false, true, Some(&libs))?;
         }
         fs::create_dir_all(&libs)?;
 
@@ -303,8 +304,8 @@ impl Installer<'_> {
         trace!(?dep, url, ?path, "installing git submodule");
         self.git.submodule_add(true, url, path)?;
 
-        trace!("updating submodule recursively");
-        self.git.submodule_update(false, false, false, Some(path))
+        trace!("initializing submodule recursively");
+        self.git.submodule_update(false, false, false, true, Some(path))
     }
 
     fn git_checkout(self, dep: &Dependency, path: &Path, recurse: bool) -> Result<String> {

crates/forge/bin/cmd/update.rs

@@ -23,14 +23,29 @@ pub struct UpdateArgs {
     /// Override the up-to-date check.
     #[clap(short, long)]
     force: bool,
+
+    /// Recursively update submodules.
+    #[clap(short, long)]
+    recursive: bool,
 }
 impl_figment_convert_basic!(UpdateArgs);
 
 impl UpdateArgs {
     pub fn run(self) -> Result<()> {
         let config = self.try_load_config_emit_warnings()?;
         let (root, paths) = dependencies_paths(&self.dependencies, &config)?;
-        Git::new(&root).submodule_update(self.force, true, false, paths)
+        // fetch the latest changes for each submodule (recursively if flag is set)
+        let git = Git::new(&root);
+        if self.recursive {
+            // update submodules recursively
+            git.submodule_update(self.force, true, false, true, paths)
+        } else {
+            // update root submodules
+            git.submodule_update(self.force, true, false, false, paths)?;
+            // initialize submodules of each submodule recursively (otherwise direct submodule
+            // dependencies will revert to last commit)
+            git.submodule_foreach(false, "git submodule update --init --progress --recursive ")
+        }
     }
 }
 

crates/forge/tests/cli/cmd.rs

@@ -1009,7 +1009,6 @@ forgetest!(can_install_latest_release_tag, |prj: TestProject, mut cmd: TestComma
 // Tests that forge update doesn't break a working dependency by recursively updating nested
 // dependencies
 forgetest!(
-    #[ignore]
     can_update_library_with_outdated_nested_dependency,
     |prj: TestProject, mut cmd: TestCommand| {
         cmd.git_init();
@@ -1018,39 +1017,63 @@ forgetest!(
         let git_mod = prj.root().join(".git/modules/lib");
         let git_mod_file = prj.root().join(".gitmodules");
 
-        let package = libs.join("issue-2264-repro");
-        let package_mod = git_mod.join("issue-2264-repro");
+        // get paths to check inside install fn
+        let package = libs.join("forge-5980-test");
+        let package_mod = git_mod.join("forge-5980-test");
 
         let install = |cmd: &mut TestCommand| {
-            cmd.forge_fuse().args(["install", "foundry-rs/issue-2264-repro", "--no-commit"]);
+            // install main dependency
+            cmd.forge_fuse().args(["install", "evalir/forge-5980-test", "--no-commit"]);
             cmd.assert_non_empty_stdout();
+
+            // assert pathbufs exist
             assert!(package.exists());
             assert!(package_mod.exists());
 
             let submods = read_string(&git_mod_file);
-            assert!(submods.contains("https://github.com/foundry-rs/issue-2264-repro"));
+            assert!(submods.contains("https://github.com/evalir/forge-5980-test"));
         };
 
         install(&mut cmd);
-        cmd.forge_fuse().args(["update", "lib/issue-2264-repro"]);
+        // try to update the top-level dependency; there should be no update for this dependency,
+        // but its sub-dependency has upstream (breaking) changes; forge should not attempt to
+        // update the sub-dependency
+        cmd.forge_fuse().args(["update", "lib/forge-5980-test"]);
         cmd.stdout_lossy();
 
+        // add explicit remappings for test file
+        let config = Config {
+            remappings: vec![
+                Remapping::from_str("forge-5980-test/=lib/forge-5980-test/src/").unwrap().into(),
+                // explicit remapping for sub-dependendy seems necessary for some reason
+                Remapping::from_str(
+                    "forge-5980-test-dep/=lib/forge-5980-test/lib/forge-5980-test-dep/src/",
+                )
+                .unwrap()
+                .into(),
+            ],
+            ..Default::default()
+        };
+        prj.write_config(config);
+
+        // create test file that uses the top-level dependency; if the sub-dependency is updated,
+        // compilation will fail
         prj.inner()
             .add_source(
-                "MyTokenCopy",
+                "CounterCopy",
                 r#"
 // SPDX-License-Identifier: UNLICENSED
-pragma solidity ^0.6.0;
-import "issue-2264-repro/MyToken.sol";
-contract MyTokenCopy is MyToken {
+pragma solidity ^0.8.0;
+import "forge-5980-test/Counter.sol";
+contract CounterCopy is Counter {
 }
    "#,
             )
             .unwrap();
 
-        cmd.forge_fuse().args(["build"]);
+        // build and check output
+        cmd.forge_fuse().arg("build");
         let output = cmd.stdout_lossy();
-
         assert!(output.contains("Compiler run successful",));
     }
 );