8000 Silently accepts ---presume-input-tty by hbina · Pull Request #2532 · uutils/coreutils · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Silently accepts ---presume-input-tty #2532

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

Merged

Conversation

hbina
Copy link
Contributor
@hbina hbina commented Jul 29, 2021

For whatever reason, the following is equivalent,

cargo run -- rm --presume-input-tty
cargo run -- rm ---presume-input-tty
cargo run -- rm -----presume-input-tty
cargo run -- rm ---------presume-input-tty

It seems that this is basically a noop in GNU as well. I haven't taken the time to go through the source code to know what it actually does.
And I can't even google anything on this (What secrets does it try to hide!?)

hbina@DESKTOP-2D0I5MJ:~/git/uutils$ touch test
hbina@DESKTOP-2D0I5MJ:~/git/uutils$ echo "test" | rm ---presume-input-tty
rm: missing operand
Try 'rm --help' for more information.
hbina@DESKTOP-2D0I5MJ:~/git/uutils$ echo "test" | rm ---presume-input-tty test

Closes #2345 (Will you look at that number)

Signed-off-by: Hanif Bin Ariffin hanif.ariffin.4326@gmail.com

For whatever reason, the following is equivalent,

cargo run -- rm --presume-input-tty
cargo run -- rm ---presume-input-tty
cargo run -- rm -----presume-input-tty
cargo run -- rm ---------presume-input-tty

Signed-off-by: Hanif Bin Ariffin <hanif.ariffin.4326@gmail.com>
@miDeb
Copy link
Contributor
miDeb commented Jul 29, 2021

This is to override stdin_tty in gnu's rm for testing, even if stdin is not really a tty. It seems like rm changes its behavior based on that, however only in one particular case: https://github.com/coreutils/coreutils/blob/00ea4bacf6063ccc125209d5186f8f2382c6f0d4/src/remove.c#L211. I can't really understand what's it doing, and until we replicate gnu's behavior (our rm does not currently look at whether stdin is a tty at all) I think it's fine to make this a no-op.

@sylvestre
Copy link
Contributor

Could you please add a test to verify that the option isn't going to be rejected?
And maybe add a comment explaining what should/might be done if we want to implement it one day?
thanks

@hbina hbina marked this pull request as draft August 1, 2021 06:53
hbina added 2 commits November 4, 2021 16:57
Signed-off-by: Hanif Bin Ariffin <hanif.ariffin.4326@gmail.com>
@hbina hbina marked this pull request as ready for review November 4, 2021 09:04
@sylvestre
Copy link
Contributor
sylvestre commented Nov 19, 2021

Running GNU testsuite:

PASS +2 / FAIL -2 / ERROR +0 / SKIP +0 

Well done!

@sylvestre sylvestre merged commit 43bdcaf into uutils:master Nov 19, 2021
@hbina hbina deleted the hbina-rm-silently-accept-presume-input-tty branch November 19, 2021 21:47
@tertsdiepraam
Copy link
Member

Quick question since I'm running into this with the transition to clap 3 (#2863). In this PR, tests were added for --presume-input-tty and ----presume-input-tty (2 and 4 hyphens), but when I test GNU rm on my machine, both give an unrecognized option error. Should we still support these?

@hbina
Copy link
Contributor Author
hbina commented Jan 18, 2022

IIRC, the only one that GNU supports is 3 hyphens, but there was an issue with clap before where it doesn't matter how many hyphens there are it will still accept them. If clap 3 fixe this issue, perhaps those tests should be removed.

Anyhow, this feature isn't really necessary either, its just there to bypass some tests or something.

@tertsdiepraam
Copy link
Member

Thanks! Clap indeed removed the arbitrary number of hyphens, so I'll move to only supporting 3 hyphens with clap 3 (although because of how long arguments work in clap I'll have to support both 2 and 3 hyphens)

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.

Implement rm ---presume-input-tty
4 participants
0