Skip to content
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

Jump to bottom

Adding line number printing when output is piped out #2983

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Adding line number printing when output is piped out #2983

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
bb411ab
Adding line number printing when output is piped out
May 31, 2024
debf7c3
Added CHANGELOG entry
Jun 1, 2024
a3234bf
cargo fmt --check
Jun 1, 2024
5727ccb
Fixed file_with_invalid_utf8_filename integration test
Jun 1, 2024
de19b52
Merge branch 'master' into 2935-compatibility-issue-with-cat-piping
domenicomastrangelo Jun 1, 2024
1eec98b
Add check for non-terminal/tty handle
Jun 1, 2024
9523e40
Merge branch '2935-compatibility-issue-with-cat-piping' of github.com…
Jun 1, 2024
76832ef
cargo fmt
Jun 1, 2024
8ad55ab
Trigger Build
Jun 1, 2024
320f42b
Added current line number prefix
Jun 2, 2024
f96427f
Added current line number prefix
Jun 1, 2024
c0c1fd6
Merge branch '2934-compatibility-issue-with-cat-piping' of github.com…
Jun 2, 2024
2ba6813
Added switch to plain style as default:w in case of loop_through = true
Jun 6, 2024
1f4d6f0
Changed default to be Plain in case of loop_through
Jun 6, 2024
b13049a
Merge branch 'master' into 2935-compatibility-issue-with-cat-piping
domenicomastrangelo Nov 9, 2024
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
- Fix handling of inputs with OSC ANSI escape sequences, see #2541 and #2544 (@eth-p)
- Fix handling of inputs with combined ANSI color and attribute sequences, see #2185 and #2856 (@eth-p)
- Fix panel width when line 10000 wraps, see #2854 (@eth-p)
- Fix compatibility issue with cat. see #2983 (@domenicomastrangelo)

## Other

Expand Down
13 changes: 11 additions & 2 deletions src/printer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ impl<'a> Printer for SimplePrinter<'a> {
&mut self,
out_of_range: bool,
handle: &mut OutputHandle,
_line_number: usize,
line_number: usize,
line_buffer: &[u8],
) -> Result<()> {
// Skip squeezed lines.
Expand Down Expand Up @@ -163,7 +163,16 @@ impl<'a> Printer for SimplePrinter<'a> {
write!(handle, "{line}")?;
} else {
match handle {
OutputHandle::IoWrite(handle) => handle.write_all(line_buffer)?,
OutputHandle::IoWrite(handle) => {
if self.config.style_components.numbers() {
handle.write_all(
format!("{line_number:4} {}", String::from_utf8_lossy(line_buffer))
.as_bytes(),
)?;
} else {
handle.write_all(line_buffer)?;
}
}
OutputHandle::FmtWrite(handle) => {
write!(
handle,
Expand Down
60 changes: 30 additions & 30 deletions tests/integration_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ fn basic() {
.arg("test.txt")
.assert()
.success()
.stdout("hello world\n")
.stdout(" 1 hello world\n")
.stderr("");
}

Expand All @@ -58,7 +58,7 @@ fn stdin() {
.write_stdin("foo\nbar\n")
.assert()
.success()
.stdout("foo\nbar\n");
.stdout(" 1 foo\n 2 bar\n");
}

#[test]
Expand All @@ -68,7 +68,7 @@ fn concatenate() {
.arg("test.txt")
.assert()
.success()
.stdout("hello world\nhello world\n");
.stdout(" 1 hello world\n 1 hello world\n");
}

#[test]
Expand All @@ -80,7 +80,7 @@ fn concatenate_stdin() {
.write_stdin("stdin\n")
.assert()
.success()
.stdout("hello world\nstdin\nhello world\n");
.stdout(" 1 hello world\n 1 stdin\n 1 hello world\n");
}

#[test]
Expand All @@ -90,7 +90,7 @@ fn concatenate_empty_first() {
.arg("test.txt")
.assert()
.success()
.stdout("hello world\n");
.stdout(" 1 hello world\n");
}

#[test]
Expand All @@ -100,7 +100,7 @@ fn concatenate_empty_last() {
.arg("empty.txt")
.assert()
.success()
.stdout("hello world\n");
.stdout(" 1 hello world\n");
}

#[test]
Expand All @@ -121,7 +121,7 @@ fn concatenate_empty_between() {
.arg("test.txt")
.assert()
.success()
.stdout("hello world\nhello world\n");
.stdout(" 1 hello world\n 1 hello world\n");
}

#[test]
Expand All @@ -132,7 +132,7 @@ fn concatenate_empty_first_and_last() {
.arg("empty.txt")
.assert()
.success()
.stdout("hello world\n");
.stdout(" 1 hello world\n");
}

#[test]
Expand All @@ -142,7 +142,7 @@ fn concatenate_single_line() {
.arg("single-line.txt")
.assert()
.success()
.stdout("Single LineSingle Line");
.stdout(" 1 Single Line 1 Single Line");
}

#[test]
Expand All @@ -153,7 +153,7 @@ fn concatenate_single_line_empty() {
.arg("single-line.txt")
.assert()
.success()
.stdout("Single LineSingle Line");
.stdout(" 1 Single Line 1 Single Line");
}

#[test]
Expand All @@ -174,7 +174,7 @@ fn line_range_2_3() {
.arg("--line-range=2:3")
.assert()
.success()
.stdout("line 2\nline 3\n");
.stdout(" 2 line 2\n 3 line 3\n");
}

#[test]
Expand All @@ -184,7 +184,7 @@ fn line_range_first_two() {
.arg("--line-range=:2")
.assert()
.success()
.stdout("line 1\nline 2\n");
.stdout(" 1 line 1\n 2 line 2\n");
}

#[test]
Expand All @@ -194,7 +194,7 @@ fn line_range_last_3() {
.arg("--line-range=2:")
.assert()
.success()
.stdout("line 2\nline 3\nline 4\n");
.stdout(" 2 line 2\n 3 line 3\n 4 line 4\n");
}

#[test]
Expand All @@ -205,7 +205,7 @@ fn line_range_multiple() {
.arg("--line-range=4:4")
.assert()
.success()
.stdout("line 1\nline 2\nline 4\n");
.stdout(" 1 line 1\n 2 line 2\n 4 line 4\n");
}

#[test]
Expand All @@ -215,7 +215,7 @@ fn squeeze_blank() {
.arg("--squeeze-blank")
.assert()
.success()
.stdout("line 1\n\nline 5\n\nline 20\nline 21\n\nline 24\n\nline 26\n\nline 30\n");
.stdout(" 1 line 1\n 2 \n 5 line 5\n 6 \n 20 line 20\n 21 line 21\n 22 \n 24 line 24\n 25 \n 26 line 26\n 27 \n 30 line 30\n");
}

#[test]
Expand All @@ -238,15 +238,15 @@ fn squeeze_limit() {
.arg("--squeeze-limit=2")
.assert()
.success()
.stdout("line 1\n\n\nline 5\n\n\nline 20\nline 21\n\n\nline 24\n\nline 26\n\n\nline 30\n");
.stdout(" 1 line 1\n 2 \n 3 \n 5 line 5\n 6 \n 7 \n 20 line 20\n 21 line 21\n 22 \n 23 \n 24 line 24\n 25 \n 26 line 26\n 27 \n 28 \n 30 line 30\n");

bat()
.arg("empty_lines.txt")
.arg("--squeeze-blank")
.arg("--squeeze-limit=5")
.assert()
.success()
.stdout("line 1\n\n\n\nline 5\n\n\n\n\n\nline 20\nline 21\n\n\nline 24\n\nline 26\n\n\n\nline 30\n");
.stdout(" 1 line 1\n 2 \n 3 \n 4 \n 5 line 5\n 6 \n 7 \n 8 \n 9 \n 10 \n 20 line 20\n 21 line 21\n 22 \n 23 \n 24 line 24\n 25 \n 26 line 26\n 27 \n 28 \n 29 \n 30 line 30\n");
}

#[test]
Expand Down Expand Up @@ -693,7 +693,7 @@ fn do_not_exit_directory() {
.arg("sub_directory")
.arg("test.txt")
.assert()
.stdout("hello world\n")
.stdout(" 1 hello world\n")
.failure();
}

Expand Down Expand Up @@ -752,7 +752,7 @@ fn pager_disable() {
.arg("test.txt")
.assert()
.success()
.stdout(predicate::eq("hello world\n").normalize());
.stdout(predicate::eq(" 1 hello world\n").normalize());
}

#[test]
Expand Down Expand Up @@ -833,7 +833,7 @@ fn env_var_pager_value_bat() {
.arg("test.txt")
.assert()
.success()
.stdout(predicate::eq("hello world\n").normalize());
.stdout(predicate::eq(" 1 hello world\n").normalize());
}

#[test]
Expand Down Expand Up @@ -871,7 +871,7 @@ fn pager_most_from_pager_env_var() {
.arg("test.txt")
.assert()
.success()
.stdout(predicate::eq("hello world\n").normalize());
.stdout(predicate::eq(" 1 hello world\n").normalize());
});
}

Expand Down Expand Up @@ -917,7 +917,7 @@ fn pager_most_with_arg() {
.arg("test.txt")
.assert()
.success()
.stdout(predicate::eq("hello world\n").normalize());
.stdout(predicate::eq(" 1 hello world\n").normalize());
});
}

Expand All @@ -932,7 +932,7 @@ fn pager_more() {
.arg("test.txt")
.assert()
.success()
.stdout(predicate::eq("hello world\n").normalize());
.stdout(predicate::eq(" 1 hello world\n").normalize());
});
}

Expand All @@ -944,7 +944,7 @@ fn alias_pager_disable() {
.arg("test.txt")
.assert()
.success()
.stdout(predicate::eq("hello world\n").normalize());
.stdout(predicate::eq(" 1 hello world\n").normalize());
}

#[test]
Expand All @@ -971,7 +971,7 @@ fn disable_pager_if_disable_paging_flag_comes_after_paging() {
.arg("test.txt")
.assert()
.success()
.stdout(predicate::eq("hello world\n").normalize());
.stdout(predicate::eq(" 1 hello world\n").normalize());
}

#[test]
Expand Down Expand Up @@ -1043,7 +1043,7 @@ fn basic_set_terminal_title() {
.arg("test.txt")
.assert()
.success()
.stdout("\u{1b}]0;bat: test.txt\x07hello world\n")
.stdout("\u{1b}]0;bat: test.txt\x07 1 hello world\n")
.stderr("");
}

Expand Down Expand Up @@ -1294,7 +1294,7 @@ fn can_print_file_named_cache() {
.arg("cache")
.assert()
.success()
.stdout("test\n")
.stdout(" 1 test\n")
.stderr("");
}

Expand All @@ -1305,7 +1305,7 @@ fn can_print_file_named_cache_with_additional_argument() {
.arg("test.txt")
.assert()
.success()
.stdout("test\nhello world\n")
.stdout(" 1 test\n 1 hello world\n")
.stderr("");
}

Expand All @@ -1315,7 +1315,7 @@ fn can_print_file_starting_with_cache() {
.arg("cache.c")
.assert()
.success()
.stdout("test\n")
.stdout(" 1 test\n")
.stderr("");
}

Expand Down Expand Up @@ -1779,7 +1779,7 @@ fn file_with_invalid_utf8_filename() {
.arg(file_path.as_os_str())
.assert()
.success()
.stdout("dummy content\n");
.stdout(" 1 dummy content\n");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm curious, is this really the expected behavior? The test invokes bat without any line number style component arguments. I think it should stay as it was before - according to the issue being solved, line numbers should only be displayed when piping when the command line argument was explicitly passed, no?

Copy link
Author

@domenicomastrangelo domenicomastrangelo Jun 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

good catch!

I added a check for handle type here

Copy link
Author

Choose a reason for hiding this comment

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

this doesn't pass the test now, it seems like there may be an issue with the filename and the is_terminal() function or something like that

fn is_terminal(&self) -> bool

───────────────────────────────────────────
Returns true if the descriptor/handle refers to a terminal/tty.

On platforms where Rust does not know how to detect a terminal yet, this will return
false. This will also return false if an unexpected error occurred, such as from
passing an invalid file descriptor.

Copy link
Author

Choose a reason for hiding this comment

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

debugging it shows self.config.style_components.numbers() returns true, is there a different way to know if line numbers are requested that i'm missing?

Copy link
Contributor

@einfachIrgendwer0815 einfachIrgendwer0815 Jun 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

My guess would be the following:

The SimplePrinter is used whenever stdout is not interactive, --color is not set to always, --decorations is not set to always and --force-colorization is not set either (see app.rs, config.rs and controller.rs). --style controls which style components (such as header or line numbers) are used. The default for --style is changes,grid,header-filename,numbers,snip. This default currently stays the same even if loop_through == true (which means that SimplePrinter will be used). Currently, SimplePrinter just does not care about --style.

So, when loop_through == true, the default for --style should be plain instead. Then your changes should work since they do when you specify --style=plain explicitly.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for the tip @einfachIrgendwer0815!

I updated the code and pushed the changes, now all the tests pass correctly and I removed all the integration test changes which were not necessary.

I also added a test for this specific case :)

}

#[test]
Expand Down