fix: update branch rev in lockfile and print updates

crates/forge/bin/cmd/install.rs

@@ -175,7 +175,9 @@ impl DependencyInstallOpts {
                 if let Some(tag_or_branch) = &installed_tag {
                     // First, check if this tag has a branch
                     dep_id = Some(DepIdentifier::resolve_type(&git, &path, tag_or_branch)?);
-                    if git.has_branch(tag_or_branch, &path)? {
+                    if git.has_branch(tag_or_branch, &path)? &&
+                        dep_id.as_ref().is_some_and(|id| id.is_branch())
+                    {
                         // always work with relative paths when directly modifying submodules
                         git.cmd()
                             .args(["submodule", "set-branch", "-b", tag_or_branch])

crates/forge/bin/cmd/update.rs

@@ -1,13 +1,14 @@
 use alloy_primitives::map::HashMap;
 use clap::{Parser, ValueHint};
 use eyre::{Context, Result};
-use forge::{DepIdentifier, Lockfile};
+use forge::{DepIdentifier, DepMap, Lockfile};
 use foundry_cli::{
     opts::Dependency,
     utils::{Git, LoadConfig},
 };
 use foundry_config::{impl_figment_convert_basic, Config};
 use std::path::PathBuf;
+use yansi::Paint;
 
 /// CLI arguments for `forge update`.
 #[derive(Clone, Debug, Parser)]
@@ -46,7 +47,7 @@ impl UpdateArgs {
         let prev_len = foundry_lock.len();
 
         // update the submodules' tags if any overrides are present
-
+        let mut prev_dep_ids: DepMap = HashMap::default();
         if dep_overrides.is_empty() {
             // running `forge update`, update all deps
             foundry_lock.iter_mut().for_each(|(_path, dep_id)| {
@@ -61,7 +62,8 @@ impl UpdateArgs {
                     .strip_prefix(&root)
                     .wrap_err("Dependency path is not relative to the repository root")?;
                 if let Ok(dep_id) = DepIdentifier::resolve_type(&git, dep_path, override_tag) {
-                    foundry_lock.override_dep(rel_path, dep_id)?
+                    let prev = foundry_lock.override_dep(rel_path, dep_id)?;
+                    prev_dep_ids.insert(rel_path.to_owned(), prev);
                 } else {
                     sh_warn!(
                         "Could not r#override submodule at {} with tag {}, try using forge install",
@@ -91,6 +93,52 @@ impl UpdateArgs {
             }
         }
 
+        // FIX: Branches should get updated to their latest commit on `forge update`.
+        // i.e if previously submodule was tracking branch `main` at rev `1234567` and now the
+        // remote `main` branch is at `7654321`, then submodule should also be updated to `7654321`.
+        // This tracking is automatically handled by git, but we need to update the lockfile entry
+        // to reflect the latest commit.
+        if dep_overrides.is_empty() {
+            let branch_overrides = foundry_lock
+                .iter_mut()
+                .filter_map(|(path, dep_id)| {
+                    if dep_id.is_branch() && dep_id.overridden() {
+                        return Some((path, dep_id))
+                    }
+                    None
+                })
+                .collect::<Vec<_>>();
+
+            for (path, dep_id) in branch_overrides {
+                let (curr_rev, curr_branch) = git.current_rev_branch(&root.join(path))?;
+                let name = dep_id.name();
+                // This can occur when the submodule is manually checked out to a different branch.
+                if curr_branch != name {
+                    let warn_msg = format!(
+                        r#"Lockfile sync warning
+                        Lockfile is tracking branch {name} for submodule at {path:?}, but the submodule is currently on {curr_branch}.
+                        Checking out branch {name} for submodule at {path:?}."#,
+                    );
+                    let _ = sh_warn!("{}", warn_msg);
+                    git.checkout_at(name, &root.join(path)).wrap_err(format!(
+                        "Could not checkout branch {name} for submodule at {}",
+                        path.display()
+                    ))?;
+                }
+
+                // Update the lockfile entry to reflect the latest commit
+                let prev = std::mem::replace(
+                    dep_id,
+                    DepIdentifier::Branch {
+                        name: name.to_string(),
+                        rev: curr_rev,
+                        r#override: true,
+                    },
+                );
+                prev_dep_ids.insert(path.to_owned(), prev);
+            }
+        }
+
         // checkout the submodules at the correct tags
         for (path, dep_id) in foundry_lock.iter() {
             git.checkout_at(dep_id.checkout_id(), &root.join(path))?;
@@ -102,6 +150,17 @@ impl UpdateArgs {
             foundry_lock.write()?;
         }
 
+        // Print updates from => to
+        for (path, prev) in prev_dep_ids {
+            let curr = foundry_lock.get(&path).unwrap();
+            sh_println!(
+                "Updated dep at '{}', (from: {prev}, to: {curr})",
+                path.display().green(),
+                prev = prev,
+                curr = curr.yellow()
+            )?;
+        }
+
         Ok(())
     }
 
@@ -129,6 +188,10 @@ pub fn dependencies_paths(
     let git_root = Git::root_of(&config.root)?;
     let libs = config.install_lib_dir();
 
+    if deps.is_empty() {
+        return Ok((git_root, Vec::new(), HashMap::default()));
+    }
+
     let mut paths = Vec::with_capacity(deps.len());
     let mut overrides = HashMap::with_capacity_and_hasher(deps.len(), Default::default());
     for dep in deps {

crates/forge/src/lockfile.rs

@@ -172,19 +172,25 @@ impl<'a> Lockfile<'a> {
 
     /// Override a dependency in the lockfile.
     ///
+    /// Returns the overridden/previous [`DepIdentifier`].
     /// This is used in `forge update` to decide whether a dep's tag/branch/rev should be updated.
     ///
     /// Throws an error if the dependency is not found in the lockfile.
-    pub fn override_dep(&mut self, dep: &Path, mut new_dep_id: DepIdentifier) -> Result<()> {
-        self.deps
+    pub fn override_dep(
+        &mut self,
+        dep: &Path,
+        mut new_dep_id: DepIdentifier,
+    ) -> Result<DepIdentifier> {
+        let prev = self
+            .deps
             .get_mut(dep)
             .map(|d| {
                 new_dep_id.mark_overide();
-                *d = new_dep_id;
+                std::mem::replace(d, new_dep_id)
             })
             .ok_or_eyre(format!("Dependency not found in lockfile: {}", dep.display()))?;
 
-        Ok(())
+        Ok(prev)
     }
 
     /// Returns the num of dependencies in the lockfile.
@@ -280,6 +286,17 @@ impl DepIdentifier {
         }
     }
 
+    /// Get the name of the dependency.
+    ///
+    /// In case of a Rev, this will return the commit hash.
+    pub fn name(&self) -> &str {
+        match self {
+            Self::Branch { name, .. } => name,
+            Self::Tag { name, .. } => name,
+            Self::Rev { rev, .. } => rev,
+        }
+    }
+
     /// Get the name/rev to checkout at.
     pub fn checkout_id(&self) -> &str {
         match self {
@@ -289,7 +306,7 @@ impl DepIdentifier {
         }
     }
 
-    /// Marks as dependency as overridden.
+    /// Marks as dependency as overriden.
     pub fn mark_overide(&mut self) {
         match self {
             Self::Branch { r#override, .. } => *r#override = true,
@@ -298,14 +315,19 @@ impl DepIdentifier {
         }
     }
 
-    /// Returns whether the dependency has been overridden.
+    /// Returns whether the dependency has been overriden.
     pub fn overridden(&self) -> bool {
         match self {
             Self::Branch { r#override, .. } => *r#override,
             Self::Tag { r#override, .. } => *r#override,
             Self::Rev { r#override, .. } => *r#override,
         }
     }
+
+    /// Returns whether the dependency is a branch.
+    pub fn is_branch(&self) -> bool {
+        matches!(self, Self::Branch { .. })
+    }
 }
 
 impl std::fmt::Display for DepIdentifier {

crates/forge/tests/cli/install.rs

@@ -190,7 +190,7 @@ forgetest!(can_install_latest_release_tag, |prj, cmd| {
     assert!(current >= version);
 });
 
-forgetest!(can_update_and_retain_tag_revs, |_prj, cmd| {
+forgetest!(can_update_and_retain_tag_revs, |prj, cmd| {
     cmd.git_init();
 
     // Installs oz at release tag
@@ -203,17 +203,66 @@ forgetest!(can_update_and_retain_tag_revs, |_prj, cmd| {
 
     let out = cmd.git_submodule_status();
     let status = String::from_utf8_lossy(&out.stdout);
+    let mut lockfile_init = Lockfile::new(prj.root());
+
+    lockfile_init.read().unwrap();
+
+    let deps = lockfile_init.iter().map(|(path, dep_id)| (path, dep_id)).collect::<Vec<_>>();
+    assert_eq!(deps.len(), 2);
+    assert_eq!(
+        deps[0],
+        (
+            &PathBuf::from("lib/openzeppelin-contracts"),
+            &DepIdentifier::Tag {
+                name: "v5.1.0".to_string(),
+                rev: "69c8def5f222ff96f2b5beff05dfba996368aa79".to_string(),
+                r#override: false
+            }
+        )
+    );
+
+    assert_eq!(
+        deps[1],
+        (
+            &PathBuf::from("lib/solady"),
+            &DepIdentifier::Rev { rev: "513f581".to_string(), r#override: false }
+        )
+    );
 
     let submodules_init: Submodules = status.parse().unwrap();
 
     cmd.forge_fuse().arg("update").assert_success();
 
     let out = cmd.git_submodule_status();
     let status = String::from_utf8_lossy(&out.stdout);
-
     let submodules_update: Submodules = status.parse().unwrap();
-
     assert_eq!(submodules_init, submodules_update);
+
+    let mut lockfile_update = Lockfile::new(prj.root());
+
+    lockfile_update.read().unwrap();
+
+    let deps = lockfile_update.iter().map(|(path, dep_id)| (path, dep_id)).collect::<Vec<_>>();
+
+    assert_eq!(deps.len(), 2);
+    assert_eq!(
+        deps[1],
+        (
+            &PathBuf::from("lib/openzeppelin-contracts"),
+            &DepIdentifier::Tag {
+                name: "v5.1.0".to_string(),
+                rev: "69c8def5f222ff96f2b5beff05dfba996368aa79".to_string(),
+                r#override: false
+            }
+        )
+    );
+    assert_eq!(
+        deps[0],
+        (
+            &PathBuf::from("lib/solady"),
+            &DepIdentifier::Rev { rev: "513f581".to_string(), r#override: false }
+        )
+    );
 });
 
 forgetest!(can_override_tag_in_update, |_prj, cmd| {