Move git index check to commit_changes and remove CommitLocksOutcome struct

Co-authored-by: Byron <[email protected]>

Author: Copilot

src/command/release/git.rs

@@ -12,21 +12,44 @@ pub(in crate::command::release_impl) fn commit_changes<'a>(
     dry_run: bool,
     empty_commit_possible: bool,
     signoff: bool,
-    new_changelogs: &[impl AsRef<Path>],
+    changelog_paths: &[impl AsRef<Path>],
     ctx: &'a crate::Context,
 ) -> anyhow::Result<Option<Id<'a>>> {
     // TODO: replace with gitoxide one day
-    // Add new changelog files to git before committing, as `git commit -a` only stages tracked files
-    if !new_changelogs.is_empty() {
-        let mut add_cmd = Command::new("git");
-        add_cmd.arg("add").arg("--");
-        for path in new_changelogs {
-            add_cmd.arg(path.as_ref());
-        }
-        log::trace!("{} run {:?}", will(dry_run), add_cmd);
-        if !dry_run && !add_cmd.status()?.success() {
-            let paths: Vec<_> = new_changelogs.iter().map(|p| p.as_ref().display().to_string()).collect();
-            bail!("Failed to add new changelog files to git: {}", paths.join(", "));
+    // Add changelog files that are not yet tracked in git index.
+    // `git commit -am` only stages tracked files, so we need to explicitly add new ones.
+    if !changelog_paths.is_empty() {
+        let index = ctx.repo.open_index()?;
+        let worktree = ctx.repo.workdir().expect("this is a worktree repository");
+        
+        let untracked_paths: Vec<_> = changelog_paths
+            .iter()
+            .filter(|path| {
+                // Convert absolute path to worktree-relative path with forward slashes
+                path.as_ref()
+                    .strip_prefix(worktree)
+                    .ok()
+                    .map(|relative_path| {
+                        let relative_path_unix =
+                            gix::path::to_unix_separators(gix::path::into_bstr(relative_path));
+                        // Check if the path is NOT tracked in the git index
+                        index.entry_by_path(relative_path_unix.as_bstr()).is_none()
+                    })
+                    .unwrap_or(false)
+            })
+            .collect();
+
+        if !untracked_paths.is_empty() {
+            let mut add_cmd = Command::new("git");
+            add_cmd.arg("add").arg("--");
+            for path in &untracked_paths {
+                add_cmd.arg(path.as_ref());
+            }
+            log::trace!("{} run {:?}", will(dry_run), add_cmd);
+            if !dry_run && !add_cmd.status()?.success() {
+                let paths: Vec<_> = untracked_paths.iter().map(|p| p.as_ref().display().to_string()).collect();
+                bail!("Failed to add new changelog files to git: {}", paths.join(", "));
+            }
         }
     }
 
@@ -190,33 +213,4 @@ mod tests {
             assert_eq!(captured_logs[0].level, Level::Trace);
         });
     }
-
-    #[test]
-    #[ignore = "TBD: isolate properly, worked in PR, but stopped working in CI"]
-    fn test_commit_changes_with_new_changelog() {
-        let ctx = crate::Context::new(
-            vec![],
-            false,
-            crate::version::BumpSpec::Auto,
-            crate::version::BumpSpec::Auto,
-        )
-        .unwrap();
-        let message = "commit message";
-        let new_changelogs: &[&std::path::Path] = &[std::path::Path::new("/some/path/CHANGELOG.md")];
-        testing_logger::setup();
-        let _ = commit_changes(message, true, false, false, new_changelogs, &ctx).unwrap();
-        testing_logger::validate(|captured_logs| {
-            assert_eq!(captured_logs.len(), 2);
-            assert_eq!(
-                captured_logs[0].body,
-                "WOULD run \"git\" \"add\" \"--\" \"/some/path/CHANGELOG.md\""
-            );
-            assert_eq!(captured_logs[0].level, Level::Trace);
-            assert_eq!(
-                captured_logs[1].body,
-                "WOULD run \"git\" \"commit\" \"-am\" \"commit message\""
-            );
-            assert_eq!(captured_logs[1].level, Level::Trace);
-        });
-    }
 }

src/command/release/manifest.rs

@@ -7,7 +7,7 @@ use std::{
 
 use anyhow::{bail, Context as ContextTrait};
 use cargo_metadata::{camino::Utf8PathBuf, Package};
-use gix::{bstr::ByteSlice, lock::File, Id};
+use gix::{lock::File, Id};
 use semver::{Version, VersionReq};
 
 use super::{cargo, git, Context, Options};
@@ -94,10 +94,13 @@ pub(in crate::command::release_impl) fn edit_version_and_fixup_dependent_crates_
 
     preview_changelogs(ctx, &pending_changelogs, opts.clone())?;
 
-    let CommitLocksOutcome {
-        bail_message,
-        new_changelog_paths,
-    } = commit_locks_and_generate_bail_message(
+    // Collect all changelog paths before they are consumed by commit_locks_and_generate_bail_message
+    let changelog_paths: Vec<std::path::PathBuf> = pending_changelogs
+        .iter()
+        .map(|(_, _, lock)| lock.resource_path().to_owned())
+        .collect();
+
+    let bail_message = commit_locks_and_generate_bail_message(
         ctx,
         pending_changelogs,
         locks_by_manifest_path,
@@ -111,7 +114,7 @@ pub(in crate::command::release_impl) fn edit_version_and_fixup_dependent_crates_
         dry_run,
         !made_change,
         opts.signoff,
-        &new_changelog_paths,
+        &changelog_paths,
         &ctx.base,
     )?;
     if let Some(bail_message) = bail_message {
@@ -124,12 +127,6 @@ pub(in crate::command::release_impl) fn edit_version_and_fixup_dependent_crates_
     }
 }
 
-struct CommitLocksOutcome {
-    bail_message: Option<String>,
-    /// Paths to changelog files that were newly created (not tracked by git)
-    new_changelog_paths: Vec<std::path::PathBuf>,
-}
-
 fn commit_locks_and_generate_bail_message(
     ctx: &Context,
     pending_changelogs: Vec<(&Package, bool, File)>,
@@ -143,32 +140,7 @@ fn commit_locks_and_generate_bail_message(
         allow_empty_release_message,
         ..
     }: Options,
-) -> anyhow::Result<CommitLocksOutcome> {
-    // Collect paths to changelog files not in the git index.
-    // These need to be explicitly added to git since `git commit -am` only stages tracked files.
-    let index = ctx.base.repo.open_index()?;
-    let worktree = ctx
-        .base
-        .repo
-        .workdir()
-        .expect("this is a worktree repository");
-    let new_changelog_paths: Vec<std::path::PathBuf> = pending_changelogs
-        .iter()
-        .filter_map(|(_, _, lock)| {
-            let path = lock.resource_path();
-            // Convert absolute path to worktree-relative path with forward slashes
-            let relative_path = path.strip_prefix(worktree).ok()?;
-            let relative_path_unix =
-                gix::path::to_unix_separators(gix::path::into_bstr(relative_path));
-            // Check if the path is tracked in the git index
-            if index.entry_by_path(relative_path_unix.as_bstr()).is_none() {
-                Some(path.to_owned())
-            } else {
-                None
-            }
-        })
-        .collect();
-
+) -> anyhow::Result<Option<String>> {
     let bail_message = if !dry_run {
         let mut packages_whose_changelogs_need_edits = None;
         let mut packages_which_might_be_fully_generated = None;
@@ -294,10 +266,7 @@ fn commit_locks_and_generate_bail_message(
         }
         None
     };
-    Ok(CommitLocksOutcome {
-        bail_message,
-        new_changelog_paths,
-    })
+    Ok(bail_message)
 }
 
 fn preview_changelogs(