Skip to content

Commit

Permalink
printer: fix --vimgrep for multi-line mode
Browse files Browse the repository at this point in the history
It turned out that --vimgrep wasn't quite getting the column of each
match correctly. Instead of printing column numbers relative to the
current line, it was printing column numbers as byte offsets relative to
where the match began. To fix this, we simply subtract the offset of the
line number from the beginning of the match. If the beginning of the
match came before the start of the current line, then there's really
nothing sensible we can do other than to use a column number of 1, which
we now document.

Interestingly, existing tests were checking that the previous behavior
was intended. My only defense is that I somehow tricked myself into
thinking it was a byte offset instead of a column number.

Kudos to @bfrg for calling this out in #1866:
#1866 (comment)
  • Loading branch information
BurntSushi committed May 15, 2021
1 parent 2af7724 commit 94e4b8e
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 8 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ Bug fixes:
Document cygwin path translation behavior in the FAQ.
* [BUG #1741](https://github.com/BurntSushi/ripgrep/issues/1741):
Fix stdin detection when using PowerShell in UNIX environments.
* [BUG #1866](https://github.com/BurntSushi/ripgrep/issues/1866#issuecomment-841635553):
Fix bug when computing column numbers in `--vimgrep` mode.


12.1.1 (2020-05-29)
Expand Down
16 changes: 9 additions & 7 deletions crates/printer/src/standard.rs
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,9 @@ impl StandardBuilder {
/// When multi-line mode is enabled, each match and its accompanying lines
/// are printed. As with single line matches, if a line contains multiple
/// matches (even if only partially), then that line is printed once for
/// each match it participates in.
/// each match it participates in. In multi-line mode, column numbers only
/// indicate the start of a match. If a line only continues a match, then
/// the column number printed is always `1`.
pub fn per_match(&mut self, yes: bool) -> &mut StandardBuilder {
self.config.per_match = yes;
self
Expand Down Expand Up @@ -1090,7 +1092,7 @@ impl<'a, M: Matcher, W: WriteColor> StandardImpl<'a, M, W> {
self.write_prelude(
self.sunk.absolute_byte_offset() + line.start() as u64,
self.sunk.line_number().map(|n| n + count),
Some(m.start() as u64 + 1),
Some(m.start().saturating_sub(line.start()) as u64 + 1),
)?;
count += 1;
if self.exceeds_max_columns(&bytes[line]) {
Expand Down Expand Up @@ -2990,9 +2992,9 @@ Holmeses, success in the province of detective work must always
let got = printer_contents(&mut printer);
let expected = "\
1:16:For the Doctor Watsons of this world, as opposed to the Sherlock
2:16:Holmeses, success in the province of detective work must always
2:1:Holmeses, success in the province of detective work must always
5:12:but Doctor Watson has to have it taken out for him and dusted,
6:12:and exhibited clearly, with a label attached.
6:1:and exhibited clearly, with a label attached.
";
assert_eq_printed!(expected, got);
}
Expand All @@ -3019,9 +3021,9 @@ Holmeses, success in the province of detective work must always
let got = printer_contents(&mut printer);
let expected = "\
1:16:For the Doctor Watsons of this world, as opposed to the Sherlock
2:16:Holmeses, success in the province of detective work must always
2:123:Holmeses, success in the province of detective work must always
3:123:be, to a very large extent, the result of luck. Sherlock Holmes
2:1:Holmeses, success in the province of detective work must always
2:58:Holmeses, success in the province of detective work must always
3:1:be, to a very large extent, the result of luck. Sherlock Holmes
";
assert_eq_printed!(expected, got);
}
Expand Down
2 changes: 1 addition & 1 deletion tests/multiline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ rgtest!(vimgrep, |dir: Dir, mut cmd: TestCommand| {
let expected = "\
sherlock:1:16:For the Doctor Watsons of this world, as opposed to the Sherlock
sherlock:1:57:For the Doctor Watsons of this world, as opposed to the Sherlock
sherlock:2:57:Holmeses, success in the province of detective work must always
sherlock:2:1:Holmeses, success in the province of detective work must always
sherlock:3:49:be, to a very large extent, the result of luck. Sherlock Holmes
sherlock:5:12:but Doctor Watson has to have it taken out for him and dusted,
";
Expand Down
18 changes: 18 additions & 0 deletions tests/regression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -864,3 +864,21 @@ use B;
]);
eqnice!("2\n", cmd.stdout());
});

rgtest!(r1866, |dir: Dir, mut cmd: TestCommand| {
dir.create("test", "foobar\nfoobar\nfoo quux");
cmd.args(&[
"--multiline",
"--vimgrep",
r"foobar\nfoobar\nfoo|quux",
"test",
]);

let expected = "\
test:1:1:foobar
test:2:1:foobar
test:3:1:foo quux
test:3:5:foo quux
";
eqnice!(expected, cmd.stdout());
});

0 comments on commit 94e4b8e

Please sign in to comment.