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.

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

Revert "cqfd: redirect command stderr to stdout" once again #125

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

Conversation

gportay
Copy link
Contributor

@gportay gportay commented Feb 25, 2024

This reverts commit 0cef50e once again.

The commit e8bc9af has mistakenly reverted the change ae5c830.

@joufellasfl
Copy link
Contributor

Thanks, change is merged. Could we have a test to catch stderr management misbehaviors?

@gportay
Copy link
Contributor Author

gportay commented Feb 25, 2024

Thanks, change is merged. Could we have a test to catch stderr management misbehaviors?

I hesitated to make a test. I will do it.

@joufellasfl
Copy link
Contributor

Thanks, this will help avoiding bringing this issue back to life every now and then :-)

@gportay
Copy link
Contributor Author

gportay commented Feb 27, 2024

Well, writing tests is not that simple; and cqfd does not work as expected.

cqfd run "cat /etc/os-release >&2" 2>/dev/null

This outputs the content of /etc/os-release, while I expected it do not.

This is because a pseudo tty is allocated since stdin is a tty (even if stderr is not).

From bash(1):

An interactive shell is one started without non-option arguments (unless -s is specified) and without the -c option, whose standard input and error are both connected to terminals (as determined by isatty(3)), or one started with the -i option. PS1 is set and $- includes i if bash is interactive, allowing a shell script or a startup file to test this state.

So I would need that change:

diff --git a/cqfd b/cqfd
index 9cff1ad..d888fe5 100755
--- a/cqfd
+++ b/cqfd
@@ -173,7 +173,7 @@ docker_run() {

        # interactive options if standard input is connected to a tty
        args+=(-i)
-       if tty -s; then
+       if [ -t 0 ] && [ -t 2 ]; then
                args+=(-t)
        fi

Note: I did it on dosh, my docker shell wrapper.

joufella pushed a commit that referenced this pull request Feb 28, 2024
This allows a better identification of when a pseudo tty should be
allocated (see #125 for details).
@joufellasfl
Copy link
Contributor

I added the tty -s to test -t change, it doesn't break any test, so either it's safe, or we don't have enough test coverage :). Let's see how it goes having it on master.

@gportay gportay force-pushed the remove-stderr-redirection-once-again branch from d55b763 to cdbe563 Compare February 28, 2024 21:33
@gportay
Copy link
Contributor Author

gportay commented Feb 28, 2024

Oops, it stops to work... I need to fix it. sorry for the noise.

**|FAIL|cqfd without redirection should allocate a tty
**|FAIL|cqfd with stdin redirected to /dev/null should not allocate a pty
**|FAIL|cqfd with stderr redirected to /dev/null should not allocate a pty
  |PASS|cqfd with allocated pty should redirect stdout and stderr to same endpoint
**|FAIL|cqfd without allocated pty should redirect stdout and stderr to distinct entpoints

EDIT; I have move the test file to the end; so I guess a test modifies the .cqfdrc file without restoring it.

@gportay gportay force-pushed the remove-stderr-redirection-once-again branch from cdbe563 to 3e07156 Compare February 28, 2024 22:19
@gportay
Copy link
Contributor Author

gportay commented Feb 28, 2024

@joufellasfl, it is done; is it what you expected?

@gportay
Copy link
Contributor Author

gportay commented Apr 25, 2024

Hello, @joufellasfl any though on the tests?

@gportay
Copy link
Contributor Author

gportay commented Aug 13, 2024

Ping?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants