From f93ec22627608f9eaf1ec513e86cd48c763c7b89 Mon Sep 17 00:00:00 2001 From: Seong Yong-ju Date: Thu, 11 Dec 2025 19:29:41 +0900 Subject: [PATCH 1/5] feat: convert merge operation to interactive picker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Convert Merge operation to use interactive picker with fuzzy search - Add comprehensive test coverage with 5 test cases - Exclude current branch from merge target list - Support custom input for commit hashes and relative refs - All tests verify operations execute correctly πŸ€– Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 --- src/ops/merge.rs | 78 ++++++++++++++++--- src/tests/merge.rs | 54 ++++++++++++- .../gitu__tests__merge__merge_picker.snap | 25 ++++++ ...tu__tests__merge__merge_picker_cancel.snap | 25 ++++++ ...sts__merge__merge_picker_custom_input.snap | 25 ++++++ ..._tests__merge__merge_select_from_list.snap | 25 ++++++ ...tests__merge__merge_use_custom_input.snap} | 4 +- 7 files changed, 221 insertions(+), 15 deletions(-) create mode 100644 src/tests/snapshots/gitu__tests__merge__merge_picker.snap create mode 100644 src/tests/snapshots/gitu__tests__merge__merge_picker_cancel.snap create mode 100644 src/tests/snapshots/gitu__tests__merge__merge_picker_custom_input.snap create mode 100644 src/tests/snapshots/gitu__tests__merge__merge_select_from_list.snap rename src/tests/snapshots/{gitu__tests__merge__merge_prompt.snap => gitu__tests__merge__merge_use_custom_input.snap} (93%) diff --git a/src/ops/merge.rs b/src/ops/merge.rs index 9986b50315..6396080d32 100644 --- a/src/ops/merge.rs +++ b/src/ops/merge.rs @@ -1,9 +1,11 @@ use super::{Action, OpTrait, selected_rev}; use crate::{ Res, - app::{App, PromptParams, State}, + app::{App, State}, + error::Error, item_data::ItemData, menu::arg::Arg, + picker::{PickerData, PickerItem, PickerState}, term::Term, }; use std::{process::Command, rc::Rc}; @@ -66,16 +68,70 @@ pub(crate) struct Merge; impl OpTrait for Merge { fn get_action(&self, _target: &ItemData) -> Option { Some(Rc::new(|app: &mut App, term: &mut Term| { - let rev = app.prompt( - term, - &PromptParams { - prompt: "Merge", - create_default_value: Box::new(selected_rev), - ..Default::default() - }, - )?; - - merge(app, term, &rev)?; + let default_rev = selected_rev(app); + let mut items = Vec::new(); + let mut seen = std::collections::HashSet::new(); + + // Add default value first if it exists + if let Some(ref default) = default_rev { + items.push(PickerItem::new( + default.clone(), + PickerData::Revision(default.clone()), + )); + seen.insert(default.clone()); + } + + // Get current branch name to exclude it + let current_branch = app.state.repo.head().ok() + .and_then(|head| head.shorthand().map(|s| s.to_string())); + + // Get all branches + let branches = app.state.repo.branches(None).map_err(Error::ListGitReferences)?; + for branch in branches { + let (branch, _) = branch.map_err(Error::ListGitReferences)?; + if let Some(name) = branch.name().map_err(Error::ListGitReferences)? { + let name = name.to_string(); + // Skip current branch and already seen names + if Some(&name) != current_branch.as_ref() && !seen.contains(&name) { + items.push(PickerItem::new(name.clone(), PickerData::Revision(name.clone()))); + seen.insert(name); + } + } + } + + // Get all tags + let tag_names = app.state.repo.tag_names(None).map_err(Error::ListGitReferences)?; + for tag_name in tag_names.iter().flatten() { + let tag_name = tag_name.to_string(); + if !seen.contains(&tag_name) { + items.push(PickerItem::new(tag_name.clone(), PickerData::Revision(tag_name.clone()))); + seen.insert(tag_name); + } + } + + // Get all remote branches + let references = app.state.repo.references().map_err(Error::ListGitReferences)?; + for reference in references { + let reference = reference.map_err(Error::ListGitReferences)?; + if reference.is_remote() && let Some(name) = reference.shorthand() { + let name = name.to_string(); + if !seen.contains(&name) { + items.push(PickerItem::new(name.clone(), PickerData::Revision(name.clone()))); + seen.insert(name); + } + } + } + + // Allow custom input to support commit hashes, relative refs (e.g., HEAD~3), + // and other git revisions not in the predefined list + let picker = PickerState::new("Merge", items, true); + let result = app.picker(term, picker)?; + + if let Some(data) = result { + let rev = data.display(); + merge(app, term, rev)?; + } + Ok(()) })) } diff --git a/src/tests/merge.rs b/src/tests/merge.rs index 430eb6d96d..be132e8fbb 100644 --- a/src/tests/merge.rs +++ b/src/tests/merge.rs @@ -9,14 +9,64 @@ fn setup(ctx: TestContext) -> TestContext { ctx } +fn setup_picker(ctx: TestContext) -> TestContext { + // Create multiple branches for merging + run(&ctx.dir, &["git", "checkout", "-b", "feature-a"]); + commit(&ctx.dir, "feature-a commit", ""); + run(&ctx.dir, &["git", "checkout", "main"]); + + run(&ctx.dir, &["git", "checkout", "-b", "feature-b"]); + commit(&ctx.dir, "feature-b commit", ""); + run(&ctx.dir, &["git", "checkout", "main"]); + + run(&ctx.dir, &["git", "checkout", "-b", "bugfix-123"]); + commit(&ctx.dir, "bugfix commit", ""); + run(&ctx.dir, &["git", "checkout", "main"]); + + // Create some tags + run(&ctx.dir, &["git", "tag", "v1.0.0", "feature-a"]); + run(&ctx.dir, &["git", "tag", "v2.0.0", "feature-b"]); + + ctx +} + #[test] fn merge_menu() { snapshot!(setup(setup_clone!()), "m"); } #[test] -fn merge_prompt() { - snapshot!(setup(setup_clone!()), "mm"); +fn merge_picker() { + snapshot!(setup_picker(setup_clone!()), "mm"); +} + +#[test] +fn merge_picker_custom_input() { + snapshot!(setup_picker(setup_clone!()), "mmHEAD~2"); +} + +#[test] +fn merge_picker_cancel() { + snapshot!(setup_picker(setup_clone!()), "mm"); +} + +#[test] +fn merge_select_from_list() { + // Select feature-a branch from the list and merge it + with_var("GIT_MERGE_AUTOEDIT", Some("no"), || { + snapshot!(setup_picker(setup_clone!()), "mmfeature-a"); + }); +} + +#[test] +fn merge_use_custom_input() { + // Use custom input with full commit hash + with_var("GIT_MERGE_AUTOEDIT", Some("no"), || { + snapshot!( + setup_picker(setup_clone!()), + "mmb66a0bf82020d6a386e94d0fceedec1f817d20c7" + ); + }); } #[test] diff --git a/src/tests/snapshots/gitu__tests__merge__merge_picker.snap b/src/tests/snapshots/gitu__tests__merge__merge_picker.snap new file mode 100644 index 0000000000..7fc1b284ff --- /dev/null +++ b/src/tests/snapshots/gitu__tests__merge__merge_picker.snap @@ -0,0 +1,25 @@ +--- +source: src/tests/merge.rs +expression: ctx.redact_buffer() +--- + On branch main | + Your branch is up to date with 'origin/main'. | + | + Recent commits | + b66a0bf main origin/main add initial-file | + | + | + | +────────────────────────────────────────────────────────────────────────────────| + 7/7 Merge β€Ί β–ˆ | +β–Œbugfix-123 | + feature-a | + feature-b | + origin/HEAD | + origin/main | + v1.0.0 | + v2.0.0 | + | + | + | +styles_hash: 16f8dd2fc9f8bc2f diff --git a/src/tests/snapshots/gitu__tests__merge__merge_picker_cancel.snap b/src/tests/snapshots/gitu__tests__merge__merge_picker_cancel.snap new file mode 100644 index 0000000000..9e3885316e --- /dev/null +++ b/src/tests/snapshots/gitu__tests__merge__merge_picker_cancel.snap @@ -0,0 +1,25 @@ +--- +source: src/tests/merge.rs +expression: ctx.redact_buffer() +--- +β–ŒOn branch main | +β–ŒYour branch is up to date with 'origin/main'. | + | + Recent commits | + b66a0bf main origin/main add initial-file | + | + | + | + | + | + | + | + | + | +────────────────────────────────────────────────────────────────────────────────| + Merge Arguments | + m merge -f Fast-forward only (--ff-only) | + a abort -n No fast-forward (--no-ff) | + c continue | + q/ Quit/Close | +styles_hash: 7b86e45a0c70a078 diff --git a/src/tests/snapshots/gitu__tests__merge__merge_picker_custom_input.snap b/src/tests/snapshots/gitu__tests__merge__merge_picker_custom_input.snap new file mode 100644 index 0000000000..ac89e1a23f --- /dev/null +++ b/src/tests/snapshots/gitu__tests__merge__merge_picker_custom_input.snap @@ -0,0 +1,25 @@ +--- +source: src/tests/merge.rs +expression: ctx.redact_buffer() +--- + On branch main | + Your branch is up to date with 'origin/main'. | + | + Recent commits | + b66a0bf main origin/main add initial-file | + | + | + | +────────────────────────────────────────────────────────────────────────────────| + 0/7 Merge β€Ί HEAD~2β–ˆ | +β–ŒHEAD~2 | + | + | + | + | + | + | + | + | + | +styles_hash: 6f95b383fced9e3c diff --git a/src/tests/snapshots/gitu__tests__merge__merge_select_from_list.snap b/src/tests/snapshots/gitu__tests__merge__merge_select_from_list.snap new file mode 100644 index 0000000000..4f622169b7 --- /dev/null +++ b/src/tests/snapshots/gitu__tests__merge__merge_select_from_list.snap @@ -0,0 +1,25 @@ +--- +source: src/tests/merge.rs +expression: ctx.redact_buffer() +--- +β–ŒOn branch main | +β–ŒYour branch is ahead of 'origin/main' by 1 commit(s). | + | + Recent commits | + 3b23a7d feature-a main v1.0.0 add feature-a commit | + b66a0bf origin/main add initial-file | + | + | + | + | + | + | + | + | + | + | + | + | +────────────────────────────────────────────────────────────────────────────────| +$ git merge feature-a | +styles_hash: 994114f03d7a9c3c diff --git a/src/tests/snapshots/gitu__tests__merge__merge_prompt.snap b/src/tests/snapshots/gitu__tests__merge__merge_use_custom_input.snap similarity index 93% rename from src/tests/snapshots/gitu__tests__merge__merge_prompt.snap rename to src/tests/snapshots/gitu__tests__merge__merge_use_custom_input.snap index 28c7bfcfd6..ca2a305f50 100644 --- a/src/tests/snapshots/gitu__tests__merge__merge_prompt.snap +++ b/src/tests/snapshots/gitu__tests__merge__merge_use_custom_input.snap @@ -21,5 +21,5 @@ expression: ctx.redact_buffer() | | ────────────────────────────────────────────────────────────────────────────────| -? Merge: β€Ί β–ˆ | -styles_hash: 60de28e06c7442d8 +$ git merge b66a0bf82020d6a386e94d0fceedec1f817d20c7 | +styles_hash: 1913e3f30ba0cc1b From 46d47d04789e34f8e657d0c39684c726f965d3d3 Mon Sep 17 00:00:00 2001 From: Seong Yong-ju Date: Mon, 19 Jan 2026 16:34:11 +0900 Subject: [PATCH 2/5] feat(merge): handle duplicate ref names and exclude current ref from picker - Exclude current ref from merge picker whether it's a branch, tag, or remote - Add RefKind::to_full_refname() and shorthand() helper methods - Handle duplicate branch/tag names with refs/ prefixes - Use full refnames for git merge when duplicates exist - Add comprehensive tests for edge cases with duplicate names --- src/item_data.rs | 18 ++ src/ops/merge.rs | 164 +++++++++++++----- src/tests/merge.rs | 104 +++++++++-- .../gitu__tests__merge__merge_picker.snap | 4 +- ...ker_current_branch_with_same_tag_name.snap | 25 +++ ...ker_default_branch_with_same_tag_name.snap | 25 +++ ...ker_default_tag_with_same_branch_name.snap | 25 +++ ...rge_picker_duplicate_branch_tag_names.snap | 25 +++ ..._picker_duplicate_names_select_branch.snap | 25 +++ ...rge_picker_duplicate_names_select_tag.snap | 25 +++ 10 files changed, 387 insertions(+), 53 deletions(-) create mode 100644 src/tests/snapshots/gitu__tests__merge__merge_picker_current_branch_with_same_tag_name.snap create mode 100644 src/tests/snapshots/gitu__tests__merge__merge_picker_default_branch_with_same_tag_name.snap create mode 100644 src/tests/snapshots/gitu__tests__merge__merge_picker_default_tag_with_same_branch_name.snap create mode 100644 src/tests/snapshots/gitu__tests__merge__merge_picker_duplicate_branch_tag_names.snap create mode 100644 src/tests/snapshots/gitu__tests__merge__merge_picker_duplicate_names_select_branch.snap create mode 100644 src/tests/snapshots/gitu__tests__merge__merge_picker_duplicate_names_select_tag.snap diff --git a/src/item_data.rs b/src/item_data.rs index 1bed5a5301..8755c90469 100644 --- a/src/item_data.rs +++ b/src/item_data.rs @@ -74,6 +74,24 @@ pub(crate) enum RefKind { Remote(String), } +impl RefKind { + /// Convert to fully qualified refname (e.g., "refs/heads/main", "refs/tags/v1.0.0") + pub(crate) fn to_full_refname(&self) -> String { + match self { + RefKind::Branch(name) => format!("refs/heads/{}", name), + RefKind::Tag(name) => format!("refs/tags/{}", name), + RefKind::Remote(name) => format!("refs/remotes/{}", name), + } + } + + /// Get the shorthand name without refs/ prefix + pub(crate) fn shorthand(&self) -> &str { + match self { + RefKind::Branch(name) | RefKind::Tag(name) | RefKind::Remote(name) => name, + } + } +} + #[derive(Clone, Debug)] pub(crate) enum SectionHeader { Remote(String), diff --git a/src/ops/merge.rs b/src/ops/merge.rs index 6396080d32..e7d7344a39 100644 --- a/src/ops/merge.rs +++ b/src/ops/merge.rs @@ -1,4 +1,4 @@ -use super::{Action, OpTrait, selected_rev}; +use super::{Action, OpTrait}; use crate::{ Res, app::{App, State}, @@ -66,60 +66,142 @@ fn merge(app: &mut App, term: &mut Term, rev: &str) -> Res<()> { pub(crate) struct Merge; impl OpTrait for Merge { - fn get_action(&self, _target: &ItemData) -> Option { - Some(Rc::new(|app: &mut App, term: &mut Term| { - let default_rev = selected_rev(app); - let mut items = Vec::new(); - let mut seen = std::collections::HashSet::new(); - - // Add default value first if it exists - if let Some(ref default) = default_rev { - items.push(PickerItem::new( - default.clone(), - PickerData::Revision(default.clone()), - )); - seen.insert(default.clone()); - } + fn get_action(&self, target: &ItemData) -> Option { + use crate::item_data::RefKind; - // Get current branch name to exclude it - let current_branch = app.state.repo.head().ok() - .and_then(|head| head.shorthand().map(|s| s.to_string())); + // Extract default ref from target if it's a Reference + let default_ref = if let ItemData::Reference { kind, .. } = target { + Some(kind.clone()) + } else { + None + }; - // Get all branches - let branches = app.state.repo.branches(None).map_err(Error::ListGitReferences)?; - for branch in branches { - let (branch, _) = branch.map_err(Error::ListGitReferences)?; + Some(Rc::new(move |app: &mut App, term: &mut Term| { + let mut items = Vec::new(); + let mut shorthand_count: std::collections::HashMap = + std::collections::HashMap::new(); + let mut branches: Vec = Vec::new(); + let mut tags: Vec = Vec::new(); + let mut remotes: Vec = Vec::new(); + + // Get current HEAD reference to exclude it from picker + // HEAD can be a branch, tag, remote, or detached (pointing to a commit) + let current_ref: Option = app.state.repo.head().ok().and_then(|head| { + // Get the full reference name from HEAD + // For branches: "refs/heads/main", for detached: "HEAD" + head.name().and_then(|name| { + name.strip_prefix("refs/heads/") + .map(|s| RefKind::Branch(s.to_string())) + .or_else(|| { + name.strip_prefix("refs/tags/") + .map(|s| RefKind::Tag(s.to_string())) + }) + .or_else(|| { + name.strip_prefix("refs/remotes/") + .map(|s| RefKind::Remote(s.to_string())) + }) + // If none match, returns None (detached HEAD or other ref type) + }) + }); + + // Collect all branches (local and remote) + let branches_iter = app + .state + .repo + .branches(None) + .map_err(Error::ListGitReferences)?; + for branch in branches_iter { + let (branch, branch_type) = branch.map_err(Error::ListGitReferences)?; if let Some(name) = branch.name().map_err(Error::ListGitReferences)? { let name = name.to_string(); - // Skip current branch and already seen names - if Some(&name) != current_branch.as_ref() && !seen.contains(&name) { - items.push(PickerItem::new(name.clone(), PickerData::Revision(name.clone()))); - seen.insert(name); + match branch_type { + git2::BranchType::Local => { + let ref_kind = RefKind::Branch(name.clone()); + *shorthand_count.entry(name).or_insert(0) += 1; + branches.push(ref_kind); + } + git2::BranchType::Remote => { + let ref_kind = RefKind::Remote(name); + remotes.push(ref_kind); + } } } } - // Get all tags - let tag_names = app.state.repo.tag_names(None).map_err(Error::ListGitReferences)?; + // Collect all tags (count for duplicate detection) + let tag_names = app + .state + .repo + .tag_names(None) + .map_err(Error::ListGitReferences)?; for tag_name in tag_names.iter().flatten() { let tag_name = tag_name.to_string(); - if !seen.contains(&tag_name) { - items.push(PickerItem::new(tag_name.clone(), PickerData::Revision(tag_name.clone()))); - seen.insert(tag_name); - } + let ref_kind = RefKind::Tag(tag_name.clone()); + *shorthand_count.entry(tag_name).or_insert(0) += 1; + tags.push(ref_kind); } - // Get all remote branches - let references = app.state.repo.references().map_err(Error::ListGitReferences)?; - for reference in references { - let reference = reference.map_err(Error::ListGitReferences)?; - if reference.is_remote() && let Some(name) = reference.shorthand() { - let name = name.to_string(); - if !seen.contains(&name) { - items.push(PickerItem::new(name.clone(), PickerData::Revision(name.clone()))); - seen.insert(name); + // Note: current_ref is already counted in shorthand_count + // during branch/tag collection, so no need to count it again + + // Add default ref first if it exists + if let Some(ref default) = default_ref { + let shorthand = default.shorthand(); + let (display, refname) = match default { + RefKind::Remote(_) => { + // Remotes never have duplicates + (shorthand.to_string(), shorthand.to_string()) + } + _ => { + let is_duplicate = shorthand_count.get(shorthand).is_some_and(|&c| c > 1); + if is_duplicate { + let full_refname = default.to_full_refname(); + (full_refname.clone(), full_refname) + } else { + (shorthand.to_string(), shorthand.to_string()) + } } + }; + items.push(PickerItem::new(display, PickerData::Revision(refname))); + } + + // Add all refs (branches, then tags, then remotes) + for ref_kind in branches.into_iter().chain(tags).chain(remotes) { + let shorthand = ref_kind.shorthand(); + + // Skip current ref (HEAD) - could be branch, tag, or remote + if current_ref + .as_ref() + .is_some_and(|current| current.to_full_refname() == ref_kind.to_full_refname()) + { + continue; + } + + // Skip if it's the same as default (compare by full refname to distinguish branch/tag) + if default_ref + .as_ref() + .is_some_and(|d| d.to_full_refname() == ref_kind.to_full_refname()) + { + continue; } + + // Handle duplicates (only for branches and tags) + let (display, refname) = match &ref_kind { + RefKind::Remote(_) => { + // Remotes never have duplicates (they have remote/ prefix) + (shorthand.to_string(), shorthand.to_string()) + } + _ => { + let is_duplicate = shorthand_count.get(shorthand).is_some_and(|&c| c > 1); + if is_duplicate { + let full_refname = ref_kind.to_full_refname(); + (full_refname.clone(), full_refname) + } else { + (shorthand.to_string(), shorthand.to_string()) + } + } + }; + items.push(PickerItem::new(display, PickerData::Revision(refname))); } // Allow custom input to support commit hashes, relative refs (e.g., HEAD~3), diff --git a/src/tests/merge.rs b/src/tests/merge.rs index be132e8fbb..af401a3620 100644 --- a/src/tests/merge.rs +++ b/src/tests/merge.rs @@ -2,14 +2,14 @@ use temp_env::with_var; use super::*; -fn setup(ctx: TestContext) -> TestContext { +fn setup_branch(ctx: TestContext) -> TestContext { run(&ctx.dir, &["git", "checkout", "-b", "other-branch"]); commit(&ctx.dir, "new-file", "hello"); run(&ctx.dir, &["git", "checkout", "main"]); ctx } -fn setup_picker(ctx: TestContext) -> TestContext { +fn setup_branches(ctx: TestContext) -> TestContext { // Create multiple branches for merging run(&ctx.dir, &["git", "checkout", "-b", "feature-a"]); commit(&ctx.dir, "feature-a commit", ""); @@ -30,31 +30,115 @@ fn setup_picker(ctx: TestContext) -> TestContext { ctx } +fn setup_branch_tag_same_name(ctx: TestContext) -> TestContext { + // Create a branch named v1.0.0 + run(&ctx.dir, &["git", "checkout", "-b", "v1.0.0"]); + commit(&ctx.dir, "branch commit", ""); + run(&ctx.dir, &["git", "checkout", "main"]); + + // Create a different branch with different content + run(&ctx.dir, &["git", "checkout", "-b", "other"]); + commit(&ctx.dir, "other commit", ""); + + // Create a tag also named v1.0.0 pointing to this different commit + run(&ctx.dir, &["git", "tag", "v1.0.0"]); + run(&ctx.dir, &["git", "checkout", "main"]); + + ctx +} + +fn setup_on_branch_with_same_tag(ctx: TestContext) -> TestContext { + // Create and checkout a branch named v1.0.0 + run(&ctx.dir, &["git", "checkout", "-b", "v1.0.0"]); + commit(&ctx.dir, "branch commit", ""); + + // Create a tag also named v1.0.0 pointing to current commit + run(&ctx.dir, &["git", "tag", "v1.0.0"]); + + // Stay on the v1.0.0 branch + ctx +} + #[test] fn merge_menu() { - snapshot!(setup(setup_clone!()), "m"); + snapshot!(setup_branch(setup_clone!()), "m"); } #[test] fn merge_picker() { - snapshot!(setup_picker(setup_clone!()), "mm"); + snapshot!(setup_branches(setup_clone!()), "mm"); +} + +#[test] +fn merge_picker_duplicate_branch_tag_names() { + // Test that when branch and tag have the same name, + // picker displays them with prefixes (heads/v1.0.0, tags/v1.0.0) + snapshot!(setup_branch_tag_same_name(setup_clone!()), "mm"); +} + +#[test] +fn merge_picker_duplicate_names_select_branch() { + // Test merging the branch when there's a duplicate name + with_var("GIT_MERGE_AUTOEDIT", Some("no"), || { + snapshot!( + setup_branch_tag_same_name(setup_clone!()), + "mmheads/v1.0.0" + ); + }); +} + +#[test] +fn merge_picker_duplicate_names_select_tag() { + // Test merging the tag when there's a duplicate name + with_var("GIT_MERGE_AUTOEDIT", Some("no"), || { + snapshot!( + setup_branch_tag_same_name(setup_clone!()), + "mmtags/v1.0.0" + ); + }); +} + +#[test] +fn merge_picker_current_branch_with_same_tag_name() { + // Test that when current branch and tag have the same name, + // current branch is excluded from picker, only tag is shown + snapshot!(setup_on_branch_with_same_tag(setup_clone!()), "mm"); +} + +#[test] +fn merge_picker_default_tag_with_same_branch_name() { + // Test that when selecting a tag, and a branch with same name exists, + // both are shown but default (tag) is not duplicated + // Navigate to tags view and select v1.0.0 tag, then open merge picker + snapshot!( + setup_branch_tag_same_name(setup_clone!()), + "Yjjjjjjjjmm" + ); +} + +#[test] +fn merge_picker_default_branch_with_same_tag_name() { + // Test that when selecting a branch, and a tag with same name exists, + // both are shown but default (branch) is not duplicated + // Navigate to branches view and select v1.0.0 branch, then open merge picker + snapshot!(setup_branch_tag_same_name(setup_clone!()), "Yjjjmm"); } #[test] fn merge_picker_custom_input() { - snapshot!(setup_picker(setup_clone!()), "mmHEAD~2"); + snapshot!(setup_branches(setup_clone!()), "mmHEAD~2"); } #[test] fn merge_picker_cancel() { - snapshot!(setup_picker(setup_clone!()), "mm"); + snapshot!(setup_branches(setup_clone!()), "mm"); } #[test] fn merge_select_from_list() { // Select feature-a branch from the list and merge it with_var("GIT_MERGE_AUTOEDIT", Some("no"), || { - snapshot!(setup_picker(setup_clone!()), "mmfeature-a"); + snapshot!(setup_branches(setup_clone!()), "mmfeature-a"); }); } @@ -63,7 +147,7 @@ fn merge_use_custom_input() { // Use custom input with full commit hash with_var("GIT_MERGE_AUTOEDIT", Some("no"), || { snapshot!( - setup_picker(setup_clone!()), + setup_branches(setup_clone!()), "mmb66a0bf82020d6a386e94d0fceedec1f817d20c7" ); }); @@ -71,12 +155,12 @@ fn merge_use_custom_input() { #[test] fn merge_ff_only() { - snapshot!(setup(setup_clone!()), "m-fmother-branch"); + snapshot!(setup_branch(setup_clone!()), "m-fmother-branch"); } #[test] fn merge_no_ff() { with_var("GIT_MERGE_AUTOEDIT", Some("no"), || { - snapshot!(setup(setup_clone!()), "m-nmother-branch"); + snapshot!(setup_branch(setup_clone!()), "m-nmother-branch"); }); } diff --git a/src/tests/snapshots/gitu__tests__merge__merge_picker.snap b/src/tests/snapshots/gitu__tests__merge__merge_picker.snap index 7fc1b284ff..b187e8a8e4 100644 --- a/src/tests/snapshots/gitu__tests__merge__merge_picker.snap +++ b/src/tests/snapshots/gitu__tests__merge__merge_picker.snap @@ -15,10 +15,10 @@ expression: ctx.redact_buffer() β–Œbugfix-123 | feature-a | feature-b | - origin/HEAD | - origin/main | v1.0.0 | v2.0.0 | + origin/HEAD | + origin/main | | | | diff --git a/src/tests/snapshots/gitu__tests__merge__merge_picker_current_branch_with_same_tag_name.snap b/src/tests/snapshots/gitu__tests__merge__merge_picker_current_branch_with_same_tag_name.snap new file mode 100644 index 0000000000..04fa15ac29 --- /dev/null +++ b/src/tests/snapshots/gitu__tests__merge__merge_picker_current_branch_with_same_tag_name.snap @@ -0,0 +1,25 @@ +--- +source: src/tests/merge.rs +expression: ctx.redact_buffer() +--- + On branch v1.0.0 | + | + Recent commits | + 4760d8f v1.0.0 v1.0.0 add branch commit | + b66a0bf main origin/main add initial-file | + | + | + | +────────────────────────────────────────────────────────────────────────────────| + 4/4 Merge β€Ί β–ˆ | +β–Œmain | + refs/tags/v1.0.0 | + origin/HEAD | + origin/main | + | + | + | + | + | + | +styles_hash: 5811f84d139176cf diff --git a/src/tests/snapshots/gitu__tests__merge__merge_picker_default_branch_with_same_tag_name.snap b/src/tests/snapshots/gitu__tests__merge__merge_picker_default_branch_with_same_tag_name.snap new file mode 100644 index 0000000000..6a4d3cfa72 --- /dev/null +++ b/src/tests/snapshots/gitu__tests__merge__merge_picker_default_branch_with_same_tag_name.snap @@ -0,0 +1,25 @@ +--- +source: src/tests/merge.rs +expression: ctx.redact_buffer() +--- + Branches | + * main | + other | + v1.0.0 | + | + Remote origin | + origin/HEAD | + origin/main | +────────────────────────────────────────────────────────────────────────────────| + 5/5 Merge β€Ί β–ˆ | +β–Œrefs/heads/v1.0.0 | + other | + refs/tags/v1.0.0 | + origin/HEAD | + origin/main | + | + | + | + | + | +styles_hash: 33dc324ad4ae7dc3 diff --git a/src/tests/snapshots/gitu__tests__merge__merge_picker_default_tag_with_same_branch_name.snap b/src/tests/snapshots/gitu__tests__merge__merge_picker_default_tag_with_same_branch_name.snap new file mode 100644 index 0000000000..126576f037 --- /dev/null +++ b/src/tests/snapshots/gitu__tests__merge__merge_picker_default_tag_with_same_branch_name.snap @@ -0,0 +1,25 @@ +--- +source: src/tests/merge.rs +expression: ctx.redact_buffer() +--- + Branches | + * main | + other | + v1.0.0 | + | + Remote origin | + origin/HEAD | + origin/main | +────────────────────────────────────────────────────────────────────────────────| + 5/5 Merge β€Ί β–ˆ | +β–Œrefs/tags/v1.0.0 | + other | + refs/heads/v1.0.0 | + origin/HEAD | + origin/main | + | + | + | + | + | +styles_hash: 7c6191b6e1887b93 diff --git a/src/tests/snapshots/gitu__tests__merge__merge_picker_duplicate_branch_tag_names.snap b/src/tests/snapshots/gitu__tests__merge__merge_picker_duplicate_branch_tag_names.snap new file mode 100644 index 0000000000..981b97af93 --- /dev/null +++ b/src/tests/snapshots/gitu__tests__merge__merge_picker_duplicate_branch_tag_names.snap @@ -0,0 +1,25 @@ +--- +source: src/tests/merge.rs +expression: ctx.redact_buffer() +--- + On branch main | + Your branch is up to date with 'origin/main'. | + | + Recent commits | + b66a0bf main origin/main add initial-file | + | + | + | +────────────────────────────────────────────────────────────────────────────────| + 5/5 Merge β€Ί β–ˆ | +β–Œother | + refs/heads/v1.0.0 | + refs/tags/v1.0.0 | + origin/HEAD | + origin/main | + | + | + | + | + | +styles_hash: 796d60861d959d60 diff --git a/src/tests/snapshots/gitu__tests__merge__merge_picker_duplicate_names_select_branch.snap b/src/tests/snapshots/gitu__tests__merge__merge_picker_duplicate_names_select_branch.snap new file mode 100644 index 0000000000..e749cd9762 --- /dev/null +++ b/src/tests/snapshots/gitu__tests__merge__merge_picker_duplicate_names_select_branch.snap @@ -0,0 +1,25 @@ +--- +source: src/tests/merge.rs +expression: ctx.redact_buffer() +--- +β–ŒOn branch main | +β–ŒYour branch is ahead of 'origin/main' by 1 commit(s). | + | + Recent commits | + 4760d8f main v1.0.0 add branch commit | + b66a0bf origin/main add initial-file | + | + | + | + | + | + | + | + | + | + | + | + | +────────────────────────────────────────────────────────────────────────────────| +$ git merge refs/heads/v1.0.0 | +styles_hash: c16a3025bad2b93c diff --git a/src/tests/snapshots/gitu__tests__merge__merge_picker_duplicate_names_select_tag.snap b/src/tests/snapshots/gitu__tests__merge__merge_picker_duplicate_names_select_tag.snap new file mode 100644 index 0000000000..4d207b761a --- /dev/null +++ b/src/tests/snapshots/gitu__tests__merge__merge_picker_duplicate_names_select_tag.snap @@ -0,0 +1,25 @@ +--- +source: src/tests/merge.rs +expression: ctx.redact_buffer() +--- +β–ŒOn branch main | +β–ŒYour branch is ahead of 'origin/main' by 1 commit(s). | + | + Recent commits | + 46369c5 main other v1.0.0 add other commit | + b66a0bf origin/main add initial-file | + | + | + | + | + | + | + | + | + | + | + | + | +────────────────────────────────────────────────────────────────────────────────| +$ git merge refs/tags/v1.0.0 | +styles_hash: 253546b91b05cb9 From 87d9bc462d358d5a87aa81b25ad3a4a8a51ee067 Mon Sep 17 00:00:00 2001 From: Seong Yong-ju Date: Mon, 19 Jan 2026 18:04:17 +0900 Subject: [PATCH 3/5] refactor: extract RefKind::from_reference() to eliminate duplicate logic Consolidate the logic for converting git2::Reference to RefKind by adding a from_reference() method. This removes duplicate code across merge.rs and show_refs.rs. --- src/item_data.rs | 13 +++++++++++++ src/ops/merge.rs | 28 +++++++--------------------- src/screen/show_refs.rs | 10 +--------- 3 files changed, 21 insertions(+), 30 deletions(-) diff --git a/src/item_data.rs b/src/item_data.rs index 8755c90469..283322c5fb 100644 --- a/src/item_data.rs +++ b/src/item_data.rs @@ -90,6 +90,19 @@ impl RefKind { RefKind::Branch(name) | RefKind::Tag(name) | RefKind::Remote(name) => name, } } + + /// Convert a git2::Reference to RefKind, returning None if the reference has no shorthand + pub(crate) fn from_reference(reference: &git2::Reference<'_>) -> Option { + let shorthand = reference.shorthand()?.to_string(); + + Some(if reference.is_branch() { + RefKind::Branch(shorthand) + } else if reference.is_tag() { + RefKind::Tag(shorthand) + } else { + RefKind::Remote(shorthand) + }) + } } #[derive(Clone, Debug)] diff --git a/src/ops/merge.rs b/src/ops/merge.rs index e7d7344a39..48f0b5112a 100644 --- a/src/ops/merge.rs +++ b/src/ops/merge.rs @@ -86,23 +86,10 @@ impl OpTrait for Merge { // Get current HEAD reference to exclude it from picker // HEAD can be a branch, tag, remote, or detached (pointing to a commit) - let current_ref: Option = app.state.repo.head().ok().and_then(|head| { - // Get the full reference name from HEAD - // For branches: "refs/heads/main", for detached: "HEAD" - head.name().and_then(|name| { - name.strip_prefix("refs/heads/") - .map(|s| RefKind::Branch(s.to_string())) - .or_else(|| { - name.strip_prefix("refs/tags/") - .map(|s| RefKind::Tag(s.to_string())) - }) - .or_else(|| { - name.strip_prefix("refs/remotes/") - .map(|s| RefKind::Remote(s.to_string())) - }) - // If none match, returns None (detached HEAD or other ref type) - }) - }); + let current_ref = { + let head = app.state.repo.head().map_err(Error::GetHead)?; + RefKind::from_reference(&head) + }; // Collect all branches (local and remote) let branches_iter = app @@ -112,16 +99,15 @@ impl OpTrait for Merge { .map_err(Error::ListGitReferences)?; for branch in branches_iter { let (branch, branch_type) = branch.map_err(Error::ListGitReferences)?; - if let Some(name) = branch.name().map_err(Error::ListGitReferences)? { - let name = name.to_string(); + let branch_ref = branch.get(); + if let Some(ref_kind) = RefKind::from_reference(branch_ref) { match branch_type { git2::BranchType::Local => { - let ref_kind = RefKind::Branch(name.clone()); + let name = ref_kind.shorthand().to_string(); *shorthand_count.entry(name).or_insert(0) += 1; branches.push(ref_kind); } git2::BranchType::Remote => { - let ref_kind = RefKind::Remote(name); remotes.push(ref_kind); } } diff --git a/src/screen/show_refs.rs b/src/screen/show_refs.rs index fc01e117ef..0ef58b1bc4 100644 --- a/src/screen/show_refs.rs +++ b/src/screen/show_refs.rs @@ -101,15 +101,7 @@ where .filter(filter) .map(move |reference| { let name = reference.name().unwrap().to_owned(); - let shorthand = reference.shorthand().unwrap().to_owned(); - - let ref_kind = if reference.is_branch() { - RefKind::Branch(shorthand) - } else if reference.is_tag() { - RefKind::Tag(shorthand) - } else { - RefKind::Remote(shorthand) - }; + let ref_kind = RefKind::from_reference(&reference).unwrap(); let prefix = create_prefix(repo, &reference); From 58a0c4ec257be547f38e9adfed4d5eed3ad26ad6 Mon Sep 17 00:00:00 2001 From: Seong Yong-ju Date: Mon, 19 Jan 2026 18:04:49 +0900 Subject: [PATCH 4/5] refactor: remove unnecessary comment --- src/ops/merge.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/ops/merge.rs b/src/ops/merge.rs index 48f0b5112a..2859f52d83 100644 --- a/src/ops/merge.rs +++ b/src/ops/merge.rs @@ -127,9 +127,6 @@ impl OpTrait for Merge { tags.push(ref_kind); } - // Note: current_ref is already counted in shorthand_count - // during branch/tag collection, so no need to count it again - // Add default ref first if it exists if let Some(ref default) = default_ref { let shorthand = default.shorthand(); From 6e5dab8afbf6c2bd63c9ebf5d10d46967f7ca3e2 Mon Sep 17 00:00:00 2001 From: Seong Yong-ju Date: Mon, 19 Jan 2026 19:44:52 +0900 Subject: [PATCH 5/5] refactor: extract PickerState::with_refs to simplify merge operation Add PickerState::with_refs method to encapsulate the complex logic of building a picker from git references. This method automatically handles duplicate detection, sorting (default -> branches -> tags -> remotes), and filtering of excluded references. Simplify merge.rs by delegating the picker construction logic to the new with_refs method, reducing code complexity from ~110 lines to ~50 lines. --- src/ops/merge.rs | 121 ++----- src/picker.rs | 296 ++++++++++++++++++ src/tests/merge.rs | 75 +---- ...ker_current_branch_with_same_tag_name.snap | 25 -- ...ker_default_branch_with_same_tag_name.snap | 25 -- ...ker_default_tag_with_same_branch_name.snap | 25 -- ...rge_picker_duplicate_branch_tag_names.snap | 25 -- 7 files changed, 332 insertions(+), 260 deletions(-) delete mode 100644 src/tests/snapshots/gitu__tests__merge__merge_picker_current_branch_with_same_tag_name.snap delete mode 100644 src/tests/snapshots/gitu__tests__merge__merge_picker_default_branch_with_same_tag_name.snap delete mode 100644 src/tests/snapshots/gitu__tests__merge__merge_picker_default_tag_with_same_branch_name.snap delete mode 100644 src/tests/snapshots/gitu__tests__merge__merge_picker_duplicate_branch_tag_names.snap diff --git a/src/ops/merge.rs b/src/ops/merge.rs index 2859f52d83..43f7608ad1 100644 --- a/src/ops/merge.rs +++ b/src/ops/merge.rs @@ -1,13 +1,15 @@ use super::{Action, OpTrait}; +use crate::item_data::RefKind; use crate::{ Res, app::{App, State}, error::Error, item_data::ItemData, menu::arg::Arg, - picker::{PickerData, PickerItem, PickerState}, + picker::PickerState, term::Term, }; + use std::{process::Command, rc::Rc}; pub(crate) fn init_args() -> Vec { @@ -67,8 +69,6 @@ fn merge(app: &mut App, term: &mut Term, rev: &str) -> Res<()> { pub(crate) struct Merge; impl OpTrait for Merge { fn get_action(&self, target: &ItemData) -> Option { - use crate::item_data::RefKind; - // Extract default ref from target if it's a Reference let default_ref = if let ItemData::Reference { kind, .. } = target { Some(kind.clone()) @@ -77,119 +77,40 @@ impl OpTrait for Merge { }; Some(Rc::new(move |app: &mut App, term: &mut Term| { - let mut items = Vec::new(); - let mut shorthand_count: std::collections::HashMap = - std::collections::HashMap::new(); - let mut branches: Vec = Vec::new(); - let mut tags: Vec = Vec::new(); - let mut remotes: Vec = Vec::new(); - // Get current HEAD reference to exclude it from picker - // HEAD can be a branch, tag, remote, or detached (pointing to a commit) - let current_ref = { + let exclude_ref = { let head = app.state.repo.head().map_err(Error::GetHead)?; RefKind::from_reference(&head) }; // Collect all branches (local and remote) - let branches_iter = app + let branches = app .state .repo .branches(None) - .map_err(Error::ListGitReferences)?; - for branch in branches_iter { - let (branch, branch_type) = branch.map_err(Error::ListGitReferences)?; - let branch_ref = branch.get(); - if let Some(ref_kind) = RefKind::from_reference(branch_ref) { - match branch_type { - git2::BranchType::Local => { - let name = ref_kind.shorthand().to_string(); - *shorthand_count.entry(name).or_insert(0) += 1; - branches.push(ref_kind); - } - git2::BranchType::Remote => { - remotes.push(ref_kind); - } - } - } - } - - // Collect all tags (count for duplicate detection) - let tag_names = app + .map_err(Error::ListGitReferences)? + .filter_map(|branch| { + let (branch, _) = branch.ok()?; + RefKind::from_reference(branch.get()) + }); + + // Collect all tags + let tags: Vec = app .state .repo .tag_names(None) - .map_err(Error::ListGitReferences)?; - for tag_name in tag_names.iter().flatten() { - let tag_name = tag_name.to_string(); - let ref_kind = RefKind::Tag(tag_name.clone()); - *shorthand_count.entry(tag_name).or_insert(0) += 1; - tags.push(ref_kind); - } - - // Add default ref first if it exists - if let Some(ref default) = default_ref { - let shorthand = default.shorthand(); - let (display, refname) = match default { - RefKind::Remote(_) => { - // Remotes never have duplicates - (shorthand.to_string(), shorthand.to_string()) - } - _ => { - let is_duplicate = shorthand_count.get(shorthand).is_some_and(|&c| c > 1); - if is_duplicate { - let full_refname = default.to_full_refname(); - (full_refname.clone(), full_refname) - } else { - (shorthand.to_string(), shorthand.to_string()) - } - } - }; - items.push(PickerItem::new(display, PickerData::Revision(refname))); - } + .map_err(Error::ListGitReferences)? + .into_iter() + .flatten() + .map(|tag_name| RefKind::Tag(tag_name.to_string())) + .collect(); - // Add all refs (branches, then tags, then remotes) - for ref_kind in branches.into_iter().chain(tags).chain(remotes) { - let shorthand = ref_kind.shorthand(); - - // Skip current ref (HEAD) - could be branch, tag, or remote - if current_ref - .as_ref() - .is_some_and(|current| current.to_full_refname() == ref_kind.to_full_refname()) - { - continue; - } - - // Skip if it's the same as default (compare by full refname to distinguish branch/tag) - if default_ref - .as_ref() - .is_some_and(|d| d.to_full_refname() == ref_kind.to_full_refname()) - { - continue; - } - - // Handle duplicates (only for branches and tags) - let (display, refname) = match &ref_kind { - RefKind::Remote(_) => { - // Remotes never have duplicates (they have remote/ prefix) - (shorthand.to_string(), shorthand.to_string()) - } - _ => { - let is_duplicate = shorthand_count.get(shorthand).is_some_and(|&c| c > 1); - if is_duplicate { - let full_refname = ref_kind.to_full_refname(); - (full_refname.clone(), full_refname) - } else { - (shorthand.to_string(), shorthand.to_string()) - } - } - }; - items.push(PickerItem::new(display, PickerData::Revision(refname))); - } + let all_refs: Vec = branches.chain(tags).collect(); // Allow custom input to support commit hashes, relative refs (e.g., HEAD~3), // and other git revisions not in the predefined list - let picker = PickerState::new("Merge", items, true); + let picker = + PickerState::with_refs("Merge", all_refs, exclude_ref, default_ref.clone(), true); let result = app.picker(term, picker)?; if let Some(data) = result { diff --git a/src/picker.rs b/src/picker.rs index ec70302ffa..55ef715ef5 100644 --- a/src/picker.rs +++ b/src/picker.rs @@ -1,9 +1,12 @@ use fuzzy_matcher::FuzzyMatcher; use fuzzy_matcher::skim::SkimMatcherV2; use std::borrow::Cow; +use std::collections::HashMap; use tui_prompts::State as _; use tui_prompts::TextState; +use crate::item_data::RefKind; + /// Data that can be selected in a picker #[derive(Debug, Clone, PartialEq)] pub enum PickerData { @@ -106,6 +109,101 @@ impl PickerState { state } + /// Create a picker from RefKinds, automatically handling duplicates and sorting + /// + /// Items are sorted as: default (if provided) -> branches -> tags -> remotes + /// Duplicate shorthands are displayed with their full refname + /// The exclude_ref is excluded from the picker options + pub(crate) fn with_refs( + prompt: impl Into>, + refs: Vec, + exclude_ref: Option, + default: Option, + allow_custom_input: bool, + ) -> Self { + let mut items = Vec::new(); + let mut shorthand_count: HashMap = HashMap::new(); + let mut branches = Vec::new(); + let mut tags = Vec::new(); + let mut remotes = Vec::new(); + + // Count shorthands for duplicate detection (before filtering) + for ref_kind in &refs { + let shorthand = ref_kind.shorthand(); + match ref_kind { + RefKind::Branch(_) | RefKind::Tag(_) => { + *shorthand_count.entry(shorthand.to_string()).or_insert(0) += 1; + } + RefKind::Remote(_) => { + // Remotes have unique names (include remote/ prefix) + } + } + } + + // Categorize and filter refs + for ref_kind in refs { + // Skip excluded ref + if exclude_ref + .as_ref() + .is_some_and(|excluded| excluded.to_full_refname() == ref_kind.to_full_refname()) + { + continue; + } + + match ref_kind { + RefKind::Branch(_) => branches.push(ref_kind), + RefKind::Tag(_) => tags.push(ref_kind), + RefKind::Remote(_) => remotes.push(ref_kind), + } + } + + // Add default ref first if it exists + if let Some(ref default) = default { + let shorthand = default.shorthand(); + let (display, refname) = match default { + RefKind::Remote(_) => (shorthand.to_string(), shorthand.to_string()), + _ => { + let is_duplicate = shorthand_count.get(shorthand).is_some_and(|&c| c > 1); + if is_duplicate { + let full_refname = default.to_full_refname(); + (full_refname.clone(), full_refname) + } else { + (shorthand.to_string(), shorthand.to_string()) + } + } + }; + items.push(PickerItem::new(display, PickerData::Revision(refname))); + } + + // Add all refs (branches, then tags, then remotes) + for ref_kind in branches.into_iter().chain(tags).chain(remotes) { + // Skip if it's the same as default + if default + .as_ref() + .is_some_and(|d| d.to_full_refname() == ref_kind.to_full_refname()) + { + continue; + } + + let shorthand = ref_kind.shorthand(); + let (display, refname) = match &ref_kind { + RefKind::Remote(_) => (shorthand.to_string(), shorthand.to_string()), + _ => { + let is_duplicate = shorthand_count.get(shorthand).is_some_and(|&c| c > 1); + if is_duplicate { + let full_refname = ref_kind.to_full_refname(); + (full_refname.clone(), full_refname) + } else { + (shorthand.to_string(), shorthand.to_string()) + } + } + }; + items.push(PickerItem::new(display, PickerData::Revision(refname))); + } + + Self::new(prompt, items, allow_custom_input) + } + /// Get current input pattern pub fn pattern(&self) -> &str { self.input_state.value() @@ -1002,4 +1100,202 @@ mod tests { state.previous(); assert_eq!(state.cursor(), 0); // Wraps to same item } + + // Tests for with_refs method + #[test] + fn test_with_refs_basic_sorting() { + // Test that items are sorted as: default -> branches -> tags -> remotes + let refs = vec![ + RefKind::Tag("v1.0.0".to_string()), + RefKind::Remote("origin/main".to_string()), + RefKind::Branch("feature".to_string()), + RefKind::Branch("main".to_string()), + RefKind::Tag("v2.0.0".to_string()), + ]; + + let state = PickerState::with_refs("Select", refs, None, None, false); + + let items: Vec<_> = state + .filtered_items() + .map(|(_, item)| item.display.as_ref()) + .collect(); + + // Should be sorted: branches first, then tags, then remotes + assert_eq!(items[0], "feature"); + assert_eq!(items[1], "main"); + assert_eq!(items[2], "v1.0.0"); + assert_eq!(items[3], "v2.0.0"); + assert_eq!(items[4], "origin/main"); + } + + #[test] + fn test_with_refs_empty_list() { + let state = PickerState::with_refs("Select", vec![], None, None, false); + + assert_eq!(state.total_items(), 0); + assert_eq!(state.filtered_count(), 0); + assert!(state.selected().is_none()); + } + + #[test] + fn test_with_refs_with_default() { + // Test that default ref is shown first + let refs = vec![ + RefKind::Branch("feature".to_string()), + RefKind::Branch("main".to_string()), + RefKind::Tag("v1.0.0".to_string()), + ]; + + let default = Some(RefKind::Branch("main".to_string())); + let state = PickerState::with_refs("Select", refs, None, default, false); + + let items: Vec<_> = state + .filtered_items() + .map(|(_, item)| item.display.as_ref()) + .collect(); + + // Default should be first, then remaining branches, then tags + assert_eq!(items[0], "main"); // default first + assert_eq!(items[1], "feature"); + assert_eq!(items[2], "v1.0.0"); + } + + #[test] + fn test_with_refs_exclude_ref() { + // Test that excluded ref is not shown + let refs = vec![ + RefKind::Branch("feature".to_string()), + RefKind::Branch("main".to_string()), + RefKind::Tag("v1.0.0".to_string()), + ]; + + let exclude = Some(RefKind::Branch("main".to_string())); + let state = PickerState::with_refs("Select", refs, exclude, None, false); + + let items: Vec<_> = state + .filtered_items() + .map(|(_, item)| item.display.as_ref()) + .collect(); + + // main should be excluded + assert_eq!(items.len(), 2); + assert_eq!(items[0], "feature"); + assert_eq!(items[1], "v1.0.0"); + } + + #[test] + fn test_with_refs_duplicate_names() { + // Test that duplicates show full refname + let refs = vec![ + RefKind::Branch("v1.0.0".to_string()), + RefKind::Tag("v1.0.0".to_string()), + RefKind::Branch("main".to_string()), + ]; + + let state = PickerState::with_refs("Select", refs, None, None, false); + + let items: Vec<_> = state + .filtered_items() + .map(|(_, item)| item.display.as_ref()) + .collect(); + + // Order is preserved from input: branches first (in order), then tags + // Duplicates should show full refname + assert_eq!(items[0], "refs/heads/v1.0.0"); // duplicate - full refname + assert_eq!(items[1], "main"); // no duplicate + assert_eq!(items[2], "refs/tags/v1.0.0"); // duplicate - full refname + } + + #[test] + fn test_with_refs_duplicate_with_default() { + // Test duplicates with default ref + let refs = vec![ + RefKind::Branch("v1.0.0".to_string()), + RefKind::Tag("v1.0.0".to_string()), + RefKind::Branch("main".to_string()), + ]; + + let default = Some(RefKind::Tag("v1.0.0".to_string())); + let state = PickerState::with_refs("Select", refs, None, default, false); + + let items: Vec<_> = state + .filtered_items() + .map(|(_, item)| item.display.as_ref()) + .collect(); + + // Default (tag) should be first with full refname, then other items in order + assert_eq!(items[0], "refs/tags/v1.0.0"); // default first - duplicate name + assert_eq!(items[1], "refs/heads/v1.0.0"); // duplicate - full refname + assert_eq!(items[2], "main"); // no duplicate + } + + #[test] + fn test_with_refs_exclude_and_default_same() { + // Test when exclude and default are the same + // The implementation adds default first, then filters exclude from remaining items + // So if default == exclude, default is still added, then exclude filters it from the rest + let refs = vec![ + RefKind::Branch("feature".to_string()), + RefKind::Branch("main".to_string()), + ]; + + let main_ref = RefKind::Branch("main".to_string()); + let state = PickerState::with_refs( + "Select", + refs, + Some(main_ref.clone()), + Some(main_ref), + false, + ); + + let items: Vec<_> = state + .filtered_items() + .map(|(_, item)| item.display.as_ref()) + .collect(); + + // Default is added first, then exclude filters remaining items + // So we get: main (default) + feature (not excluded) + assert_eq!(items.len(), 2); + assert_eq!(items[0], "main"); // default + assert_eq!(items[1], "feature"); + } + + #[test] + fn test_with_refs_with_custom_input() { + let refs = vec![ + RefKind::Branch("main".to_string()), + RefKind::Tag("v1.0.0".to_string()), + ]; + + let state = PickerState::with_refs("Select", refs, None, None, true); + + // Custom input should be allowed + assert!(state.allow_custom_input); + + // With empty pattern, no custom input item + assert!(state.custom_input_item.is_none()); + } + + #[test] + fn test_with_refs_duplicate_detection_before_exclusion() { + // Test that duplicate detection happens before exclusion + // If we have branch "v1.0.0" and tag "v1.0.0", and we exclude the branch, + // the tag should still show full refname because the duplicate existed + let refs = vec![ + RefKind::Branch("v1.0.0".to_string()), + RefKind::Tag("v1.0.0".to_string()), + ]; + + let exclude = Some(RefKind::Branch("v1.0.0".to_string())); + let state = PickerState::with_refs("Select", refs, exclude, None, false); + + let items: Vec<_> = state + .filtered_items() + .map(|(_, item)| item.display.as_ref()) + .collect(); + + // Only tag remains, but should show full refname because duplicate existed + assert_eq!(items.len(), 1); + assert_eq!(items[0], "refs/tags/v1.0.0"); + } } diff --git a/src/tests/merge.rs b/src/tests/merge.rs index af401a3620..ec6e4e9206 100644 --- a/src/tests/merge.rs +++ b/src/tests/merge.rs @@ -47,18 +47,6 @@ fn setup_branch_tag_same_name(ctx: TestContext) -> TestContext { ctx } -fn setup_on_branch_with_same_tag(ctx: TestContext) -> TestContext { - // Create and checkout a branch named v1.0.0 - run(&ctx.dir, &["git", "checkout", "-b", "v1.0.0"]); - commit(&ctx.dir, "branch commit", ""); - - // Create a tag also named v1.0.0 pointing to current commit - run(&ctx.dir, &["git", "tag", "v1.0.0"]); - - // Stay on the v1.0.0 branch - ctx -} - #[test] fn merge_menu() { snapshot!(setup_branch(setup_clone!()), "m"); @@ -70,10 +58,21 @@ fn merge_picker() { } #[test] -fn merge_picker_duplicate_branch_tag_names() { - // Test that when branch and tag have the same name, - // picker displays them with prefixes (heads/v1.0.0, tags/v1.0.0) - snapshot!(setup_branch_tag_same_name(setup_clone!()), "mm"); +fn merge_picker_custom_input() { + snapshot!(setup_branches(setup_clone!()), "mmHEAD~2"); +} + +#[test] +fn merge_picker_cancel() { + snapshot!(setup_branches(setup_clone!()), "mm"); +} + +#[test] +fn merge_select_from_list() { + // Select feature-a branch from the list and merge it + with_var("GIT_MERGE_AUTOEDIT", Some("no"), || { + snapshot!(setup_branches(setup_clone!()), "mmfeature-a"); + }); } #[test] @@ -98,50 +97,6 @@ fn merge_picker_duplicate_names_select_tag() { }); } -#[test] -fn merge_picker_current_branch_with_same_tag_name() { - // Test that when current branch and tag have the same name, - // current branch is excluded from picker, only tag is shown - snapshot!(setup_on_branch_with_same_tag(setup_clone!()), "mm"); -} - -#[test] -fn merge_picker_default_tag_with_same_branch_name() { - // Test that when selecting a tag, and a branch with same name exists, - // both are shown but default (tag) is not duplicated - // Navigate to tags view and select v1.0.0 tag, then open merge picker - snapshot!( - setup_branch_tag_same_name(setup_clone!()), - "Yjjjjjjjjmm" - ); -} - -#[test] -fn merge_picker_default_branch_with_same_tag_name() { - // Test that when selecting a branch, and a tag with same name exists, - // both are shown but default (branch) is not duplicated - // Navigate to branches view and select v1.0.0 branch, then open merge picker - snapshot!(setup_branch_tag_same_name(setup_clone!()), "Yjjjmm"); -} - -#[test] -fn merge_picker_custom_input() { - snapshot!(setup_branches(setup_clone!()), "mmHEAD~2"); -} - -#[test] -fn merge_picker_cancel() { - snapshot!(setup_branches(setup_clone!()), "mm"); -} - -#[test] -fn merge_select_from_list() { - // Select feature-a branch from the list and merge it - with_var("GIT_MERGE_AUTOEDIT", Some("no"), || { - snapshot!(setup_branches(setup_clone!()), "mmfeature-a"); - }); -} - #[test] fn merge_use_custom_input() { // Use custom input with full commit hash diff --git a/src/tests/snapshots/gitu__tests__merge__merge_picker_current_branch_with_same_tag_name.snap b/src/tests/snapshots/gitu__tests__merge__merge_picker_current_branch_with_same_tag_name.snap deleted file mode 100644 index 04fa15ac29..0000000000 --- a/src/tests/snapshots/gitu__tests__merge__merge_picker_current_branch_with_same_tag_name.snap +++ /dev/null @@ -1,25 +0,0 @@ ---- -source: src/tests/merge.rs -expression: ctx.redact_buffer() ---- - On branch v1.0.0 | - | - Recent commits | - 4760d8f v1.0.0 v1.0.0 add branch commit | - b66a0bf main origin/main add initial-file | - | - | - | -────────────────────────────────────────────────────────────────────────────────| - 4/4 Merge β€Ί β–ˆ | -β–Œmain | - refs/tags/v1.0.0 | - origin/HEAD | - origin/main | - | - | - | - | - | - | -styles_hash: 5811f84d139176cf diff --git a/src/tests/snapshots/gitu__tests__merge__merge_picker_default_branch_with_same_tag_name.snap b/src/tests/snapshots/gitu__tests__merge__merge_picker_default_branch_with_same_tag_name.snap deleted file mode 100644 index 6a4d3cfa72..0000000000 --- a/src/tests/snapshots/gitu__tests__merge__merge_picker_default_branch_with_same_tag_name.snap +++ /dev/null @@ -1,25 +0,0 @@ ---- -source: src/tests/merge.rs -expression: ctx.redact_buffer() ---- - Branches | - * main | - other | - v1.0.0 | - | - Remote origin | - origin/HEAD | - origin/main | -────────────────────────────────────────────────────────────────────────────────| - 5/5 Merge β€Ί β–ˆ | -β–Œrefs/heads/v1.0.0 | - other | - refs/tags/v1.0.0 | - origin/HEAD | - origin/main | - | - | - | - | - | -styles_hash: 33dc324ad4ae7dc3 diff --git a/src/tests/snapshots/gitu__tests__merge__merge_picker_default_tag_with_same_branch_name.snap b/src/tests/snapshots/gitu__tests__merge__merge_picker_default_tag_with_same_branch_name.snap deleted file mode 100644 index 126576f037..0000000000 --- a/src/tests/snapshots/gitu__tests__merge__merge_picker_default_tag_with_same_branch_name.snap +++ /dev/null @@ -1,25 +0,0 @@ ---- -source: src/tests/merge.rs -expression: ctx.redact_buffer() ---- - Branches | - * main | - other | - v1.0.0 | - | - Remote origin | - origin/HEAD | - origin/main | -────────────────────────────────────────────────────────────────────────────────| - 5/5 Merge β€Ί β–ˆ | -β–Œrefs/tags/v1.0.0 | - other | - refs/heads/v1.0.0 | - origin/HEAD | - origin/main | - | - | - | - | - | -styles_hash: 7c6191b6e1887b93 diff --git a/src/tests/snapshots/gitu__tests__merge__merge_picker_duplicate_branch_tag_names.snap b/src/tests/snapshots/gitu__tests__merge__merge_picker_duplicate_branch_tag_names.snap deleted file mode 100644 index 981b97af93..0000000000 --- a/src/tests/snapshots/gitu__tests__merge__merge_picker_duplicate_branch_tag_names.snap +++ /dev/null @@ -1,25 +0,0 @@ ---- -source: src/tests/merge.rs -expression: ctx.redact_buffer() ---- - On branch main | - Your branch is up to date with 'origin/main'. | - | - Recent commits | - b66a0bf main origin/main add initial-file | - | - | - | -────────────────────────────────────────────────────────────────────────────────| - 5/5 Merge β€Ί β–ˆ | -β–Œother | - refs/heads/v1.0.0 | - refs/tags/v1.0.0 | - origin/HEAD | - origin/main | - | - | - | - | - | -styles_hash: 796d60861d959d60