Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lunygithub wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 57 additions & 1 deletion src/command/log.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,10 @@ pub struct LogArgs {
/// Files to limit diff output (used with -p, --name-only, or --stat)
#[clap(value_name = "PATHS", num_args = 0..)]
pathspec: Vec<String>,

/// Filter commits by message content (case-sensitive substring match)
#[clap(long)]
pub grep: Option<String>,
}

#[derive(PartialEq, Debug)]
Expand Down Expand Up @@ -382,6 +386,16 @@ pub async fn execute_safe(args: LogArgs, output: &OutputConfig) -> CliResult<()>
// 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();
Comment on lines +392 to +395
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

}
}

Comment on lines 386 to +398
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--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 uses AI. Check for mistakes.
if output.quiet {
return Ok(());
}
Expand Down Expand Up @@ -1120,4 +1134,46 @@ mod tests {
assert!(name_status);
assert!(!patch);
}
}

// 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);
}
Comment on lines +1140 to +1146
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
Comment on lines +1138 to +1146
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot generated this review using guidance from repository custom instructions.

// 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()));
}
}
42 changes: 42 additions & 0 deletions tests/command/log_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1093,3 +1093,45 @@ async fn test_log_short_number_flag_with_double_dash_before_subcommand() {
assert!(status.success(), "libra -- log -2 failed: {err}");
assert_eq!(count_commit_lines(&out), 2);
}

// 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()));
Comment on lines +1099 to +1101
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.


let args = LogArgs::parse_from(["libra", "log"]);
assert_eq!(args.grep, None);
}
Comment on lines +1097 to +1105
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 generated this review using guidance from repository custom instructions.

// 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()));
Comment on lines +1100 to +1136
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
}
Loading