Skip to content

Commit 56f88ef

Browse files
id: cleanup parse_str branch handling
In `parse_str`, despite the comment near `self.find_branches_by_name`, `self.find_branches_by_name` actually handles partial branch name lookup already, which means that the other branch searches are redundant. Remove them, which also allows a lot of other code cleanup. In particular, `Context` is no longer needed when parsing strings - only when generating the `IdMap`. This makes it clear that the only inputs are the `RefInfo` and the hunk assignments.
1 parent e6ff67e commit 56f88ef

File tree

11 files changed

+30
-82
lines changed

11 files changed

+30
-82
lines changed

crates/but/src/command/legacy/branch/mod.rs

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,10 +88,9 @@ pub async fn handle(
8888

8989
let anchor = if let Some(anchor_str) = anchor {
9090
// Use the new create_reference API when anchor is provided
91-
let mut ctx = ctx; // Make mutable for CliId resolution
9291

9392
// Resolve the anchor string to a CliId
94-
let anchor_ids = id_map.parse_str(&mut ctx, &anchor_str)?;
93+
let anchor_ids = id_map.parse_str(&anchor_str)?;
9594
if anchor_ids.is_empty() {
9695
return Err(anyhow::anyhow!("Could not find anchor: {}", anchor_str));
9796
}

crates/but/src/command/legacy/commit.rs

Lines changed: 6 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ pub(crate) fn insert_blank_commit(
2727
let id_map = IdMap::new(&mut ctx)?;
2828

2929
// Resolve the target ID
30-
let cli_ids = id_map.parse_str(&mut ctx, target)?;
30+
let cli_ids = id_map.parse_str(target)?;
3131

3232
if cli_ids.is_empty() {
3333
bail!("Target '{}' not found", target);
@@ -173,15 +173,8 @@ pub(crate) fn commit(
173173
})
174174
.collect();
175175

176-
let (target_stack_id, target_stack) = select_stack(
177-
&mut ctx,
178-
&id_map,
179-
project,
180-
&stacks,
181-
branch_hint,
182-
create_branch,
183-
out,
184-
)?;
176+
let (target_stack_id, target_stack) =
177+
select_stack(&id_map, project, &stacks, branch_hint, create_branch, out)?;
185178

186179
// Get changes and assignments using but-api
187180
let worktree_changes = diff::changes_in_worktree(project_id)?;
@@ -248,7 +241,7 @@ pub(crate) fn commit(
248241
.find(|branch| branch.name == hint)
249242
.or_else(|| {
250243
// If no exact match, try to parse as CLI ID and match
251-
if let Ok(cli_ids) = id_map.parse_str(&mut ctx, hint) {
244+
if let Ok(cli_ids) = id_map.parse_str(hint) {
252245
for cli_id in cli_ids {
253246
if let crate::legacy::id::CliId::Branch { name, .. } = cli_id
254247
&& let Some(branch) =
@@ -347,7 +340,6 @@ fn create_independent_branch(
347340
}
348341

349342
fn select_stack(
350-
ctx: &mut Context,
351343
id_map: &IdMap,
352344
project: &Project,
353345
stacks: &[(
@@ -377,7 +369,7 @@ fn select_stack(
377369
match branch_hint {
378370
Some(hint) => {
379371
// Try to find stack by branch hint
380-
if let Some(stack) = find_stack_by_hint(ctx, id_map, stacks, hint) {
372+
if let Some(stack) = find_stack_by_hint(id_map, stacks, hint) {
381373
return Ok(stack);
382374
}
383375

@@ -409,7 +401,6 @@ fn select_stack(
409401
}
410402

411403
fn find_stack_by_hint(
412-
ctx: &mut Context,
413404
id_map: &IdMap,
414405
stacks: &[(
415406
but_core::ref_metadata::StackId,
@@ -428,7 +419,7 @@ fn find_stack_by_hint(
428419
}
429420

430421
// Try CLI ID parsing
431-
let cli_ids = id_map.parse_str(ctx, hint).ok()?;
422+
let cli_ids = id_map.parse_str(hint).ok()?;
432423
for cli_id in cli_ids {
433424
if let CliId::Branch { name, .. } = cli_id {
434425
for (stack_id, stack_details) in stacks {

crates/but/src/command/legacy/describe.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ pub(crate) fn describe_target(
2121
let id_map = IdMap::new(&mut ctx)?;
2222

2323
// Resolve the commit ID
24-
let cli_ids = id_map.parse_str(&mut ctx, target)?;
24+
let cli_ids = id_map.parse_str(target)?;
2525

2626
if cli_ids.is_empty() {
2727
bail!("ID '{}' not found", target);

crates/but/src/command/legacy/forge/review.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ fn get_branch_names(project: &Project, branch_id: &str) -> anyhow::Result<Vec<St
100100
let mut ctx = Context::new_from_legacy_project(project.clone())?;
101101
let id_map = IdMap::new(&mut ctx)?;
102102
let branch_ids = id_map
103-
.parse_str(&mut ctx, branch_id)?
103+
.parse_str(branch_id)?
104104
.iter()
105105
.filter_map(|clid| match clid {
106106
CliId::Branch { name, .. } => Some(name.clone()),

crates/but/src/command/legacy/mark.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ pub(crate) fn handle(
1919
) -> anyhow::Result<()> {
2020
let ctx = &mut Context::new_from_legacy_project(project.clone())?;
2121
let id_map = IdMap::new(ctx)?;
22-
let target_result = id_map.parse_str(ctx, target_str)?;
22+
let target_result = id_map.parse_str(target_str)?;
2323
if target_result.len() != 1 {
2424
return Err(anyhow::anyhow!(
2525
"Target {} is ambiguous: {:?}",

crates/but/src/command/legacy/push.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ fn resolve_branch_name(
118118
branch_id: &str,
119119
) -> anyhow::Result<String> {
120120
// Try to resolve as CliId first
121-
let cli_ids = id_map.parse_str(ctx, branch_id)?;
121+
let cli_ids = id_map.parse_str(branch_id)?;
122122

123123
if cli_ids.is_empty() {
124124
// If no CliId matches, treat as literal branch name but validate it exists

crates/but/src/command/legacy/rub/mod.rs

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,7 @@ fn ids(
168168
target: &str,
169169
) -> anyhow::Result<(Vec<CliId>, CliId)> {
170170
let sources = parse_sources(ctx, id_map, source)?;
171-
let target_result = id_map.parse_str(ctx, target)?;
171+
let target_result = id_map.parse_str(target)?;
172172
if target_result.len() != 1 {
173173
if target_result.is_empty() {
174174
return Err(anyhow::anyhow!(
@@ -207,11 +207,11 @@ pub(crate) fn parse_sources(
207207
}
208208
// Check if it's a list (contains ',')
209209
else if source.contains(',') {
210-
parse_list(ctx, id_map, source)
210+
parse_list(id_map, source)
211211
}
212212
// Single source
213213
else {
214-
let source_result = id_map.parse_str(ctx, source)?;
214+
let source_result = id_map.parse_str(source)?;
215215
if source_result.len() != 1 {
216216
if source_result.is_empty() {
217217
return Err(anyhow::anyhow!(
@@ -253,8 +253,8 @@ fn parse_range(ctx: &mut Context, id_map: &IdMap, source: &str) -> anyhow::Resul
253253
let end_str = parts[1];
254254

255255
// Get the start and end IDs
256-
let start_matches = id_map.parse_str(ctx, start_str)?;
257-
let end_matches = id_map.parse_str(ctx, end_str)?;
256+
let start_matches = id_map.parse_str(start_str)?;
257+
let end_matches = id_map.parse_str(end_str)?;
258258

259259
if start_matches.len() != 1 {
260260
return Err(anyhow::anyhow!(
@@ -360,13 +360,13 @@ fn get_all_files_in_display_order(ctx: &mut Context, id_map: &IdMap) -> anyhow::
360360
Ok(all_files)
361361
}
362362

363-
fn parse_list(ctx: &mut Context, id_map: &IdMap, source: &str) -> anyhow::Result<Vec<CliId>> {
363+
fn parse_list(id_map: &IdMap, source: &str) -> anyhow::Result<Vec<CliId>> {
364364
let parts: Vec<&str> = source.split(',').collect();
365365
let mut result = Vec::new();
366366

367367
for part in parts {
368368
let part = part.trim();
369-
let matches = id_map.parse_str(ctx, part)?;
369+
let matches = id_map.parse_str(part)?;
370370
if matches.len() != 1 {
371371
if matches.is_empty() {
372372
return Err(anyhow::anyhow!(

crates/but/src/command/legacy/status/assignment.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,11 @@ pub(crate) struct FileAssignment {
3030
}
3131

3232
impl FileAssignment {
33-
pub fn from_assignments(id_map: &IdMap, path: &BString, assignments: &[HunkAssignment]) -> Self {
33+
pub fn from_assignments(
34+
id_map: &IdMap,
35+
path: &BString,
36+
assignments: &[HunkAssignment],
37+
) -> Self {
3438
let mut filtered_assignments = Vec::new();
3539
for assignment in assignments {
3640
if assignment.path_bytes == *path {

crates/but/src/command/legacy/status/mod.rs

Lines changed: 1 addition & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,7 @@ const DATE_ONLY: CustomFormat = CustomFormat::new("%Y-%m-%d");
1818
pub(crate) mod assignment;
1919
pub(crate) mod json;
2020

21-
use crate::{
22-
legacy::id::{CliId, IdMap},
23-
utils::OutputChannel,
24-
};
21+
use crate::{legacy::id::IdMap, utils::OutputChannel};
2522

2623
type StackDetail = (Option<StackDetails>, Vec<FileAssignment>);
2724
type StackEntry = (Option<gitbutler_stack::StackId>, StackDetail);
@@ -560,18 +557,6 @@ fn path_with_color(status: &TreeStatus, path: String) -> ColoredString {
560557
}
561558
}
562559

563-
pub(crate) fn all_branches(ctx: &mut Context) -> anyhow::Result<Vec<CliId>> {
564-
let mut id_map = IdMap::new(ctx)?;
565-
let stacks = crate::legacy::commits::stacks(ctx)?;
566-
let mut branches = Vec::new();
567-
for stack in stacks {
568-
for head in stack.heads {
569-
branches.push(id_map.branch(head.name.as_ref()).clone());
570-
}
571-
}
572-
Ok(branches)
573-
}
574-
575560
fn status_from_changes(changes: &[ui::TreeChange], path: BString) -> Option<ui::TreeStatus> {
576561
changes.iter().find_map(|change| {
577562
if change.path_bytes == path {

crates/but/src/legacy/id/mod.rs

Lines changed: 5 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@ impl IdMap {
224224

225225
/// Cli ID generation
226226
impl IdMap {
227-
pub fn parse_str(&self, ctx: &mut Context, s: &str) -> anyhow::Result<Vec<CliId>> {
227+
pub fn parse_str(&self, s: &str) -> anyhow::Result<Vec<CliId>> {
228228
if s.len() < 2 {
229229
return Err(anyhow::anyhow!(
230230
"Id needs to be at least 2 characters long: {}",
@@ -234,7 +234,7 @@ impl IdMap {
234234

235235
let mut matches = Vec::new();
236236

237-
// First, try exact branch name match
237+
// First, try partial branch name match
238238
if let Ok(branch_matches) = self.find_branches_by_name(s.into()) {
239239
matches.extend(branch_matches);
240240
}
@@ -250,17 +250,8 @@ impl IdMap {
250250
}
251251
}
252252

253-
// Then try CliId matching (both prefix and exact)
254-
if s.len() > 2 {
255-
// For longer strings, try prefix matching on CliIds
256-
let mut cli_matches = Vec::new();
257-
crate::command::legacy::status::all_branches(ctx)?
258-
.into_iter()
259-
.filter(|id| id.matches_prefix(s))
260-
.for_each(|id| cli_matches.push(id));
261-
matches.extend(cli_matches);
262-
} else {
263-
// For 2-character strings, try exact CliId matching
253+
// Then try CliId matching
254+
if s.len() == 2 {
264255
let mut cli_matches = Vec::new();
265256
if let Some(UncommittedFile {
266257
assignment_path: (assignment, path),
@@ -284,13 +275,9 @@ impl IdMap {
284275
id: s.to_string(),
285276
});
286277
}
287-
crate::command::legacy::status::all_branches(ctx)?
288-
.into_iter()
289-
.filter(|id| id.matches(s))
290-
.for_each(|id| cli_matches.push(id));
291278
matches.extend(cli_matches);
292279
}
293-
if self.unassigned().matches(s) {
280+
if s.find(|c: char| c != '0').is_none() {
294281
matches.push(self.unassigned().clone());
295282
}
296283

@@ -410,24 +397,6 @@ impl CliId {
410397
pub fn commit(oid: gix::ObjectId) -> Self {
411398
CliId::Commit { oid }
412399
}
413-
414-
pub fn matches(&self, s: &str) -> bool {
415-
match self {
416-
CliId::Unassigned { .. } => s.find(|c: char| c != '0').is_none(),
417-
_ => s == self.to_string(),
418-
}
419-
}
420-
421-
pub fn matches_prefix(&self, s: &str) -> bool {
422-
match self {
423-
CliId::Commit { oid } => {
424-
let oid_hash = hash(&oid.to_string());
425-
oid_hash.starts_with(s)
426-
}
427-
CliId::Unassigned { .. } => s.find(|c: char| c != '0').is_none(),
428-
_ => self.to_string().starts_with(s),
429-
}
430-
}
431400
}
432401

433402
impl Display for CliId {

0 commit comments

Comments
 (0)