Skip to content
Closed
Changes from all commits
Commits
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
38 changes: 38 additions & 0 deletions src/command/log.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,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
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider clarifying that the grep filter performs case-sensitive substring matching. This helps set user expectations about the behavior. For example:

/// Filter commits by message content (case-sensitive substring match)

or

/// Filter commits by message content. Performs case-sensitive substring matching.
Suggested change
/// Filter commits by message content
/// Filter commits by message content (case-sensitive substring match)

Copilot uses AI. Check for mistakes.
#[clap(long)]
pub grep: Option<String>,
}

#[derive(PartialEq, Debug)]
Expand Down Expand Up @@ -200,6 +204,14 @@ pub async fn execute(args: LogArgs) {
// default sort with signature time
reachable_commits.sort_by(|a, b| b.committer.timestamp.cmp(&a.committer.timestamp));

// Apply grep filtering
if let Some(pattern) = &args.grep {
reachable_commits = reachable_commits
.into_iter()
.filter(|commit| commit.message.contains(pattern))
.collect();
Comment on lines +209 to +212
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider handling the edge case where the grep pattern is an empty string. Currently, an empty string will match all commits (since contains("") returns true for any string). While this might be acceptable, it could be clearer to either:

  1. Skip filtering when the pattern is empty
  2. Document this behavior in the help text

Example improvement:

// 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();
    }
}
Suggested change
reachable_commits = reachable_commits
.into_iter()
.filter(|commit| commit.message.contains(pattern))
.collect();
if !pattern.is_empty() {
reachable_commits = reachable_commits
.into_iter()
.filter(|commit| commit.message.contains(pattern))
.collect();
}

Copilot uses AI. Check for mistakes.
Comment on lines +207 to +212

Choose a reason for hiding this comment

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

P2 Badge Clear graph columns when commits are skipped by --grep

The new grep filter is applied before rendering the graph, but GraphState only clears a column when that commit is actually rendered (see GraphState::render in the same file). With libra log --graph --grep <pattern>, if a matching commit’s parent does not match the pattern, the parent hash is pushed into columns but never removed, so later unrelated commits are printed with stray | branches and the column list grows as more ancestors are skipped. This yields incorrect graph output whenever grep hides intermediate commits.

Useful? React with 👍 / 👎.

}

let ref_commits = create_reference_commit_map().await;

let max_output_number = min(args.number.unwrap_or(usize::MAX), reachable_commits.len());
Expand Down Expand Up @@ -861,4 +873,30 @@ mod tests {
assert!(name_only);
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);
}

// 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()));
}
}
Loading