Fix `run_external::expand_glob()` to return paths that are PWD-relative but reflect the original intent #13028

devyn · 2024-06-01T02:02:02Z

Description

This changes the expand_glob() function to use nu_engine::glob_from() so that absolute paths are actually preserved, rather than being made relative to the provided parent. This preserves the intent of whoever wrote the original path/glob, and also makes it so that tilde always produces absolute paths.

I also made expand_glob() handle Ctrl-C so that it can be interrupted.

cc @YizhePKU

Tests + Formatting

No additional tests here... but that might be a good idea.

devyn · 2024-06-01T02:06:29Z

Oops, this isn't quite finished.

… like other shells

devyn · 2024-06-01T02:45:51Z

Ok yeah I realized that this didn't handle "." and "./foo" properly before, and decided to do two things about it:

Don't pass patterns not containing glob chars through glob_from()
If the glob started with "." as a path component, add it back in. This fixes something like ./nu*

WindSoilder · 2024-06-02T01:30:00Z

crates/nu-command/src/system/run_external.rs

- if remainder.components().next().is_none() {
- Path::new(".")
+ // If the first component of the original `arg` string path was '.', that should be preserved
+ let relative_to_dot = Path::new(arg).starts_with(".");


I don't think you need to handle these relative logic by yourself. I think the logic before rewrite is good to use. It uses diff_paths to handle the difference between cwd and prefix

nushell/crates/nu-command/src/system/run_external.rs

Lines 623 to 640 in c61075e

if let Ok(arg) = m {

let arg = if let Some(prefix) = &prefix {

if let Ok(remainder) = arg.strip_prefix(prefix) {

let new_prefix = if let Some(pfx) = diff_paths(prefix, &cwd) {

pfx

} else {

prefix.to_path_buf()

};

new_prefix.join(remainder).to_string_lossy().to_string()

} else {

arg.to_string_lossy().to_string()

}

} else {

arg.to_string_lossy().to_string()

};

process.arg(&arg);

Ok I can test whether it passes my tests

I won't be able to check this until tomorrow, but I'm still interested in trying it

YizhePKU · 2024-06-02T13:34:09Z

Why are we checking for Ctrl-C in the middle of glob expansion?

I'm not familar with how Nushell handles Ctrl-C yet, but it feels weird that we're dealing with Ctrl-C within expand_glob().

If I recall correctly, the old implementation didn't do anything special with Ctrl-C other than wrap it in the resulting PipelineData.

devyn · 2024-06-02T20:52:44Z

@YizhePKU you have to check Ctrl-C during any potentially long loop, as it's impossible to interrupt otherwise. We may not have been doing it before but it's a good idea.

) # Description This pr is going to use `pathdiff::diff_path`, so we don't need to handle for relative path by ourselves. This is also the behavior before the rewritten of run_external.rs It's a follow up to #13028 # User-Facing Changes NaN # Tests + Formatting No need to add tests

…he parser (#13089) # Description We've had a lot of different issues and PRs related to arg handling with externals since the rewrite of `run-external` in #12921: - #12950 - #12955 - #13000 - #13001 - #13021 - #13027 - #13028 - #13073 Many of these are caused by the argument handling of external calls and `run-external` being very special and involving the parser handing quoted strings over to `run-external` so that it knows whether to expand tildes and globs and so on. This is really unusual and also makes it harder to use `run-external`, and also harder to understand it (and probably is part of the reason why it was rewritten in the first place). This PR moves a lot more of that work over to the parser, so that by the time `run-external` gets it, it's dealing with much more normal Nushell values. In particular: - Unquoted strings are handled as globs with no expand - The unescaped-but-quoted handling of strings was removed, and the parser constructs normal looking strings instead, removing internal quotes so that `run-external` doesn't have to do it - Bare word interpolation is now supported and expansion is done in this case - Expressions typed as `Glob` containing `Expr::StringInterpolation` now produce `Value::Glob` instead, with the quoted status from the expr passed through so we know if it was a bare word - Bare word interpolation for values typed as `glob` now possible, but not implemented - Because expansion is now triggered by `Value::Glob(_, false)` instead of looking at the expr, externals now support glob types # User-Facing Changes - Bare word interpolation works for external command options, and otherwise embedded in other strings: ```nushell ^echo --foo=(2 + 2) # prints --foo=4 ^echo -foo=$"(2 + 2)" # prints -foo=4 ^echo foo="(2 + 2)" # prints (no interpolation!) foo=(2 + 2) ^echo foo,(2 + 2),bar # prints foo,4,bar ``` - Bare word interpolation expands for external command head/args: ```nushell let name = "exa" ~/.cargo/bin/($name) # this works, and expands the tilde ^$"~/.cargo/bin/($name)" # this doesn't expand the tilde ^echo ~/($name)/* # this glob is expanded ^echo $"~/($name)/*" # this isn't expanded ``` - Ndots are now supported for the head of an external command (`^.../foo` works) - Glob values are now supported for head/args of an external command, and expanded appropriately: ```nushell ^("~/.cargo/bin/exa" | into glob) # the tilde is expanded ^echo ("*.txt" | into glob) # this glob is expanded ``` - `run-external` now works more like any other command, without expecting a special call convention for its args: ```nushell run-external echo "'foo'" # before PR: 'foo' # after PR: foo run-external echo "*.txt" # before PR: (glob is expanded) # after PR: *.txt ``` # Tests + Formatting Lots of tests added and cleaned up. Some tests that weren't active on Windows changed to use `nu --testbin cococo` so that they can work. Added a test for Linux only to make sure tilde expansion of commands works, because changing `HOME` there causes `~` to reliably change. - 🟢 `toolkit fmt` - 🟢 `toolkit clippy` - 🟢 `toolkit test` - 🟢 `toolkit test stdlib` # After Submitting - [ ] release notes: make sure to mention the new syntaxes that are supported

devyn added 2 commits May 31, 2024 18:40

Fix run_external::expand_glob()

989629d

add log::trace before running command too

9f9029e

devyn marked this pull request as draft June 1, 2024 02:06

fix issues with tests and make glob behavior at least feel a bit more…

638bbf4

… like other shells

devyn marked this pull request as ready for review June 1, 2024 02:44

try to fix the test on windows

0c14822

WindSoilder reviewed Jun 2, 2024

View reviewed changes

kubouch merged commit be8c1dc into nushell:main Jun 3, 2024
13 checks passed

BrewTestBot mentioned this pull request Jun 3, 2024

nushell 0.94.2 Homebrew/homebrew-core#173617

Merged

hustcer added this to the v0.95.0 milestone Jun 4, 2024

WindSoilder mentioned this pull request Jun 4, 2024

run_external.rs: use pathdiff::diff_path to handle relative path #13056

Merged

devyn mentioned this pull request Jun 7, 2024

Move most of the peculiar argument handling for external calls into the parser #13089

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `run_external::expand_glob()` to return paths that are PWD-relative but reflect the original intent #13028

Fix `run_external::expand_glob()` to return paths that are PWD-relative but reflect the original intent #13028

devyn commented Jun 1, 2024

devyn commented Jun 1, 2024

devyn commented Jun 1, 2024

WindSoilder Jun 2, 2024 •

edited

Loading

devyn Jun 2, 2024

devyn Jun 2, 2024

YizhePKU commented Jun 2, 2024 •

edited

Loading

devyn commented Jun 2, 2024

	if let Ok(arg) = m {
	let arg = if let Some(prefix) = &prefix {
	if let Ok(remainder) = arg.strip_prefix(prefix) {
	let new_prefix = if let Some(pfx) = diff_paths(prefix, &cwd) {
	pfx
	} else {
	prefix.to_path_buf()
	};

	new_prefix.join(remainder).to_string_lossy().to_string()
	} else {
	arg.to_string_lossy().to_string()
	}
	} else {
	arg.to_string_lossy().to_string()
	};

	process.arg(&arg);

Fix run_external::expand_glob() to return paths that are PWD-relative but reflect the original intent #13028

Fix run_external::expand_glob() to return paths that are PWD-relative but reflect the original intent #13028

Conversation

devyn commented Jun 1, 2024

Description

Tests + Formatting

devyn commented Jun 1, 2024

devyn commented Jun 1, 2024

WindSoilder Jun 2, 2024 • edited Loading

Choose a reason for hiding this comment

devyn Jun 2, 2024

Choose a reason for hiding this comment

devyn Jun 2, 2024

Choose a reason for hiding this comment

YizhePKU commented Jun 2, 2024 • edited Loading

devyn commented Jun 2, 2024

Fix `run_external::expand_glob()` to return paths that are PWD-relative but reflect the original intent #13028

Fix `run_external::expand_glob()` to return paths that are PWD-relative but reflect the original intent #13028

WindSoilder Jun 2, 2024 •

edited

Loading

YizhePKU commented Jun 2, 2024 •

edited

Loading