fix(grep): allow hyphens in extension-less file paths - #2202
Open
albatrossflyon-coder wants to merge 2 commits into
Open
fix(grep): allow hyphens in extension-less file paths#2202albatrossflyon-coder wants to merge 2 commits into
albatrossflyon-coder wants to merge 2 commits into
Conversation
Fixes dandavison#2144 The fallback heuristic regex used when a grep-matched file has no recognizable extension (GREP_LINE_REGEX_ASSUMING_NO_INTERNAL_SEPARATOR_CHARS) forbade ':', '-', and '=' anywhere inside the file path, on the assumption that those are always separator characters. Real paths routinely contain hyphens (e.g. foo-bar), which caused the path to be split at the hyphen instead of the real ':' separator -- 'foo-bar:content' misparsed as path='foo', producing 'foo:bar:content' in the rendered output. Adds a new regex tier (WithoutFileExtension), tried after the extension-based regexes and before the strictest fallback, whose path class only forbids ':' (matching what the extension-based regexes already permit) rather than forbidding '-'/'=' as well. Since the middle path class can never consume a literal colon, the parse is still unambiguous: the path is forced to end at the real ':' separator regardless of how many hyphens it contains. This also un-ignores test_parse_grep_n_match_file_name_with_dashes_and_no_extension, a pre-existing test in this file marked '#[ignore] // This fails: we can't parse it currently' -- the same bug class, now fixed by the same change. Added a second regression test for the exact no-line-number repro from the issue. Verified: cargo test --workspace -- 416 passed, 7 ignored (were 415/8 before), zero failures. cargo fmt --check clean. cargo clippy currently fails on this repo, but only on pre-existing issues in unrelated files (wrapping.rs, git_config/mod.rs, subcommands/diff.rs, utils/process.rs) -- confirmed via git diff --stat that this PR touches only src/handlers/grep.rs.
…ches Fixes dandavison#2042 rg --json -l (and plain rg -l/--files-with-matches) ignores --json entirely and emits a bare path per line, with no separator and no code/match content at all. Delta's grep-line heuristic regexes assume every line has a path+separator+code structure, so a bare filename that happens to contain a hyphen (e.g. 'test-1') gets misparsed as path='test' + separator '-' + code='1', corrupting the output ('test-1' -> 'test:1'). This is fundamentally different from dandavison#2144/dandavison#2202: there the ambiguity was resolvable from the string alone once colons were treated as an unambiguous anchor. Here there is no separator character in the input at all -- the string is just a path, full stop. No regex heuristic can correctly infer that from the bare text; it requires knowing that -l was passed. To fix this, CallingProcess::OtherGrep now carries a CommandLine (mirroring GitGrep, which already tracks options this way for the -h/--no-filename fix in dandavison#2202/dandavison#2194). This makes rg's own flags visible to the grep handler for the first time. When -l or --files-with-matches is detected, parse_grep_line now treats the whole line as a bare path (LineType::FileHeader, previously defined but never actually constructed anywhere) instead of attempting separator-based parsing at all. Verified with a real reproduction: rg --json -l genuinely emits plain filenames, not JSON (confirmed empirically), so this path is real, not theoretical. Since the underlying process-detection couldn't be reproduced live in this environment, verification is via a deterministic unit test using delta's own FakeParentArgs test harness (the same mechanism delta's own test suite uses to avoid depending on live process introspection). Testing: - cargo test --workspace: 417 passed, 7 ignored, 0 failed (no regressions; two new tests: the -l/CommandLine behavior in handlers::grep, and calling-process detection for 'rg -l' in utils::process). - cargo fmt --check: clean. - cargo clippy: pre-existing repo-wide failures only (git_config/mod.rs, subcommands/diff.rs, features/side_by_side.rs, handlers/diff_header.rs, handlers/merge_conflict.rs, options/set.rs, wrapping.rs, features/hyperlinks.rs, and 4 needless-borrow hits in utils/process.rs at lines unrelated to this change) -- confirmed via git diff --stat that this PR only touches src/handlers/grep.rs and src/utils/process.rs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2144
Root cause
The fallback heuristic regex used when a grep-matched file has no recognizable extension (
GREP_LINE_REGEX_ASSUMING_NO_INTERNAL_SEPARATOR_CHARS) forbids:,-, and=anywhere inside the file path, on the assumption that those are always separator characters. Real paths routinely contain hyphens (e.g.foo-bar), which caused the path to be split at the hyphen instead of the real:separator:Fix
Adds a new regex tier (
WithoutFileExtension), tried after the extension-based regexes and before the strictest fallback. Its path character class only forbids:(matching what the extension-based regexes already permit), rather than also forbidding-/=. Since the path can never consume a literal colon, the parse stays unambiguous -- the path is forced to end at the real:separator no matter how many hyphens it contains.This also un-ignores
test_parse_grep_n_match_file_name_with_dashes_and_no_extension, a pre-existing test in this file marked#[ignore] // This fails: we can't parse it currently-- same bug class, fixed by the same change. Added a second regression test covering the exact no-line-number repro from #2144.Testing
cargo test --workspace: 416 passed, 7 ignored (were 415/8 before this change) -- zero failures, zero regressions.cargo fmt --check: clean.cargo clippy: currently fails on this repo, but only on pre-existing issues in files this PR doesn't touch (wrapping.rs,git_config/mod.rs,subcommands/diff.rs,utils/process.rs) -- confirmed viagit diff --statthat this PR only touchessrc/handlers/grep.rs.