feat(branch): implement safe delete with -d flag (#168) #170

Nook001 · 2026-01-26T18:02:59Z

Issue

Closes #168

Summary

实现 branch -d 选项，用于安全删除已合并的分支。branch -d 会在删除前检查分支是否已完全合并到 HEAD，防止意外丢失未合并的代码。

Changes

✅ 在 BranchArgs 中使用 delete_safe 字段对应 -d 参数
✅ 实现 delete_branch_safe() 函数，包含完整的合并检查逻辑
✅ 使用 log::get_reachable_commits() 遍历提交图，判断分支是否已合并
✅ 添加集成测试 test_branch_delete_safe，覆盖已合并和未合并两种场景
✅ 更新 README.md 兼容性表格

Implementation Details

合并检查算法

// 1. 获取 HEAD 的所有可达提交
let head_reachable = get_reachable_commits(head_commit).await;
// 2. 获取待删除分支的所有可达提交
let branch_reachable = get_reachable_commits(branch.commit).await;
// 3. 检查分支的每个提交是否都在 HEAD 的历史中
for commit in branch_reachable {
if !head_commit_ids.contains(&commit.id) {
// 未合并，拒绝删除
return;
}
}
// 4. 所有检查通过，执行删除

与 Git 行为对照

命令	场景	Git	Libra	结果
`branch -d`	已合并	✅ 删除	✅ 删除	一致
`branch -d`	未合并	❌ 拒绝	❌ 拒绝	一致
`branch -D`	任意	✅ 强制删除	✅ 强制删除	一致

Testing

运行所有 branch 相关测试：

cargo test --test command_test branch_test -- --test-threads=1
结果：
✅ 8/8 测试通过
✅ 新增测试 test_branch_delete_safe 验证了两个场景：
拒绝删除未合并分支
成功删除已合并分支

Add branch -d option to safely delete branches by checking if they are fully merged into HEAD before deletion, preventing accidental loss of unmerged work. Closes web3infra-foundation#168 Signed-off-by: yuanzhiliu <[email protected]> Signed-off-by: Nook001 <[email protected]>

Signed-off-by: Nook001 <[email protected]>

genedna · 2026-01-27T02:17:03Z

@codex review

genedna · 2026-01-27T02:17:12Z

@claude

github-actions · 2026-01-27T02:17:28Z

Claude finished @genedna's task in 2m 35s —— View job

Code Review Complete ✅

I've reviewed the implementation of branch -d (safe delete) functionality. Overall, this is a well-implemented feature with proper merge checking and good test coverage. Here are my detailed findings:

✅ Strengths

Correct Implementation Approach: The merge checking algorithm correctly uses BFS traversal via get_reachable_commits() to determine if a branch is fully merged.
Good Test Coverage: The test test_branch_delete_safe properly covers both scenarios (refusing unmerged branches and allowing merged ones).
User-Friendly Error Messages: The implementation provides helpful error messages that match Git's behavior, including suggesting the -D flag for force deletion.
Proper Safety Checks: All necessary checks are in place (branch exists, not current branch, merge status).
Documentation: README.md updated correctly to reflect the new -d flag support.

🔍 Issues Found

1. Critical: Incorrect Merge Detection Algorithm ⚠️

Location: src/command/branch.rs:203-213

The current algorithm has a logical flaw that makes it overly complex:

for commit in &branch_reachable {
    if !head_commit_ids.contains(&commit.id.to_string()) {
        eprintln!("error: The branch '{}' is not fully merged.", branch_name);
        return;
    }
}

Problem: This checks if all commits from the branch are in HEAD's history, but the correct approach is simpler: just check if the branch's HEAD commit is reachable from current HEAD.

Correct approach:

// Simply check if branch commit is in HEAD's reachable commits
if !head_commit_ids.contains(&branch.commit.to_string()) {
    eprintln!("error: The branch '{}' is not fully merged.", branch_name);
    eprintln!("If you are sure you want to delete it, run 'libra branch -D {}'.", branch_name);
    return;
}

If branch.commit is in HEAD's history, then by definition all commits in that branch are also in HEAD's history (since git commits form a DAG). This is how Git implements the check.

Fix merge detection logic →

2. Performance: Unnecessary Traversal ⚠️

Location: src/command/branch.rs:195-196

The code calls get_reachable_commits() for the branch being deleted, but this is unnecessary given the corrected logic above:

// This is not needed!
let branch_reachable = 
    crate::command::log::get_reachable_commits(branch.commit.to_string()).await;

If you only need to check if branch.commit is reachable from head_commit, you only need to traverse from HEAD.

Performance impact: For a large repo with long history, this doubles the work and memory usage unnecessarily.

Remove unnecessary traversal →

3. Code Style: Inconsistent Error Handling

Location: src/command/branch.rs:153-166 vs src/command/branch.rs:168-217

The delete_branch() function uses panic! for error cases:

panic!("fatal: Cannot delete the branch '{branch_name}' which you are currently on");

But delete_branch_safe() uses eprintln! + early return:

eprintln!("error: The branch '{}' is not fully merged.", branch_name);
return;

Recommendation: For consistency, both functions should use the same error handling approach. Using panic! for user errors is not idiomatic Rust. Consider using Result<(), String> return type or at minimum use eprintln! consistently.

4. Test Coverage Gap

Location: tests/command/branch_test.rs:514-639

The test test_branch_delete_safe is good but missing edge cases:

Same commit scenario: What if both branches point to the exact same commit? (Should allow deletion)
Detached HEAD: What happens if HEAD is detached and you try to delete a branch with -d?
Branch ahead by multiple commits: The test only creates one extra commit on the feature branch. What about multiple commits?

Recommendation: Add more test cases to cover these scenarios, especially the "same commit" case which should be allowed.

💡 Suggestions for Improvement

5. Documentation: Missing Function Doc Comment

Location: src/command/branch.rs:168

The delete_branch_safe() function lacks a doc comment. For consistency with other functions in the file and Rust best practices, add:

/// Safely delete a branch, refusing if it contains unmerged commits.
/// 
/// This performs a merge check to ensure the branch is fully merged into HEAD
/// before deletion. If the branch is not fully merged, prints an error and
/// suggests using `branch -D` for force deletion.
async fn delete_branch_safe(branch_name: String) {

6. Success Message Inconsistency

Location: src/command/branch.rs:217

println!("Deleted branch {} (was {}).", branch_name, branch.commit);

The force delete (delete_branch) function doesn't print anything on success, but safe delete does. For consistency, either:

Add the same message to delete_branch(), or
Remove it from delete_branch_safe()

Git prints this message for both -d and -D, so I'd recommend adding it to delete_branch() as well.

🎯 Summary

Category	Rating	Notes
Correctness	⚠️	Logic is overly complex but should work (issue #1)
Performance	⚠️	Unnecessary double traversal (issue #2)
Code Quality	✅	Clean and readable
Test Coverage	✅	Good basic coverage, but missing edge cases (issue #4)
Documentation	✅	README updated, but missing function docs (issue #5)
Security	✅	No security concerns

📋 Recommended Actions

Must Fix (before merge):

Simplify the merge detection logic (Issue move mercury and libra #1) - while the current logic should work, it's overly complex
Remove unnecessary branch_reachable traversal (Issue fix compilation #2) - performance optimization

Should Fix (before merge):
3. Consider using consistent error handling (Issue #3)
4. Add doc comment to delete_branch_safe() (Issue #5)

Nice to Have (can be follow-up):
5. Add more edge case tests (Issue #4)
6. Add success message to delete_branch() for consistency (Issue #6)

Great work overall! The implementation logic appears sound, but the merge detection can be significantly simplified and optimized. I've added inline comments on the specific code sections that need attention. Once issues #1 and #2 are addressed, this will be a solid addition to Libra. 🚀

src/command/branch.rs

tests/command/branch_test.rs

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0b049af729

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-01-27T02:20:56Z

src/command/branch.rs

    #[clap(short = 'D', long, group = "sub")]
    pub delete: Option<String>,

+    /// safe delete branch
+    #[clap(short = 'd', long, group = "sub")]


Align long flag so --delete is safe delete

The new safe-delete option is declared with #[clap(short = 'd', long)], which makes clap derive --delete-safe from the field name, while the force-delete option above still owns --delete. As a result, libra branch --delete <branch> continues to force-delete unmerged branches even though the README and Git semantics expect --delete to be safe, which can cause accidental data loss when users follow the docs. Consider moving --delete to the safe path and renaming the force delete long flag (e.g., --delete-force) to preserve expected behavior.

Useful? React with 👍 / 👎.

Copilot

Pull request overview

This PR adds support for branch -d (safe delete) to refuse deletion of unmerged branches, aligning Libra’s behavior with Git.

Changes:

Extend BranchArgs and the branch command dispatcher to support a new delete_safe (-d) mode that performs reachability checks before deleting a branch.
Implement delete_branch_safe using log::get_reachable_commits to ensure the target branch is fully merged into the current HEAD, including handling detached HEAD and current-branch cases.
Update branch-related integration tests (including a new test_branch_delete_safe) and README compatibility documentation to cover the new behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
`src/command/branch.rs`	Adds `delete_safe` flag handling and `delete_branch_safe` implementation using commit reachability to enforce safe branch deletion semantics.
`tests/command/branch_test.rs`	Updates existing `BranchArgs` constructions for the new field and adds `test_branch_delete_safe` to validate both unmerged and merged deletion scenarios.
`tests/command/reset_test.rs`	Updates helper `BranchArgs` initializations to include the new `delete_safe` field, keeping tests compiling and behavior unchanged.
`README.md`	Marks `branch -d` as implemented and updates the compatibility matrix to reflect safe delete support (with a minor mismatch in the documented long flag name).

README.md

src/command/branch.rs

- Fix merge detection logic: only check branch HEAD commit instead of traversing all commits - Remove unnecessary get_reachable_commits() call for branch - Add documentation for delete_branch_safe function - Fix long flag naming: --delete is now safe delete, --delete-force is force delete - Improve performance by avoiding unnecessary commit traversal Addresses code review comments from PR web3infra-foundation#170 Signed-off-by: Nook001 <[email protected]>

Store ObjectHash types directly in the HashSet instead of converting to String, eliminating unnecessary memory allocations during merge checks. This improves performance for repositories with large commit histories. - Replace .map(|c| c.id.to_string()) with .map(|c| c.id) - Compare branch.commit directly without .to_string() conversion - Reduces O(n) string allocations to zero for merge detection Signed-off-by: Nook001 <[email protected]>

genedna · 2026-01-27T09:47:43Z

@claude

genedna · 2026-01-27T09:47:52Z

@codex review

github-actions · 2026-01-27T09:47:59Z

Claude finished @genedna's task in 1m 45s —— View job

Code Review Complete ✅

I've conducted a comprehensive review of the branch -d (safe delete) implementation. The code is well-structured and functional, but I've identified several important issues and opportunities for improvement.

Summary

This PR successfully implements the safe delete functionality for branches (-d flag), which checks if a branch is fully merged before deletion. The implementation correctly uses commit reachability checking via BFS traversal. However, there are some important issues around the CLI flag design and test coverage that should be addressed.

🔴 Critical Issues

1. CLI Flag Naming Inconsistency Creates Dangerous UX

Location: src/command/branch.rs:33-38

The current implementation has a critical UX issue:

/// force delete branch
#[clap(short = 'D', long = "delete-force", group = "sub")]
pub delete: Option<String>,

/// safe delete branch (checks if merged before deletion)
#[clap(short = 'd', long = "delete", group = "sub")]
pub delete_safe: Option<String>,

Problem:

-d → safe delete (correct, matches Git)
-D → force delete with --delete-force (correct)
--delete → safe delete (correct, matches Git)
BUT: The README says -d, --delete <branch> which matches Git and user expectations

This is actually correct behavior now! The implementation properly maps:

Short flags: -d (safe) and -D (force) ✅
Long flags: --delete (safe) and --delete-force (force) ✅

This matches Git's behavior exactly. The Codex bot's suggestion to change this is incorrect and should be ignored.

Verdict: ✅ This is correct as implemented. No changes needed.

2. Merge Detection Algorithm is Correct but Simpler Alternative Exists

Location: src/command/branch.rs:199-205

The current implementation correctly checks if the branch commit is reachable from HEAD:

let head_commit_ids: std::collections::HashSet<_> =
    head_reachable.iter().map(|c| c.id).collect();

if !head_commit_ids.contains(&branch.commit) {
    eprintln!("error: The branch '{}' is not fully merged.", branch_name);
    // ...
}

Analysis: This is correct. The algorithm:

Gets all commits reachable from HEAD via BFS traversal
Checks if the branch's tip commit is in that set
If yes → branch is merged; if no → branch has diverged

This matches Git's behavior. While the previous bot review suggested this was overly complex, it's actually the right approach and has already been optimized in recent commits.

Verdict: ✅ Algorithm is correct. The recent optimization using ObjectHash directly (avoiding string allocations) is excellent.

⚠️ Important Issues

3. Inconsistent Error Handling Between delete_branch() and delete_branch_safe()

Location: src/command/branch.rs:153-166 vs src/command/branch.rs:173-218

The force delete function uses panic! for errors:

panic!("fatal: Cannot delete the branch '{branch_name}' which you are currently on");

While safe delete uses the same approach, which is good for consistency. However, using panic! for user errors is not idiomatic Rust. These are expected error conditions, not programming bugs.

Recommendation: Consider refactoring both functions to return Result<(), String> in a future PR. This would:

Allow better error handling in tests
Follow Rust best practices
Make the codebase more maintainable

Current Status: Both functions are consistent with each other, which is acceptable for now. This is a candidate for future refactoring.

4. Success Message Inconsistency

Location: src/command/branch.rs:217 vs src/command/branch.rs:165

Safe delete prints a success message:

println!("Deleted branch {} (was {}).", branch_name, branch.commit);

But force delete (delete_branch) does not print anything on success.

Git's behavior: Git prints the deletion message for both -d and -D:

$ git branch -d feature
Deleted branch feature (was abc1234).

$ git branch -D feature  
Deleted branch feature (was abc1234).

Recommendation: Add the same success message to delete_branch() for consistency with Git and the safe delete implementation.

Add success message to force delete →

💡 Test Coverage Gaps

5. Missing Edge Case Tests

Location: tests/command/branch_test.rs:514-639

The test test_branch_delete_safe covers the basic scenarios well:

✅ Refusing to delete an unmerged branch
✅ Successfully deleting a merged branch

However, it's missing several important edge cases:

Missing scenarios:

Same commit scenario: What if both branches point to the exact same commit?

// master and feature both at commit A
// libra branch -d feature  -> should succeed

Detached HEAD scenario: Try deleting with -d when HEAD is detached

// HEAD detached at commit A
// feature branch also at commit A
// libra branch -d feature -> should succeed

Multiple unmerged commits: The test only creates one commit on the feature branch. Test with multiple commits ahead:
```
// master at A, feature at A -> B -> C
// libra branch -d feature -> should refuse
```
Attempting to delete current branch with -d: This should fail with the same error as -D

Recommendation: Add these test cases in a follow-up commit or PR to increase confidence in the implementation.

✅ Strengths

✅ Correct Algorithm: The merge detection using BFS traversal via get_reachable_commits() is the right approach and matches Git's behavior.
✅ Performance Optimization: Recent commits optimized to use ObjectHash directly instead of string conversions, avoiding unnecessary allocations.
✅ Good Documentation: The function has a clear doc comment explaining what it does.
✅ Proper Safety Checks: All necessary validations are in place:
- Branch exists
- Not deleting current branch
- Merge status verification
- Helpful error messages
✅ CLI Design: The flag naming (-d/--delete for safe, -D/--delete-force for force) correctly matches Git's behavior.
✅ README Updated: Compatibility table correctly documents the new feature.

📊 Review Summary

Category	Rating	Notes
Correctness	✅ Excellent	Algorithm is correct and well-implemented
Performance	✅ Good	Recent optimization using ObjectHash is smart
Code Quality	✅ Good	Clean, readable, well-documented
Test Coverage	⚠️ Fair	Basic scenarios covered, but missing edge cases
CLI/UX	✅ Excellent	Properly matches Git's behavior
Documentation	✅ Good	Function docs and README updated
Security	✅ Good	No security concerns

🎯 Recommendations

Before Merge (Nice to Have):

✅ Consider adding success message to delete_branch() for consistency (Issue Add .github and docs #4)
📝 Add edge case tests for same-commit, detached HEAD, and multiple unmerged commits scenarios (Issue remove gemini #5)

Future Improvements (Follow-up PR):
3. Consider refactoring error handling from panic! to Result types for both delete functions (Issue #3)

🎉 Conclusion

This is a solid implementation that correctly implements Git's branch -d behavior. The merge detection algorithm is correct, the CLI flags match Git's expectations, and the code is clean and well-documented.

Previous bot reviews had some incorrect suggestions (particularly around the CLI flag naming and merge detection "complexity"). The current implementation is correct and the recent optimizations show good attention to performance.

The main improvement opportunity is in test coverage - adding more edge case tests would increase confidence. The success message inconsistency is minor but worth fixing for better UX alignment with Git.

Recommendation: Approve with minor suggestions for enhanced test coverage and consistency improvements.

Great work on this feature! The implementation shows solid understanding of Git's merge detection logic and attention to both correctness and performance. 🚀

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 65ba205e60

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-01-27T09:53:32Z

src/command/branch.rs

+    // Get all commits reachable from HEAD
+    let head_reachable = crate::command::log::get_reachable_commits(head_commit.to_string()).await;


Consider upstream when checking safe delete

The safe-delete check builds reachability only from the current HEAD. If a branch has an upstream configured and was merged into that upstream while the user is on a different branch, this logic will still refuse deletion because the upstream history isn’t consulted. That produces false negatives for safe deletes in common workflows (e.g., delete a merged feature branch while staying on another branch), forcing users to switch branches or use -D. Consider consulting the branch’s upstream when present and falling back to HEAD only when no upstream is configured.

Useful? React with 👍 / 👎.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Copilot · 2026-01-27T10:03:48Z

src/command/branch.rs

+    #[clap(short = 'D', long = "delete-force", group = "sub")]
    pub delete: Option<String>,

+    /// safe delete branch (checks if merged before deletion)
+    #[clap(short = 'd', long = "delete", group = "sub")]


Changing the long flag for force delete from the default --delete to --delete-force while also reusing --delete for safe delete introduces a breaking CLI change that isn’t documented and may surprise existing users or scripts that rely on libra branch --delete for force deletion. Consider either preserving --delete as the long form for force delete (and using a different long name for safe delete, such as --delete-safe), or supporting both long forms for force delete via clap aliases so that existing usage keeps working.

Suggested change

#[clap(short = 'D', long = "delete-force", group = "sub")]

pub delete: Option<String>,

/// safe delete branch (checks if merged before deletion)

#[clap(short = 'd', long = "delete", group = "sub")]

#[clap(short = 'D', long = "delete", visible_alias = "delete-force", group = "sub")]

pub delete: Option<String>,

/// safe delete branch (checks if merged before deletion)

#[clap(short = 'd', long = "delete-safe", group = "sub")]

Nook001 force-pushed the feat/branch-safe-delete-168 branch from abde9f1 to 3a3dc93 Compare January 26, 2026 18:13

style: apply rustfmt formatting

0b049af

Signed-off-by: Nook001 <[email protected]>

genedna requested a review from Copilot January 27, 2026 02:15

Copilot started reviewing on behalf of genedna January 27, 2026 02:16 View session

github-actions bot reviewed Jan 27, 2026

View reviewed changes

src/command/branch.rs Outdated Show resolved Hide resolved

github-actions bot reviewed Jan 27, 2026

View reviewed changes

src/command/branch.rs Outdated Show resolved Hide resolved

github-actions bot reviewed Jan 27, 2026

View reviewed changes

src/command/branch.rs Show resolved Hide resolved

github-actions bot reviewed Jan 27, 2026

View reviewed changes

tests/command/branch_test.rs Show resolved Hide resolved

chatgpt-codex-connector bot reviewed Jan 27, 2026

View reviewed changes

Copilot AI reviewed Jan 27, 2026

View reviewed changes

README.md Show resolved Hide resolved

src/command/branch.rs Outdated Show resolved Hide resolved

Nook001 added 2 commits January 27, 2026 14:29

genedna requested a review from Copilot January 27, 2026 09:47

Copilot started reviewing on behalf of genedna January 27, 2026 09:48 View session

chatgpt-codex-connector bot reviewed Jan 27, 2026

View reviewed changes

genedna approved these changes Jan 27, 2026

View reviewed changes

genedna merged commit 37e9234 into web3infra-foundation:main Jan 27, 2026
16 checks passed

Copilot AI reviewed Jan 27, 2026

View reviewed changes

		// Get all commits reachable from HEAD
		let head_reachable = crate::command::log::get_reachable_commits(head_commit.to_string()).await;

feat(branch): implement safe delete with -d flag (#168) #170

feat(branch): implement safe delete with -d flag (#168) #170

Uh oh!

Conversation

Nook001 commented Jan 26, 2026

Issue

Summary

Changes

Implementation Details

合并检查算法

与 Git 行为对照

Testing

Uh oh!

genedna commented Jan 27, 2026

Uh oh!

genedna commented Jan 27, 2026

Uh oh!

github-actions bot commented Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Complete ✅

✅ Strengths

🔍 Issues Found

1. Critical: Incorrect Merge Detection Algorithm ⚠️

2. Performance: Unnecessary Traversal ⚠️

3. Code Style: Inconsistent Error Handling

4. Test Coverage Gap

💡 Suggestions for Improvement

5. Documentation: Missing Function Doc Comment

6. Success Message Inconsistency

🎯 Summary

📋 Recommended Actions

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

genedna commented Jan 27, 2026

Uh oh!

genedna commented Jan 27, 2026

Uh oh!

github-actions bot commented Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Complete ✅

Summary

🔴 Critical Issues

1. CLI Flag Naming Inconsistency Creates Dangerous UX

2. Merge Detection Algorithm is Correct but Simpler Alternative Exists

⚠️ Important Issues

3. Inconsistent Error Handling Between delete_branch() and delete_branch_safe()

4. Success Message Inconsistency

💡 Test Coverage Gaps

5. Missing Edge Case Tests

✅ Strengths

📊 Review Summary

🎯 Recommendations

🎉 Conclusion

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

github-actions bot commented Jan 27, 2026 •

edited

Loading

github-actions bot commented Jan 27, 2026 •

edited

Loading