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

ls: Unexpected space before stdout in TTY output #6386

Closed
RenjiSann opened this issue May 8, 2024 · 8 comments · Fixed by #6402
Closed

ls: Unexpected space before stdout in TTY output #6386

RenjiSann opened this issue May 8, 2024 · 8 comments · Fixed by #6402
Labels

Comments

@RenjiSann
Copy link
Contributor

RenjiSann commented May 8, 2024

I was fixing TODOs in the testsuite to check TTY outputs.

And I realized that there is a seemingly unexpected character (presumably a space) that gets printed.

❯ /bin/ls "$(echo -e 'a\nb')" --hide-control-chars  
'a'$'\n''b'

❯ target/debug/ls "$(echo -e 'a\nb')" --hide-control-chars
 'a'$'\n''b'
^
Unexpected space

The default escaping strategy when outputing to a TTY is shell-escape. Strangely, the bug does not appear when output is not a TTY but with --quoting-style=shell-escape set, as already checked by the testsuite

@RenjiSann
Copy link
Contributor Author

Well maybe it is not exactly a space.
I have written this test:

  scene
      .ucmd()
      .arg("--hide-control-chars")
      .arg("one\ntwo")
      .terminal_simulation(true)
      .succeeds()
      .stdout_only(" 'one'$'\\n''two'\n");

and this still fails, but the diff message is broken:

assertion failed: `(left == right)`

Diff < left / right > :
  'one'$'\n''two'
 

I'll investigate more to find what is this character and try to find where it comes from.

@RenjiSann
Copy link
Contributor Author

RenjiSann commented May 8, 2024

I modified the assertion code to dump the bytes of expected/got stdouts on failure, and I ended up with this:

---- test_ls::test_ls_quoting_and_color stdout ----
touch: /tmp/.tmpyfPDjs/one two
run: /home/renji/projects/coreutils/target/debug/coreutils ls --color one two
run: /home/renji/projects/coreutils/target/debug/coreutils ls --color one two
EXPECTED: [32, 39, 111, 110, 101, 32, 116, 119, 111, 39, 10]
GOT     : [32, 39, 111, 110, 101, 32, 116, 119, 111, 39, 13, 10]
thread 'test_ls::test_ls_quoting_and_color' panicked at tests/by-util/test_ls.rs:2905:10:
explicit panic

So it appears that the prefix character is indeed a 0x20 space, but the code also includes a trailing carriage return \r (0x0D) before the linefeed \n.

I still don't have a clue for why is this happening though.

EDIT:

I keep finding things !
I don't know if it is specific to my computer, but eventhough I don't know why, /bin/ls does include a \r as well, so uu-ls must be right to do so.

For reference:

❯ script -q -c '/bin/ls "$(echo "a\nb")"' | xxd
00000000: 2761 2724 275c 6e27 2762 270d 0a         'a'$'\n''b'..

❯ script -q -c 'target/debug/ls "$(echo "a\nb")"' | xxd
00000000: 2027 6127 2427 5c6e 2727 6227 0d0a        'a'$'\n''b'..

Now what's left is to actually understand WHY a space is inserted here.

@RenjiSann
Copy link
Contributor Author

Found: this is where the space is added.

No searching why is the behavior only triggered by TTY

@RenjiSann
Copy link
Contributor Author

I removed the "adding space if quoted and starts with '" logic here and it didn't break any test.

I am not sure what it is made for, but so far, removing it fixes my problem.

@tertsdiepraam I see you changed this code in 3346b4147aa7c9dd5e5ad822ac927407010eb8de, maybe you'll have some insights.

@tertsdiepraam
Copy link
Member

For anyone else, link to that commit: 3346b41

Seems like I made a mistake? The logic should be that there's a space if the quoting style does not add a quote, but I seem to have flipped that behaviour? That's not good :)

Negating the condition should fix it. But, we also need to think about when it's quoted with " maybe. We might need to come up with a more robust check. Do you want to give that a shot?

@RenjiSann
Copy link
Contributor Author

For anyone else, link to that commit: 3346b41

Seems like I made a mistake? The logic should be that there's a space if the quoting style does not add a quote, but I seem to have flipped that behaviour? That's not good :)

Negating the condition should fix it. But, we also need to think about when it's quoted with " maybe. We might need to come up with a more robust check. Do you want to give that a shot?

I can, but I don't have all the context. I don't know in what case a space is required in the first place.
Would you have an example to share, or some doc that talk about this behavior ?

@tertsdiepraam
Copy link
Member

It's just an alignment thing. Here's an example using GNU:

> ls
'bla bla'   Cargo.lock   CODE_OF_CONDUCT.md   deny.toml        docs   GNUmakefile   Makefile        oranda.json   renovate.json   target   util
 build.rs   Cargo.toml   CONTRIBUTING.md      DEVELOPMENT.md   fuzz   LICENSE       Makefile.toml   README.md     src             tests

Note how all the files have a space to compensate for not having quotes and that 'bla bla' doesn't have a space.

@RenjiSann
Copy link
Contributor Author

Oh, I think I get it.

I'll give it a try !

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