feat(log): add --grep parameter for commit message filtering by lunygithub · Pull Request #328 · web3infra-foundation/libra

lunygithub · 2026-03-31T06:49:29Z

功能描述
为 log 命令添加 --grep 参数，支持按提交信息过滤提交历史。
实现功能

基于提交消息的内容进行搜索匹配（大小写敏感）
支持与其他参数组合使用（如 --oneline、-n、--stat 等）
空字符串 --grep "" 时跳过过滤，显示所有提交
使用示例
libra log --grep "fix" # 显示包含 "fix" 的提交
libra log --grep "feature" --oneline # 单行显示包含 "feature" 的提交
libra log --graph --grep "fix" # 图形化显示包含 "fix" 的提交
libra log --grep "" # 空字符串显示所有提交
测试验证
所有测试已通过：
test_log_args_grep ✅
test_grep_with_other_args ✅
test_grep_case_sensitive ✅
test_grep_empty_string ✅
test_graph_with_grep ✅
相关链接
原 Issue: [r2cn-测试任务] 为 log 命令添加 --grep 参数，支持按提交信息过滤提交历史 #47
原 PR: feat(log): add --grep parameter for commit message filtering #60
@genedna 请帮忙审核

genedna · 2026-03-31T06:52:30Z

Thanks for the PR! The --grep feature is a useful addition. I have a few suggestions after reviewing the diff:

1. Integrate `--grep` into the existing `CommitFilter` instead of a separate pass

There's already a CommitFilter struct (line 121) that handles --author, --since, --until, and path filtering via passes_non_path_filters(). The --grep filter should be added there rather than as a standalone Vec::collect() pass after sorting. This:

Keeps all commit filtering logic in one place
Avoids an extra allocation (the current code collects into a new Vec before the existing per-commit filter loop)
Ensures consistent behavior with other filters

Suggested approach — add a grep field to CommitFilter and check it in passes_non_path_filters():

struct CommitFilter {
    author: Option<String>,
    since: Option<i64>,
    until: Option<i64>,
    paths: Vec<PathBuf>,
    grep: Option<String>,  // add this
}

fn passes_non_path_filters(&self, commit: &Commit) -> bool {
    // ... existing author/since/until checks ...

    if let Some(pattern) = &self.grep {
        if !pattern.is_empty() && !commit.message.contains(pattern.as_str()) {
            return false;
        }
    }
    true
}

2. Consider `--graph` interaction

When --grep filters out commits in the middle of the history, the graph output may show disconnected/misleading topology. Git handles this via history simplification. This is probably fine for an initial implementation, but worth noting in the CLI help text or as a known limitation.

3. Duplicate tests — remove one copy

The tests in tests/command/log_test.rs are exact copies of the unit tests added to src/command/log.rs. Only one copy is needed — I'd suggest keeping them in tests/command/log_test.rs (the integration test file) and removing the duplicates from the mod tests block in log.rs, or vice versa.

4. Missing integration test for actual filtering behavior

All tests only verify CLI argument parsing (LogArgs::parse_from), not that grep actually filters commits. Consider adding an integration test that:

Creates a repo with several commits (e.g., "fix: bug", "feat: new feature", "docs: readme")
Runs libra log --grep "fix"
Asserts only the matching commit(s) appear in the output

This would catch regressions in the filtering logic itself.

5. Minor: missing newline at end of file

Both changed files are missing a trailing newline (\ No newline at end of file in the diff). Please add one to keep POSIX compliance and avoid noisy diffs in the future.

Generated by Claude Code

Copilot

Pull request overview

This PR adds a --grep option to the log command to filter the displayed commit history by commit message substring (case-sensitive), aligning Libra’s CLI with common Git log workflows.

Changes:

Add --grep to LogArgs and apply message substring filtering in execute_safe.
Add tests for --grep argument parsing (currently duplicated across unit + integration test files).

Reviewed changes

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

File	Description
`src/command/log.rs`	Adds `--grep` argument and applies commit-message filtering before rendering log output; adds parsing-focused tests.
`tests/command/log_test.rs`	Adds parsing-focused tests for the new `--grep` argument in the command integration test suite.

Copilot · 2026-03-31T06:55:57Z

src/command/log.rs

    // default sort with signature time
    reachable_commits.sort_by_key(|b| std::cmp::Reverse(b.committer.timestamp));

+    // Apply grep filtering
+    if let Some(pattern) = &args.grep {
+        if !pattern.is_empty() {
+            reachable_commits = reachable_commits
+                .into_iter()
+                .filter(|commit| commit.message.contains(pattern))
+                .collect();
+        }
+    }
+


--grep filtering is applied after sorting reachable_commits, which means you still sort commits that will be discarded. For large histories this adds avoidable overhead when --grep is used. Consider filtering first (or using retain) and then sorting the remaining commits, keeping the same output order semantics.

Suggested change

// default sort with signature time

reachable_commits.sort_by_key(|b| std::cmp::Reverse(b.committer.timestamp));

// Apply grep filtering

if let Some(pattern) = &args.grep {

if !pattern.is_empty() {

reachable_commits = reachable_commits

.into_iter()

.filter(|commit| commit.message.contains(pattern))

.collect();

}

}

// Apply grep filtering before sorting to avoid sorting commits that will be discarded.

if let Some(pattern) = &args.grep {

if !pattern.is_empty() {

reachable_commits

.retain(|commit| commit.message.contains(pattern));

}

}

// default sort with signature time

reachable_commits.sort_by_key(|b| std::cmp::Reverse(b.committer.timestamp));

Copilot · 2026-03-31T06:55:58Z

tests/command/log_test.rs

+    let args = LogArgs::parse_from(["libra", "log", "--grep", "fix"]);
+    assert_eq!(args.grep, Some("fix".to_string()));
+
+    let args = LogArgs::parse_from(["libra", "log"]);
+    assert_eq!(args.grep, None);
+}
+
+// Test grep combined with other arguments
+#[test]
+fn test_grep_with_other_args() {
+    let args =
+        LogArgs::parse_from(["libra", "log", "--grep", "feature", "--oneline", "-n", "5"]);
+    assert_eq!(args.grep, Some("feature".to_string()));
+    assert!(args.oneline);
+    assert_eq!(args.number, Some(5));
+}
+
+// Test case-sensitive matching
+#[test]
+fn test_grep_case_sensitive() {
+    let args = LogArgs::parse_from(["libra", "log", "--grep", "FIX"]);
+    assert_eq!(args.grep, Some("FIX".to_string()));
+}
+
+// Test empty string grep
+#[test]
+fn test_grep_empty_string() {
+    let args = LogArgs::parse_from(["libra", "log", "--grep", ""]);
+    assert_eq!(args.grep, Some("".to_string()));
+}
+
+// Test graph with grep combination
+#[test]
+fn test_graph_with_grep() {
+    let args = LogArgs::parse_from(["libra", "log", "--graph", "--grep", "fix"]);
+    assert!(args.graph);
+    assert_eq!(args.grep, Some("fix".to_string()));


These LogArgs::parse_from(["libra", "log", ...]) assertions don’t actually parse the real CLI shape: the literal "log" is consumed as a positional pathspec value (since pathspec: Vec<String> accepts arbitrary trailing args). That makes the test pass while silently setting pathspec, and it won’t catch regressions in subcommand wiring. Prefer parsing without the extra "log" (e.g. ["libra", "--grep", "fix"]) and/or assert args.pathspec.is_empty() here.

Suggested change

let args = LogArgs::parse_from(["libra", "log", "--grep", "fix"]);

assert_eq!(args.grep, Some("fix".to_string()));

let args = LogArgs::parse_from(["libra", "log"]);

assert_eq!(args.grep, None);

}

// Test grep combined with other arguments

#[test]

fn test_grep_with_other_args() {

let args =

LogArgs::parse_from(["libra", "log", "--grep", "feature", "--oneline", "-n", "5"]);

assert_eq!(args.grep, Some("feature".to_string()));

assert!(args.oneline);

assert_eq!(args.number, Some(5));

}

// Test case-sensitive matching

#[test]

fn test_grep_case_sensitive() {

let args = LogArgs::parse_from(["libra", "log", "--grep", "FIX"]);

assert_eq!(args.grep, Some("FIX".to_string()));

}

// Test empty string grep

#[test]

fn test_grep_empty_string() {

let args = LogArgs::parse_from(["libra", "log", "--grep", ""]);

assert_eq!(args.grep, Some("".to_string()));

}

// Test graph with grep combination

#[test]

fn test_graph_with_grep() {

let args = LogArgs::parse_from(["libra", "log", "--graph", "--grep", "fix"]);

assert!(args.graph);

assert_eq!(args.grep, Some("fix".to_string()));

let args = LogArgs::parse_from(["libra", "--grep", "fix"]);

assert_eq!(args.grep, Some("fix".to_string()));

assert!(args.pathspec.is_empty());

let args = LogArgs::parse_from(["libra"]);

assert_eq!(args.grep, None);

assert!(args.pathspec.is_empty());

}

// Test grep combined with other arguments

#[test]

fn test_grep_with_other_args() {

let args =

LogArgs::parse_from(["libra", "--grep", "feature", "--oneline", "-n", "5"]);

assert_eq!(args.grep, Some("feature".to_string()));

assert!(args.oneline);

assert_eq!(args.number, Some(5));

assert!(args.pathspec.is_empty());

}

// Test case-sensitive matching

#[test]

fn test_grep_case_sensitive() {

let args = LogArgs::parse_from(["libra", "--grep", "FIX"]);

assert_eq!(args.grep, Some("FIX".to_string()));

assert!(args.pathspec.is_empty());

}

// Test empty string grep

#[test]

fn test_grep_empty_string() {

let args = LogArgs::parse_from(["libra", "--grep", ""]);

assert_eq!(args.grep, Some("".to_string()));

assert!(args.pathspec.is_empty());

}

// Test graph with grep combination

#[test]

fn test_graph_with_grep() {

let args = LogArgs::parse_from(["libra", "--graph", "--grep", "fix"]);

assert!(args.graph);

assert_eq!(args.grep, Some("fix".to_string()));

assert!(args.pathspec.is_empty());

Copilot · 2026-03-31T06:55:58Z

tests/command/log_test.rs

+// Test grep parameter parsing
+#[test]
+fn test_log_args_grep() {
+    let args = LogArgs::parse_from(["libra", "log", "--grep", "fix"]);
+    assert_eq!(args.grep, Some("fix".to_string()));
+
+    let args = LogArgs::parse_from(["libra", "log"]);
+    assert_eq!(args.grep, None);
+}


The added tests here only validate clap parsing; they don’t verify that log --grep <pattern> actually filters output (including the “no matches => no output” behavior and --grep "" bypass). Since this PR changes execute_safe output behavior, please add an integration test that runs the libra log binary (you already have run_log_cmd) and asserts the filtered commit list.

Copilot · 2026-03-31T06:55:58Z

src/command/log.rs

+    fn test_log_args_grep() {
+        let args = LogArgs::parse_from(["libra", "log", "--grep", "fix"]);
+        assert_eq!(args.grep, Some("fix".to_string()));
+
+        let args = LogArgs::parse_from(["libra", "log"]);
+        assert_eq!(args.grep, None);
+    }


Same issue as the integration test file: LogArgs::parse_from(["libra", "log", ...]) treats the "log" token as a positional pathspec entry, not a subcommand name. That makes these tests misleading and can mask bugs. Please parse LogArgs without the extra "log" token and consider asserting pathspec is empty.

Copilot · 2026-03-31T06:55:58Z

src/command/log.rs

+    // Test grep parameter parsing
+    #[test]
+    fn test_log_args_grep() {
+        let args = LogArgs::parse_from(["libra", "log", "--grep", "fix"]);
+        assert_eq!(args.grep, Some("fix".to_string()));
+
+        let args = LogArgs::parse_from(["libra", "log"]);
+        assert_eq!(args.grep, None);
+    }


These new parsing-only tests are duplicated in both src/command/log.rs and tests/command/log_test.rs, but neither validates the actual --grep filtering behavior in execute_safe. Prefer keeping a single parsing test (if needed) and add/keep integration tests that execute libra log --grep ... and assert stdout matches expected commits.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ed1f48fdb9

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector · 2026-03-31T06:56:21Z

src/command/log.rs

+            reachable_commits = reachable_commits
+                .into_iter()
+                .filter(|commit| commit.message.contains(pattern))
+                .collect();


Preserve global hash uniqueness when filtering by --grep

execute_safe filters reachable_commits by --grep before default_abbrev is computed, so abbreviation length is now derived from only the matched subset instead of full reachable history. In repositories where a 7-character prefix is unique only within the subset, --oneline/--abbrev-commit output can become ambiguous when users reuse that hash in later commands. Keep abbreviation-length calculation based on the unfiltered reachable set (or retain a pre-filter copy) and apply grep only to the displayed iteration set.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-03-31T06:56:21Z

tests/command/log_test.rs

+fn test_log_args_grep() {
+    let args = LogArgs::parse_from(["libra", "log", "--grep", "fix"]);
+    assert_eq!(args.grep, Some("fix".to_string()));


Add end-to-end tests for --grep filtering behavior

The added tests only verify Clap argument parsing and never execute log to confirm that commit selection is actually filtered by message content, case sensitivity, empty pattern handling, or interactions with flags like -n/--graph. That leaves the new runtime filtering path in execute_safe effectively untested, so logic regressions could ship while this suite remains green. Add at least one integration test that creates commits with different messages and asserts CLI output for matching and non-matching --grep queries.

Useful? React with 👍 / 👎.

feat(log): add --grep parameter for commit message filtering

ed1f48f

genedna requested a review from Copilot March 31, 2026 06:51

Copilot started reviewing on behalf of genedna March 31, 2026 06:52 View session

Copilot AI reviewed Mar 31, 2026

View reviewed changes

chatgpt-codex-connector bot reviewed Mar 31, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(log): add --grep parameter for commit message filtering#328

feat(log): add --grep parameter for commit message filtering#328
lunygithub wants to merge 1 commit intoweb3infra-foundation:mainfrom
lunygithub:feat/add-grep-param-v2

lunygithub commented Mar 31, 2026

Uh oh!

genedna commented Mar 31, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

chatgpt-codex-connector bot left a comment

Uh oh!

chatgpt-codex-connector bot Mar 31, 2026

Uh oh!

chatgpt-codex-connector bot Mar 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

lunygithub commented Mar 31, 2026

Uh oh!

genedna commented Mar 31, 2026

1. Integrate --grep into the existing CommitFilter instead of a separate pass

2. Consider --graph interaction

3. Duplicate tests — remove one copy

4. Missing integration test for actual filtering behavior

5. Minor: missing newline at end of file

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector bot Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

1. Integrate `--grep` into the existing `CommitFilter` instead of a separate pass

2. Consider `--graph` interaction