Should nnn -p - emit file details to stdout?

[hovsater]

Environment details (Put x in the checkbox along with the information)

• Operating System: macOS, 11.3.1
• Desktop Environment: macOS
• Terminal Emulator: Kitty, 0.20.3
• Shell: bash, 5.1.8
• Custom desktop opener (if applicable):
• Program options used: -p -
• Configuration options set:
• Issue exists on nnn master

Exact steps to reproduce the issue

When providing -p -, my assumption was that file selection would output to stdout, but when pressing f to get file details, this output is also outputted to stdout, which is kind of unexpected. If I instead, provide a regular file to -p like so nnn -p output.txt, pressing f does not output into it.

1. nnn -p -
2. Press f
3. Press enter
4. See output below

16777220 24380305 -rw-r--r-- 1 kevinsjoberg wheel 0 0 "Jun 10 18:14:55 2021" "Jun 10 18:14:55 2021" "Jun 10 18:14:55 2021" "Jun 10 18:14:55 2021" 4096 0 0 /private/tmp/test.txt


  empty

  inode/x-empty; charset=binary

/private/tmp/test.txt

1. nnn -p output.txt
2. Press f
3. Press enter
4. See output below

/private/tmp/test.txt

[jarun]

When providing -p -, my assumption was that file selection would output to stdout

That's correct!

but when pressing f to get file details, this output is also outputted to stdout, which is kind of unexpected

No, this output is paged, nnn captures the output of file and shows it in PAGER. If not, please provide a screenshot or video of your observation.

If I instead, provide a regular file to -p like so nnn -p output.txt, pressing f does not output into it.

Because file writes to stdout, it's not aware of the file you provided. However, as I mentioned above, nnn pages the file output.

[jarun]

Relevant code: https://github.com/jarun/nnn/blob/master/src/nnn.c#L4175

[0xACE]

Hejsan Kevin, nice catch :)

from 8238410d4c6c1164f6dcf2dadc834e1a2e0b9d9a (master) I can recreate this issue.

$ nnn -p - | sed 's/^/CAPTURED STDOUT:/
CAPTURED STDOUT:  File: nnn/master/.gitignore
CAPTURED STDOUT:  Size: 8               Blocks: 8          IO Block: 4096   regular file
CAPTURED STDOUT:Device: 802h/2050d      Inode: 44717757    Links: 1
CAPTURED STDOUT:Access: (0644/-rw-r--r--)  Uid: ( 1000/     ace)   Gid: ( 1000/     ace)
CAPTURED STDOUT:Access: 2021-02-15 18:12:05.647931819 +0100
CAPTURED STDOUT:Modify: 2020-07-27 00:44:35.629162072 +0200
CAPTURED STDOUT:Change: 2020-07-27 00:44:35.629162072 +0200
CAPTURED STDOUT: Birth: 2020-07-27 00:44:35.629162072 +0200
CAPTURED STDOUT:
CAPTURED STDOUT:
CAPTURED STDOUT:  ASCII text
CAPTURED STDOUT:
CAPTURED STDOUT:  text/plain; charset=us-ascii
CAPTURED STDOUT:
CAPTURED STDOUT:nnn/master/CHANGELOG
CAPTURED STDOUT:nnn/master/.git
CAPTURED STDOUT:nnn/master/.gitignore

$ env | grep '\(LESS\|PAGE\)'`
LESS=-Ri

I'm replying via phone that is connected to my pc via VNC and my connection is spotty so i can't delve into this atm but a quick guess would be that we need to fork and then hijack the STDOUT before we exec the PAGER inplace. I'm not even sure how we handle the -p situation as it is today, but I get the feeling there might be a easier solution... (Jarun already analyzed this so I hope that I didn't misunderstand the topic. If I did I apologize for bumping a closed thread.)

[jarun]

I still see this in the pager in a temp file:

  File: /home/vaio/GitHub/nnn/CHANGELOG
  Size: 39910           Blocks: 80         IO Block: 4096   regular file
Device: 802h/2050d      Inode: 2364778     Links: 1
Access: (0664/-rw-rw-r--)  Uid: ( 1001/    vaio)   Gid: ( 1001/    vaio)
Access: 2021-06-10 00:02:27.705364528 +0530
Modify: 2021-06-09 23:05:19.789203113 +0530
Change: 2021-06-09 23:05:19.789203113 +0530
 Birth: -

/tmp/.nnnWrz7hN (END)

[annagrram]

@jarun Yeah. Me too.
But I managed to see it more clearly by using nnn -p - > foo and then when pressing f nothing would happen because the STDOUT was redirected to the file.

[jarun]

I don't have a solution for this, or maybe the time to explore a solution of this. Sorry.
Leaving it open for someone who has time to work on this.

[jarun]

BTW, I understand you guys are seeing it when you redirect/pipe the output from nnn. I don't think there's a way to block the stdout and stderr of a process and get the output in a pipe. So we just dup2 stdout and strerr. That's why when you redirect/pipe you see the std* output in the file.

I see the following:

~/GitHub/nnn$ nnn -p hello // pressed f on a file
1 ~/GitHub/nnn$ cat hello
/home/vaio/GitHub/nnn/CHANGELOG
1 ~/GitHub/nnn$ nnn -p - // pressed f on a file
/home/vaio/GitHub/nnn/CHANGELOG
1 ~/GitHub/nnn$

So, @kevinsjoberg can you please confirm if you see the file details in the terminal when you run nnn -p - (no piping/redirection)?

If yes, please share an asciicast.

[hovsater]

@jarun I can confirm that everything works as expected when there's no redirection. It seems like it's only a problem when stdout is redirected.

[jarun]

Then do not redirect.
Why do you need to redirect when you have nnn -p output_file for the same purpose?

[annagrram]

I don't know if that's really a problem.
You ask nnn to redirect its selections to stdout; nnn uses a pager to show the stats of files and the pager also uses stdout. They are bound to step on each other's toes.

[jarun]

My first problem is - the redirection was completely missing from the issue description. Why? Others explored possibilities with redirection so we figured this out. Otherwise, I wouldn't even bother to try redirection because your steps were straight and should be reproducible easily. I was going to download kitty before I saw that you mentioned you redirected.

Please try to be accurate when you raise issues.

[annagrram]

So I think the only solution is to show the stats on a curses screen instead of external pager. But, that's for @jarun to decide. (Well, or to use stderr for external spawning, which is also for @jarun to decide)

[jarun]

I don't know if that's really a problem.

No there is no problem. The only problem here is user is using redirection for -p when it is clearly documented you can use a file to do that.

So I think the only solution is to show the stats on a curses screen

This is not an isolated use case. Paged output is necessary for large output on smaller screens... mediainfo output, for example. For all the reasons people prefer a pager.

[annagrram]

Technically,

diff --git a/src/nnn.c b/src/nnn.c
index c7347e6..00894ea 100644
--- a/src/nnn.c
+++ b/src/nnn.c
@@ -2081,7 +2081,9 @@ static int spawn(char *file, char *arg1, char *arg2, char *arg3, uchar_t flag)
                        dup2(fd, 1);
                        dup2(fd, 2);
                        close(fd);
-               }
+        } else {
+            dup2(2, 1);
+        }
 
                execvp(*argv, argv);
                _exit(EXIT_SUCCESS);

solves this issue, but I didn't check if it causes others nor whether it's a viable solution.

I always forget to clear my expandtab when working on nnn code :/

[hovsater]

@jarun sorry, I should've probably shared some backstory.

I'm using dte as my primary editor and it supports exec-open for opening files. See https://gitlab.com/craigbarnes/dte/-/blob/master/src/commands.c#L465 for its implementation. It will read from stdout and open the matching paths.

I'm not explicitly redirecting stdout.

[jarun]

solves this issue, but I didn't check if it causes others nor whether it's a viable solution.

Does this mean all child processes with un-suppressed output will write to stderr instead of stdout?

[annagrram]

@jarun sorry, I should've probably shared some backstory.

I'm using dte as my primary editor and it supports exec-open for opening files. See https://gitlab.com/craigbarnes/dte/-/blob/master/src/commands.c#L465 for its implementation. It will read from stdout and open the matching paths.

In that case I would use something like nnn -p /tmp/sel && cat /tmp/sel | dte

Does this mean all child processes with un-suppressed output will write to stderr instead of stdout?

This specific diff will make all spawned process which are not silenced to redirect their stdout to stderr.
I've never said it's a good solution, just that it is a solution for the current scenario.

[jarun]

I think we have a working design that covers this case (nnn -p file).

I wouldn't want to do anything that universally affects all spawned cli utilities.

[hovsater]

In that case I would use something like nnn -p /tmp/sel && cat /tmp/sel | dte

For that I could just use nnn -p - | dte. My hope was to get it working with exec-open.

In any case, I totally understand if this is a use case not worth supporting and if so, I'll see if I can create a shell wrapper instead.

[hovsater]

@jarun @annagrram quick follow up. I managed to get everything working by wrapping nnn with the following shell script:

#!/bin/sh
set -eu

TMPFILE="$(mktemp)"
trap "rm -f '$TMPFILE'" INT QUIT TERM HUP EXIT
nnn -p "$TMPFILE" > /dev/tty
cat "$TMPFILE"

Everything now works as expected for particular use case. 🙂

[jarun]

Thanks for the update!

[hovsater]

@jarun turns out there's actually something that can be addressed in nnn if you deem it worth it. I spoke to Craig (the author of dte) and it looks like we should check isatty(STDOUT_FILENO and if false, use /dev/tty instead of stdout.

See https://gitlab.com/craigbarnes/dte/-/issues/23#note_599738376 for details.

[jarun]

I understand that you are writing a script and want to get it right... now you have to understand that I am a chip guy with a family and silly terminal quirks are as trivial as it gets to me. :)

Please raise the PR or if @annagrram or @KlzXS can take a look I'm fine with it. 👍

[hovsater]

@jarun absolutely. I'm a father of two myself, so I have nothing but respect for you prioritising your time. 🙂 I'm still fairly new to C, but if I manage to get something working, I'll definitely submit a pull request. 👍

[jarun]

Sure thing! I'm re-opening the issue. If we don't have anything in a week, I will add a line item with reference to the issue.

[0xACE]

assigning myself in case no one picks this up ill take a look at this by the end of summer...

[annagrram]

@jarun turns out there's actually something that can be addressed in nnn if you deem it worth it. I spoke to Craig (the author of dte) and it looks like we should check isatty(STDOUT_FILENO and if false, use /dev/tty instead of stdout.

See https://gitlab.com/craigbarnes/dte/-/issues/23#note_599738376 for details.

This relies on the output being piped, but that was what you eventually wanted, so nice idea :)
Please take a look at the PR and see if that works for you.

[jarun]

Fixed at #1070.

[jarun]

@kevinsjoberg please confirm your implementation works fine with commit 25fab4c.