Fix: Improve merge commit information retrieval

The `get_merge_commit_info` function was previously only checking the
latest commit on a branch for merge information. This could lead to
incomplete or misleading reports if earlier merge commits existed on the
branch.

This commit modifies `get_merge_commit_info` to iterate through all
commits on the specified branch. It now correctly identifies all merge
commits that merged from the given parent branch and extracts their
respective commit messages and statistics.

Additionally, a new test case (`test_get_merge_commit_info_multiple_merges`)
has been added to `tests/merge.rs`. This test verifies that the updated
function correctly retrieves information for multiple merge commits on a
single branch, ensuring more robust and accurate reporting of merge
activities.

Author: google-labs-jules[bot]

src/main.rs

@@ -1725,9 +1725,9 @@ impl GitChain {
         parent_branch: &str,
         branch_name: &str,
     ) -> Result<Vec<MergeCommitInfo>, Error> {
-        // Get the latest commit on the branch
+        // 1. Fetch all commit hashes for the given `branch_name`.
         let mut command = Command::new("git");
-        command.args(["log", "--oneline", "-1", branch_name]);
+        command.args(["log", "--format=%H", branch_name]); // Get only commit hashes
         let output = match command.output() {
             Ok(output) => output,
             Err(_) => return Ok(vec![]), // Return empty vec on error
@@ -1737,92 +1737,101 @@ impl GitChain {
             return Ok(vec![]);
         }
 
-        let latest_commit = String::from_utf8_lossy(&output.stdout).trim().to_string();
-        if latest_commit.is_empty() {
-            return Ok(vec![]);
-        }
+        let commit_hashes_str = String::from_utf8_lossy(&output.stdout);
+        let commit_hashes: Vec<&str> = commit_hashes_str.trim().lines().collect();
 
-        // Check if it's a merge commit by looking for parent commits
-        let commit_hash = latest_commit.split_whitespace().next().unwrap_or("");
-        if commit_hash.is_empty() {
+        if commit_hashes.is_empty() {
             return Ok(vec![]);
         }
 
-        // Get commit information
-        let mut command = Command::new("git");
-        command.args(["show", "--stat", commit_hash]);
-        let output = match command.output() {
-            Ok(output) => output,
-            Err(_) => return Ok(vec![]),
-        };
+        let mut merge_commits_info = Vec::new();
 
-        if !output.status.success() {
-            return Ok(vec![]);
-        }
+        // 2. Iterate through each commit hash.
+        for commit_hash in commit_hashes {
+            if commit_hash.is_empty() {
+                continue;
+            }
+
+            // 3. For each commit, use `git show --stat <commit_hash>` to get its information.
+            let mut show_command = Command::new("git");
+            show_command.args(["show", "--stat", commit_hash]);
+            let show_output = match show_command.output() {
+                Ok(output) => output,
+                Err(_) => continue, // Skip this commit on error
+            };
+
+            if !show_output.status.success() {
+                continue; // Skip this commit on error
+            }
 
-        let commit_info = String::from_utf8_lossy(&output.stdout).to_string();
+            let commit_info_str = String::from_utf8_lossy(&show_output.stdout).to_string();
+            let commit_lines: Vec<&str> = commit_info_str.lines().collect();
 
-        // Check if it's a merge commit, which typically contains "Merge" in the commit message
-        if commit_info.contains(&format!("Merge branch '{}'", parent_branch))
-            || commit_info.contains("Merge branch")
-        {
-            // Extract commit message (first line after commit hash)
-            let commit_lines: Vec<&str> = commit_info.lines().collect();
-            let message = commit_lines
+            // 4. If the commit is a merge commit (contains "Merge branch" in its message AND refers to `parent_branch`),
+            //    extract its message and stats.
+            let merge_line_prefix = format!("Merge branch '{}'", parent_branch);
+            let is_merge_from_parent = commit_lines
                 .iter()
-                .position(|line| line.trim().starts_with("Merge branch"))
-                .map(|idx| commit_lines[idx].trim().to_string());
+                .any(|line| line.trim().starts_with(&merge_line_prefix));
 
-            // Extract stats
-            let stats_line = commit_lines
+            // Also check for generic "Merge branch" if parent_branch is empty (e.g. for initial merge to main)
+            // or if the specific format isn't exact.
+            let is_generic_merge = commit_lines
                 .iter()
-                .find(|line| line.contains("files changed") || line.contains("file changed"));
+                .any(|line| line.trim().starts_with("Merge branch"));
 
-            let stats = stats_line.map(|line| {
-                let mut files_changed = 0;
-                let mut insertions = 0;
-                let mut deletions = 0;
+            // A merge commit must have a line starting with "Merge: "
+            let is_actual_merge_commit = commit_lines.iter().any(|line| line.starts_with("Merge: "));
 
-                if let Some(files_idx) = line.find("file changed") {
-                    if let Some(files_num) = line[..files_idx].split_whitespace().last() {
-                        files_changed = files_num.parse().unwrap_or(0);
-                    }
-                } else if let Some(files_idx) = line.find("files changed") {
-                    if let Some(files_num) = line[..files_idx].split_whitespace().last() {
-                        files_changed = files_num.parse().unwrap_or(0);
-                    }
-                }
+            if is_actual_merge_commit && (is_merge_from_parent || (parent_branch.is_empty() && is_generic_merge)) {
+                // Extract commit message (the line starting with "Merge branch ...")
+                let message = commit_lines
+                    .iter()
+                    .find(|line| line.trim().starts_with("Merge branch"))
+                    .map(|line| line.trim().to_string());
 
-                if let Some(ins_idx) = line.find("insertion") {
-                    if let Some(ins_end) = line[..ins_idx].rfind(' ') {
-                        if let Some(ins_start) = line[..ins_end].rfind(' ') {
-                            let ins_str = &line[ins_start + 1..ins_end];
-                            insertions = ins_str.parse().unwrap_or(0);
+                // Extract stats (preserve existing logic, with slight refinement for parsing)
+                let stats_line = commit_lines
+                    .iter()
+                    .find(|line| line.contains("files changed") || line.contains("file changed"));
+
+                let stats = stats_line.map(|line| {
+                    let mut files_changed = 0;
+                    let mut insertions = 0;
+                    let mut deletions = 0;
+
+                    // Example line: "1 file changed, 5 insertions(+), 2 deletions(-)"
+                    // More robust parsing:
+                    let parts: Vec<&str> = line.split(',').map(|s| s.trim()).collect();
+                    for part in parts {
+                        if part.contains("file changed") || part.contains("files changed") {
+                            if let Some(num_str) = part.split_whitespace().next() {
+                                files_changed = num_str.parse().unwrap_or(0);
+                            }
+                        } else if part.contains("insertion") {
+                            if let Some(num_str) = part.split_whitespace().next() {
+                                insertions = num_str.parse().unwrap_or(0);
+                            }
+                        } else if part.contains("deletion") {
+                            if let Some(num_str) = part.split_whitespace().next() {
+                                deletions = num_str.parse().unwrap_or(0);
+                            }
                         }
                     }
-                }
-
-                if let Some(del_idx) = line.find("deletion") {
-                    if let Some(del_end) = line[..del_idx].rfind(' ') {
-                        if let Some(del_start) = line[..del_end].rfind(' ') {
-                            let del_str = &line[del_start + 1..del_end];
-                            deletions = del_str.parse().unwrap_or(0);
-                        }
+                    MergeStats {
+                        files_changed,
+                        insertions,
+                        deletions,
                     }
-                }
+                });
 
-                MergeStats {
-                    files_changed,
-                    insertions,
-                    deletions,
-                }
-            });
-
-            return Ok(vec![MergeCommitInfo { message, stats }]);
+                merge_commits_info.push(MergeCommitInfo { message, stats });
+            }
         }
 
-        // It's not a merge commit
-        Ok(vec![])
+        // 5. Collect all `MergeCommitInfo` structs for the identified merge commits.
+        // 6. Return `Ok(Vec<MergeCommitInfo>)`
+        Ok(merge_commits_info)
     }
 
     fn report_merge_results(

tests/merge.rs

@@ -296,6 +296,106 @@ fn merge_subcommand_simple() {
     teardown_git_repo(repo_name);
 }
 
+#[test]
+fn test_get_merge_commit_info_multiple_merges() {
+    let repo_name = "test_get_merge_commit_info_multiple_merges";
+    let repo = setup_git_repo(repo_name);
+    let path_to_repo = generate_path_to_repo(repo_name);
+    let original_cwd = std::env::current_dir().unwrap(); // Save CWD
+    std::env::set_current_dir(&path_to_repo).unwrap(); // Change CWD to repo path
+
+    // 1. Set up a Git repository with a `main` branch and a feature branch (e.g., `feat/test-merge-info`).
+    create_new_file(&path_to_repo, "initial.txt", "Initial content on main");
+    first_commit_all(&repo, "Initial commit on main");
+
+    create_branch(&repo, "feat/test-merge-info");
+    checkout_branch(&repo, "feat/test-merge-info");
+    create_new_file(&path_to_repo, "feature_initial.txt", "Initial content on feature branch");
+    commit_all(&repo, "Initial commit on feature branch");
+
+    // 2. Create a few commits on the `main` branch.
+    checkout_branch(&repo, "main");
+    create_new_file(&path_to_repo, "main_commit1.txt", "Main commit 1");
+    commit_all(&repo, "Main C1");
+    create_new_file(&path_to_repo, "main_commit2.txt", "Main commit 2");
+    commit_all(&repo, "Main C2");
+
+    // 3. Create a few commits on the `feat/test-merge-info` branch.
+    checkout_branch(&repo, "feat/test-merge-info");
+    create_new_file(&path_to_repo, "feat_commit1.txt", "Feature commit 1");
+    commit_all(&repo, "Feat C1");
+    create_new_file(&path_to_repo, "feat_commit2.txt", "Feature commit 2");
+    commit_all(&repo, "Feat C2");
+
+    // 4. Merge `main` into `feat/test-merge-info` (this will be the first merge commit).
+    // Add some file changes in this merge.
+    // The stats will reflect files from main brought into feat/test-merge-info.
+    // For this merge, main_commit1.txt and main_commit2.txt are new to feat/test-merge-info.
+    let merge_msg1 = "Merge main into feat/test-merge-info (1st)";
+    run_git_command(&path_to_repo, vec!["merge", "main", "--no-ff", "-m", merge_msg1]);
+
+    // 5. Create more commits on `main`.
+    checkout_branch(&repo, "main");
+    create_new_file(&path_to_repo, "main_commit3.txt", "Main commit 3");
+    commit_all(&repo, "Main C3");
+    create_new_file(&path_to_repo, "main_commit4.txt", "Main commit 4");
+    commit_all(&repo, "Main C4");
+
+    // 6. Create more commits on `feat/test-merge-info`.
+    checkout_branch(&repo, "feat/test-merge-info");
+    create_new_file(&path_to_repo, "feat_commit3.txt", "Feature commit 3");
+    commit_all(&repo, "Feat C3");
+
+    // 7. Merge `main` into `feat/test-merge-info` again (this will be the second merge commit).
+    // Add different file changes in this merge.
+    // For this merge, main_commit3.txt and main_commit4.txt are new to feat/test-merge-info.
+    let merge_msg2 = "Merge main into feat/test-merge-info (2nd)";
+    run_git_command(&path_to_repo, vec!["merge", "main", "--no-ff", "-m", merge_msg2]);
+
+    // 8. Call `get_merge_commit_info` with `parent_branch` as "main" and `branch_name` as "feat/test-merge-info".
+    // Need to instantiate GitChain for the current repo.
+    // The GitChain::init() discovers the repo from the current directory.
+    let git_chain_path = Path::new("."); // Relative to current_dir which is path_to_repo
+    let git_chain = git_chain::GitChain::init(git_chain_path).expect("Failed to init GitChain");
+    
+    let merge_infos_result = git_chain.get_merge_commit_info("main", "feat/test-merge-info");
+
+    // 9. Assert that the function returns `Ok` with a vector containing two `MergeCommitInfo` structs.
+    assert!(merge_infos_result.is_ok(), "get_merge_commit_info should return Ok");
+    let merge_infos = merge_infos_result.unwrap();
+    assert_eq!(merge_infos.len(), 2, "Should find 2 merge commits from main");
+
+    // 10. Assert that the extracted information (message, stats) for both merge commits is correct.
+    // Merge commits are typically listed in reverse chronological order by `git log`.
+    // The function as implemented iterates `git log` and then `git show` for each,
+    // so the order in `merge_infos` should be reverse chronological.
+
+    // Second merge (most recent)
+    let info2 = &merge_infos[0];
+    assert_eq!(info2.message, Some(merge_msg2.to_string()));
+    assert!(info2.stats.is_some(), "Stats should be present for the 2nd merge");
+    if let Some(stats) = &info2.stats {
+        // main_commit3.txt, main_commit4.txt are introduced.
+        // Exact insertions/deletions depend on line count, but files_changed should be 2.
+        assert_eq!(stats.files_changed, 2, "2nd merge: files_changed should be 2");
+        assert!(stats.insertions > 0, "2nd merge: insertions should be > 0");
+        // Deletions could be 0 if only new files are added.
+    }
+
+    // First merge
+    let info1 = &merge_infos[1];
+    assert_eq!(info1.message, Some(merge_msg1.to_string()));
+    assert!(info1.stats.is_some(), "Stats should be present for the 1st merge");
+    if let Some(stats) = &info1.stats {
+        // main_commit1.txt, main_commit2.txt are introduced.
+        assert_eq!(stats.files_changed, 2, "1st merge: files_changed should be 2");
+        assert!(stats.insertions > 0, "1st merge: insertions should be > 0");
+    }
+    
+    std::env::set_current_dir(original_cwd).unwrap(); // Restore CWD
+    teardown_git_repo(repo_name);
+}
+
 #[test]
 fn merge_subcommand_with_ahead_behind() {
     // Test that merge command works with branches that are ahead and behind