From 652adf5fac05cba95b46d5ac0ace476a623f1dc5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=8E=E5=86=A0=E8=BE=B0?= Date: Thu, 21 May 2026 19:12:47 +0800 Subject: [PATCH] fix(git/log): stop dropping merge commits when user pins a selection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `rtk git log -1 HEAD` on a repo whose HEAD is a `--no-ff` merge commit M (parents [P1, P2]) was returning P2 instead of M. Same for explicit SHAs, annotated tags, `--graph`, and `--graph --oneline` — every shape that names a specific commit got the second parent of the merge instead of the merge itself. Caller can't tell the result is wrong because P2 is a real recent commit. Root cause: `run_log` unconditionally injects `--no-merges` as a token-saving default. When the user passes `-1`, git walks past the dropped merge to the next non-merge commit (the second parent because git log defaults to a topology-aware traversal), so a precise pin silently shifts the selection. `--no-merges` is the right default only when RTK is fully driving the output — bare `git log` with no flags, no ref, no format. As soon as the user signals "I'm picking", the default has to stand down. Extract `should_add_no_merges_default(args, has_format, has_limit)` as a pure helper. Returns `true` only when none of these signals are present: - `--format` / `--pretty` / `--oneline` (caller controls output shape) - `-N` / `-n N` / `--max-count=N` (caller picked a precise window) - `--graph` (caller wants topology — needs merges) - any positional argument (ref / SHA / tag / pathspec) - explicit `--merges` / `--min-parents=2` (caller already opted in) `has_format` and `has_limit` are passed in from the caller's existing scan so we don't re-walk the args list. Behaviour matrix (existing tests stayed green, the new ones pin each shape): | invocation | --no-merges? | |---------------------------------------|--------------| | `rtk git log` | yes (RTK default) | `rtk git log -1 HEAD` | no (#2009 fix) | `rtk git log -1` | no | `rtk git log --format=%H HEAD` | no | `rtk git log ` | no | `rtk git log --graph` | no | `rtk git log --graph --oneline` | no | `rtk git log --merges` | no (explicit opt-in) | `rtk git log --min-parents=2` | no | `rtk git log --author=foo` | yes (filter, not a pin) | `rtk git log --max-count=5` | no Tests: - test_no_merges_added_for_bare_git_log — token-saving default stays. - test_no_merges_skipped_with_explicit_limit_one + _only — covers `-1 HEAD` and bare `-1`, the headline reproduction from #2009. - test_no_merges_skipped_with_format_flag_and_ref + _explicit_sha + _graph_flag + _graph_and_oneline — covers every other shape the issue body called out as broken. - test_no_merges_skipped_with_explicit_merges + _min_parents — locks in the pre-existing opt-in semantics so the refactor doesn't regress it. - test_no_merges_still_added_with_author_filter — documents the deliberate narrow scope: filter flags that don't pick a single commit still benefit from the token-saving default. - test_no_merges_skipped_with_max_count_arg — short-circuit via has_limit. cargo fmt / clippy --all-targets / test --bin rtk: 1920 passed, 0 warnings. Fixes #2009 --- src/cmds/git/git.rs | 145 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 140 insertions(+), 5 deletions(-) diff --git a/src/cmds/git/git.rs b/src/cmds/git/git.rs index 6f42d20e8..4993d9eea 100644 --- a/src/cmds/git/git.rs +++ b/src/cmds/git/git.rs @@ -519,11 +519,13 @@ fn run_log( (10, false) }; - // Only add --no-merges if user didn't explicitly request merge commits - let wants_merges = args - .iter() - .any(|arg| arg == "--merges" || arg == "--min-parents=2"); - if !wants_merges { + // Add --no-merges only when RTK is fully driving defaults. When the user + // pins to specific commits (--format / -N / --graph / explicit ref or + // pathspec), --no-merges silently shifts the selection — e.g. + // `git log -1 HEAD` on a merge commit M resolves to M's second parent + // because `-1` walks past the dropped merge to the next non-merge + // commit. See #2009. + if should_add_no_merges_default(args, has_format_flag, has_limit_flag) { cmd.arg("--no-merges"); } @@ -557,6 +559,44 @@ fn run_log( Ok(0) } +/// Decide whether RTK should inject `--no-merges` as a token-saving default. +/// +/// RTK drops merge commits from `git log` to keep output compact in the +/// summary case, but that silently shifts the result whenever the user has +/// pinned a specific commit — `git log -1 HEAD` against a merge commit M +/// walks past M and returns its second parent instead of M itself (#2009). +/// +/// The function returns `true` only when none of these "user is driving +/// selection" signals are present: +/// - `--format` / `--pretty` / `--oneline` (caller controls output shape) +/// - `-N` / `-n N` / `--max-count=N` (caller asked for a precise window) +/// - `--graph` (caller explicitly wants topology, which depends on merges) +/// - any positional argument (treated as ref / SHA / tag / pathspec — even +/// `--` separators or pathspecs land here; in every case it's a user +/// selection signal, not RTK-driven defaults) +/// - explicit `--merges` / `--min-parents=2` (caller already opted into +/// merges; injecting `--no-merges` would contradict the request) +/// +/// `has_format` and `has_limit` are pre-computed in the caller to avoid +/// re-scanning the args list. +fn should_add_no_merges_default(args: &[String], has_format: bool, has_limit: bool) -> bool { + if has_format || has_limit { + return false; + } + let user_specified_selection = args.iter().any(|arg| { + // --graph signals "show me topology" — dropping merges defeats it. + // Any positional (non-flag) arg is treated as a ref or pathspec. + arg == "--graph" || !arg.starts_with('-') + }); + if user_specified_selection { + return false; + } + let explicit_merge_request = args + .iter() + .any(|arg| arg == "--merges" || arg == "--min-parents=2"); + !explicit_merge_request +} + /// Filter git log output: truncate long messages, cap lines /// Parse the user-specified limit from git log args. /// Handles: -20, -n 20, --max-count=20, --max-count 20 @@ -2454,6 +2494,101 @@ A added.rs assert_eq!(parse_user_limit(&args), None); } + // --- #2009: --no-merges must NOT be added when the user pins a commit --- + + fn strs(args: &[&str]) -> Vec { + args.iter().map(|s| s.to_string()).collect() + } + + #[test] + fn test_no_merges_added_for_bare_git_log() { + // No user args at all → RTK fully drives defaults, drop merges for + // token savings. + assert!(should_add_no_merges_default(&[], false, false)); + } + + #[test] + fn test_no_merges_skipped_with_explicit_limit_one() { + // `git log -1 HEAD` — the exact reproduction from #2009. `-1` is a + // user-driven pin; dropping merges would skip a merge at HEAD and + // surface its second parent. + let args = strs(&["-1", "HEAD"]); + assert!(!should_add_no_merges_default(&args, false, true)); + } + + #[test] + fn test_no_merges_skipped_with_explicit_limit_only() { + // `git log -1` (no ref) — still a precise pin; dropping merges is + // wrong because the user asked for exactly one commit. + let args = strs(&["-1"]); + assert!(!should_add_no_merges_default(&args, false, true)); + } + + #[test] + fn test_no_merges_skipped_with_format_flag_and_ref() { + // `git log --format=%H HEAD` — user controls output shape AND + // points at a specific ref. + let args = strs(&["--format=%H", "HEAD"]); + assert!(!should_add_no_merges_default(&args, true, false)); + } + + #[test] + fn test_no_merges_skipped_with_explicit_sha() { + // `git log ` — bare positional arg. Even without a format or + // limit flag, naming a ref is a "user is driving selection" signal. + let args = strs(&["abc123def456"]); + assert!(!should_add_no_merges_default(&args, false, false)); + } + + #[test] + fn test_no_merges_skipped_with_graph_flag() { + // `git log --graph` — the user explicitly wants topology. Dropping + // merges hides exactly what they came to see. + let args = strs(&["--graph"]); + assert!(!should_add_no_merges_default(&args, false, false)); + } + + #[test] + fn test_no_merges_skipped_with_graph_and_oneline() { + // `git log --graph --oneline` from #2009 — both signals present. + let args = strs(&["--graph", "--oneline"]); + assert!(!should_add_no_merges_default(&args, true, false)); + } + + #[test] + fn test_no_merges_skipped_with_explicit_merges() { + // `git log --merges` — user explicitly asked for merge commits; + // adding `--no-merges` would directly contradict that. + let args = strs(&["--merges"]); + assert!(!should_add_no_merges_default(&args, false, false)); + } + + #[test] + fn test_no_merges_skipped_with_min_parents() { + // `git log --min-parents=2` — semantic alias for "merges only". + let args = strs(&["--min-parents=2"]); + assert!(!should_add_no_merges_default(&args, false, false)); + } + + #[test] + fn test_no_merges_still_added_with_author_filter() { + // `git log --author=foo` — author filter is a flag, not a positional + // pin. RTK is still mostly driving (no format, no limit, no graph), + // so the token-saving default stays. This documents the intentional + // narrow scope of the fix: filters that don't pick a single commit + // do not need to defeat --no-merges. + let args = strs(&["--author=foo"]); + assert!(should_add_no_merges_default(&args, false, false)); + } + + #[test] + fn test_no_merges_skipped_with_max_count_arg() { + // `git log --max-count=5` — has_limit_flag short-circuits even + // before we look at positional args. + let args = strs(&["--max-count=5"]); + assert!(!should_add_no_merges_default(&args, false, true)); + } + #[test] fn test_filter_log_output_token_savings() { fn count_tokens(text: &str) -> usize {