Skip to content

keep raw for variable inputed argument#6426

Merged
fdncred merged 3 commits intonushell:mainfrom
WindSoilder:quote_again
Aug 27, 2022
Merged

keep raw for variable inputed argument#6426
fdncred merged 3 commits intonushell:mainfrom
WindSoilder:quote_again

Conversation

@WindSoilder
Copy link
Copy Markdown
Contributor

Description

It's a further and user friendly improvement after #6420
Reiative issue: #6399

With this pr, users no longer need to quote variable as arguments which passed to external string.

Before:

let dump_command = "PGPASSWORD='db_secret' pg_dump -Fc -h 'db.host' -p '$db.port' -U postgres -d 'db_name' > '/tmp/dump_name'"
^echo $""($dump_command)""

After:

let dump_command = "PGPASSWORD='db_secret' pg_dump -Fc -h 'db.host' -p '$db.port' -U postgres -d 'db_name' > '/tmp/dump_name'"
# no need any quotes, if we pass variable as echo's argument, the inner quote will keep the same.
^echo $dump_command

Tests

Make sure you've done the following:

  • Add tests that cover your changes, either in the command examples, the crate/tests folder, or in the /tests folder.
  • Try to think about corner cases and various ways how your changes could break. Cover them with tests.
  • If adding tests is not possible, please document in the PR body a minimal example with steps on how to reproduce so one can verify your change works.

Make sure you've run and fixed any issues with these commands:

  • cargo fmt --all -- --check to check standard code formatting (cargo fmt --all applies these changes)
  • cargo clippy --workspace --features=extra -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect to check that you're using the standard code style
  • cargo test --workspace --features=extra to check that all the tests pass

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Aug 27, 2022

nice - how does this affect the other scenarios that we documented on that hackmd document? i.e. is this going to break some existing functionality?

@WindSoilder
Copy link
Copy Markdown
Contributor Author

Sorry, I have only one mac, and did some manual tests for MacOS relative commands, all work fine for me.

And I believe it doesn't affect any existing functionality

@kubouch
Copy link
Copy Markdown
Contributor

kubouch commented Aug 27, 2022

When I try to run the "before" example on Windows, I get "The system cannot find the path specified." Can you provide a simpler example? It is hard to understand what problem this PR is trying to solve.

@kubouch
Copy link
Copy Markdown
Contributor

kubouch commented Aug 27, 2022

I'm having the following output without this PR on Windows:

> let cmd = "PGPASSWORD='db_secret' pg_dump -Fc -h 'db.host'"

> ^echo $cmd
"PGPASSWORD='db_secret' pg_dump -Fc -h 'db.host'"

> ^echo $""($cmd)""
"\"PGPASSWORD='db_secret' pg_dump -Fc -h 'db.host'\""

> $cmd
PGPASSWORD='db_secret' pg_dump -Fc -h 'db.host'

And with this PR applied:

> let cmd = "PGPASSWORD='db_secret' pg_dump -Fc -h 'db.host'"

> ^echo $cmd
"PGPASSWORD='db_secret' pg_dump -Fc -h 'db.host'"

> ^echo $""($cmd)""
"\"PGPASSWORD='db_secret' pg_dump -Fc -h 'db.host'\""

> $cmd
PGPASSWORD='db_secret' pg_dump -Fc -h 'db.host'

Seems the same to me. But it might be that echo on Windows is doing some extra magic.

I'd suggest writing a simple one-liner in C or Rust that prints out the args you pass to it. And actually, you cal already do that with one of the Nushell's testbins: nu --testbin nonu $cmd. This might be useful way of testing instead of relying on a platform-specific echo. You can see the test_bins.rs for details.

@kubouch
Copy link
Copy Markdown
Contributor

kubouch commented Aug 27, 2022

So, trying out the testbin:

without this PR:

i let cmd = "PGPASSWORD='db_secret' pg_dump -Fc -h 'db.host'"

i nu --testbin nonu $cmd
PGPASSWORD=db_secret' pg_dump -Fc -h 'db.host

i nu --testbin nonu $""($cmd)""
PGPASSWORD='db_secret' pg_dump -Fc -h 'db.host'

with this PR:

> let cmd = "PGPASSWORD='db_secret' pg_dump -Fc -h 'db.host'"

> nu --testbin nonu $cmd
PGPASSWORD='db_secret' pg_dump -Fc -h 'db.host'

i nu --testbin nonu $""($cmd)""
PGPASSWORD='db_secret' pg_dump -Fc -h 'db.host'

So yeah, seems like it's fixed even on Windows as well. I'd suggest changing the test to use the testbin and remove the OS guard.

@WindSoilder
Copy link
Copy Markdown
Contributor Author

@kubouch Thanks for testing these on Windows, and nu --testbin nonu $cmd looks really interesting.

I just changed test code

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Aug 27, 2022

thanks. let's move forward with this.

@fdncred fdncred merged commit b88ace4 into nushell:main Aug 27, 2022
@WindSoilder WindSoilder deleted the quote_again branch August 27, 2022 13:27
dheater pushed a commit to dheater/nushell that referenced this pull request Sep 2, 2022
* keep raw for variable inputed argument

* fix clippy for windows

* make test runs on windows
IanManske pushed a commit that referenced this pull request May 23, 2024
This PR is a complete rewrite of `run_external.rs`. The main goal of the
rewrite is improving readability, but it also fixes some bugs related to
argument handling and the PATH variable (fixes
#6011).

I'll discuss some technical details to make reviewing easier.

## Argument handling

Quoting arguments for external commands is hard. Like, *really* hard.
We've had more than a dozen issues and PRs dedicated to quoting
arguments (see Appendix) but the current implementation is still buggy.

Here's a demonstration of the buggy behavior:

```nu
let foo = "'bar'"
^touch $foo            # This creates a file named `bar`, but it should be `'bar'`
^touch ...[ "'bar'" ]  # Same
```

I'll describe how this PR deals with argument handling.

First, we'll introduce the concept of **bare strings**. Bare strings are
**string literals** that are either **unquoted** or **quoted by
backticks** [^1]. Strings within a list literal are NOT considered bare
strings, even if they are unquoted or quoted by backticks.

When a bare string is used as an argument to external process, we need
to perform tilde-expansion, glob-expansion, and inner-quotes-removal, in
that order. "Inner-quotes-removal" means transforming from
`--option="value"` into `--option=value`.

## `.bat` files and CMD built-ins

On Windows, `.bat` files and `.cmd` files are considered executable, but
they need `CMD.exe` as the interpreter. The Rust standard library
supports running `.bat` files directly and will spawn `CMD.exe` under
the hood (see
[documentation](https://doc.rust-lang.org/std/process/index.html#windows-argument-splitting)).
However, other extensions are not supported [^2].

Nushell also supports a selected number of CMD built-ins. The problem
with CMD is that it uses a different set of quoting rules. Correctly
quoting for CMD requires using
[Command::raw_arg()](https://doc.rust-lang.org/std/os/windows/process/trait.CommandExt.html#tymethod.raw_arg)
and manually quoting CMD special characters, on top of quoting from the
Nushell side. ~~I decided that this is too complex and chose to reject
special characters in CMD built-ins instead [^3]. Hopefully this will
not affact real-world use cases.~~ I've implemented escaping that works
reasonably well.

## `which-support` feature

The `which` crate is now a hard dependency of `nu-command`, making the
`which-support` feature essentially useless. The `which` crate is
already a hard dependency of `nu-cli`, and we should consider removing
the `which-support` feature entirely.

## Appendix

Here's a list of quoting-related issues and PRs in rough chronological
order.

* #4609
* #4631
* #4601
  * #5846
* #5978
  * #6014
* #6154
  * #6161
* #6399
  * #6420
  * #6426
* #6465
* #6559
  * #6560

[^1]: The idea that backtick-quoted strings act like bare strings was
introduced by Kubouch and briefly mentioned in [the language
reference](https://www.nushell.sh/lang-guide/chapters/strings_and_text.html#backtick-quotes).

[^2]: The documentation also said "running .bat scripts in this way may
be removed in the future and so should not be relied upon", which is
another reason to move away from this. But again, quoting for CMD is
hard.

[^3]: If anyone wants to try, the best resource I found on the topic is
[this](https://daviddeley.com/autohotkey/parameters/parameters.htm).
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.

3 participants