fix: query stdout terminal size to see if the output gose to a tty.

[hehelego]

terminal_size::terminal_size() is used to determine if stdout is connected to a real tty/pty. In previous version of terminal-size, this method only queries the terminal size for stdout. However, it is changed in the following commit
eminence/terminal-size@08f0e73 Now the method will use stdout, stdin, stderr, trying its best to determine the terminal size. This cannot be used to determine if eza output is piped or redirected.
We should use terminal_size_using_fd on stdout.as_raw_fd instead.
This patch makes eza --color=auto behaves like ls --color=auto.

Resolves #434 Fixes #433

Also Resolves #441 as well.

[PThorpe92]

This looks good, Thanks for the contribution!

I see that this causes some issues on Windows though, take a look at the test results and it shouldn't be too hard to set up a solution. If you don't have access to a windows machine, I can try to grab one from work tomorrow and figure it out.

[hehelego]

Member

Sorry, I don't have a machine with MS Windows installed. With that said, I can look into the document of terminal-size and try to fix them.

[PThorpe92]

It looks like it even suggests a solution in the error output. No worries, I can get a windows box tomorrow and I'm sure it's just specifying a windows specific method. Shouldn't be too tricky.

[hehelego]

CI/CD tasks passed, but I am uncertain about whether this works on real windows machine.

[cafkafk]

The code looks good to me, and testing seems to show it as working.

I'd prefer if we first got a windows user to test it out as well. Also, from #433 I got the impression that this wasn't documented that well, maybe it would be a good idea to update completions, man page, and readme to reflect this behavior.

Also, I'd like to pair this with a test case. I'm assuming something like testing eza --color=automatic --online > eza-out; cat ./eza-out (as mentioned in #433) and ensuring it doesn't change following this PR would be an option, but I wonder if you had more ideas.

[hehelego]

Hi @cafkafk , thanks for pointing out these needy updates. I didn't realize the need for including new testcases and improving documentation.
I'd like to contribute more code/docs and push this forward. I can help revise the man page / shell completion tomorrow.
However, I am unfamiliar with the way you do functionality-level testing (the things in tests/cmd/). I'll have to spend some time doing it.

[cafkafk]

Hi @cafkafk , thanks for pointing out these needy updates. I didn't realize the need for including new testcases and improving documentation. I'd like to contribute more code/docs and push this forward. I can help revise the man page / shell completion tomorrow. However, I am unfamiliar with the way you do functionality-level testing (the things in tests/cmd/). I'll have to spend some time doing it.

No worries, take as much time as you need! If you have any questions, feel free to ask here, or pop into the matrix chat.

[daviessm]

@hehelego I'm just checking this out on Windows. As far as I can tell, redirecting to a file with and without --color=auto works as I'd expect. It also worked before though, were we expecting to see any difference on Windows? I may be looking at the wrong thing 😎

[hehelego]

@hehelego I'm just checking this out on Windows. As far as I can tell, redirecting to a file with and without --color=auto works as I'd expect. It also worked before though, were we expecting to see any difference on Windows? I may be looking at the wrong thing 😎

Hi @daviessm , thanks for testing it on Windows. I am not sure what is the behavior on windows before this patch. But as long as this patch works as expected, I think we are fine.

I spotted the unexpected behavior on Linux platform (see the related issue threads for details). I tried to correct the bug by query terminal_size_using_fd of stdout.as_raw_fd, however this dose not work for windows (they use handles instead of file descriptors). Later on, I tried to use terminal_size_using_handle on STD_OUTPUT_HANDLE, expecting that to work on windows.
We were waiting to test if that actually works on real Windows installation.

[daviessm]

Yeah, looks like the original code always worked for Windows and this doesn't break it so LGTM for Windows 👍

[hehelego]

BTW, I fish shell has a special builtin function fish_update_completions which extracts completions from manpages.
If we are keeping the manpage exa(1) up to date, there is no need to keep a separated and synchronized fish completion

[hehelego]

Yeah, looks like the original code always worked for Windows and this doesn't break it so LGTM for Windows 👍

Great. I'll keep working on manpages improvement and test case introduction.

[daviessm]

BTW, I fish shell has a special builtin function fish_update_completions which extracts completions from manpages. If we are keeping the manpage exa(1) up to date, there is no need to keep a separated and synchronized fish completion

Yep, once the clap PR #197 gets merged we can autogenerate completions for multiple shells from that - see #400 (comment)

[hehelego]

I'll need some help. Looks like trycmd dose not support complicated CLI testing involving shell, redirection and pipeline.
We may need something like snapbox for that.

See assert-rs/snapbox#168 for related discussion. The OP switched to snapbox eventually.

[PThorpe92]

Interesting, I've never heard of it. I'm going to turn this into a discussion post, and look into snapbox.

Feel free to come by the matrix channel if you want, we can see about how to proceed here particularly

[hehelego]

Wait, I think I've mis-read that issue. They actually build another testing framework based on snapbox.
I've just looked into the document of snapbox and seems no easy way to test IO redirecting and pipelining..
Sorry for that.

[hehelego]

I think we can move forward to merge this PR first. Finding a proper way to test eza's behavior when redirection and pipelining are involved should be a separate task.

[cfxegbert]

Why not use the trait IsTerminal?

[hehelego]

Why not use the trait IsTerminal?

Thanks for pointing it our, exactly the thing we need.

[hehelego]

I think we can move forward to merge this PR first. Finding a proper way to test eza's behavior when redirection and pipelining are involved should be a separate task.

Update: I've come up with a plan to test this using snapbox. I'll try to implement it soon.
We can merge this PR after the test are available.

[daviessm]

I'll give the new code a whiz on Windows later today.

[daviessm]

Windows still looks good

[hehelego]

Yep. I find another API misuse:

eza/src/output/mod.rs

Line 56 in afe832b

Self::Automatic => terminal_size::terminal_size().map(|(w, _)| w.0.into()),

The comment shows that we want only to query stdout, but that is not what terminal_size::terminal_size() does.

[hehelego]

I cannot test this with std::process:Command or snapbox::cmd::Command. They use pipe to capture stdout of the spawned process. I have no way to get the "normal" output (when you directly run eza from a shell session).

You may try this code snippet:

    use std::io::Write;
    let out = std::process::Command::new("bash")
        .args(["-c", "/bin/ls --color=auto"])
        .output()
        .unwrap()
        .stdout;
    std::io::stdout().write_all(&out).unwrap();

ls will not colorize the output.

Does anyone have any idea on how should we do this?

[hehelego]

Alright, this thread is becoming too long to read. I shall give a quick summary of the current progress:

• An API misuse is causing all this. terminal_size::terminal_size() queries the terminal size of stdout, stdin, stderr while we only want size of the terminal connected to stdout.
• We've replaced the calls with terminal size query on stdout only.
• We want to set up several tests to ensure that eza dose the expected behavior. However we currently have no way to test --color=auto: In any case, we have to somehow capture the output of the spawned eza process, which disables terminal color.

[cafkafk]

I can't push to your branch for some reason, but I rebased and added a breaking notifiers (! and BREAKING CHANGE:) to the first commit.

commit 9a3b1cbd66a07ae0208b99418db0bb2598303582
Author: hehelego <[email protected]>
Date:   Wed Sep 27 18:05:39 2023 -0500

    fix!: query stdout terminal size to see if the output goes to a tty.
    
    `terminal_size::terminal_size()` is used to determine if stdout is
    connected to a real tty/pty. In previous version of `terminal-size`,
    this method only queries the terminal size for `stdout`.
    However, it is changed in the following commit
    https://github.com/eminence/terminal-size/commit/08f0e73926c11adc3105dbf4eb84dd8c9e6c873a
    Now the method will use `stdout, stdin, stderr`, trying its best to
    determine the terminal size. This cannot be used to determine if `eza`
    output is piped or redirected.
    We should use `terminal_size_using_fd` on `stdout.as_raw_fd` instead.
    
    Resolves #434 Fixes #433
    
    BREAKING CHANGE: This is a change in behaviour from before, and so we
    have marked it as breaking. In practice, it probably won't break
    anything except if you're doing something really weird (more power to you!).

You need to do that, and this should be good to ship today

[cafkafk]

I'm just gonna fix this after merge, still needs a follow up with tests