Skip to content

Commit

Permalink
Unify glob behavior on open, rm, cp-old, mv, umv, cp and …
Browse files Browse the repository at this point in the history
…`du` commands (#11621)

# Description
This pr is a follow up to
[#11569](#11569 (comment))
> Revert the logic in #10694 and
apply the logic in this pr to mv, cp, rv will require a larger change, I
need to think how to achieve the bahavior

And sorry @bobhy for reverting some of your changes.

This pr is going to unify glob behavior on the given commands:
* open
* rm
* cp-old
* mv
* umv
* cp
* du

So they have the same behavior to `ls`, which is:
If given parameter is quoted by single quote(`'`) or double quote(`"`),
don't auto-expand the glob pattern. If not quoted, auto-expand the glob
pattern.

Fixes: #9558  Fixes: #10211 Fixes: #9310 Fixes: #10364 

# TODO
But there is one thing remains: if we give a variable to the command, it
will always auto-expand the glob pattern, e.g:
```nushell
let path = "a[123]b"
rm $path
```
I don't think it's expected. But I also think user might want to
auto-expand the glob pattern in variables.

So I'll introduce a new command called `glob escape`, then if user
doesn't want to auto-expand the glob pattern, he can just do this: `rm
($path | glob escape)`

# User-Facing Changes
<!-- List of all changes that impact the user experience here. This
helps us keep track of breaking changes. -->

# Tests + Formatting
Done

# After Submitting
<!-- If your PR had any user-facing changes, update [the
documentation](https://github.com/nushell/nushell.github.io) after the
PR is merged, if necessary. This will help us keep the docs up to date.
-->

## NOTE
This pr changes the semantic of `GlobPattern`, before this pr, it will
`expand path` after evaluated, this makes `nu_engine::glob_from` have no
chance to glob things right if a path contains glob pattern.

e.g: [#9310
](#9310 (comment))
#10211

I think changing the semantic is fine, because it makes glob works if
path contains something like '*'.

It maybe a breaking change if a custom command's argument are annotated
by `: glob`.
  • Loading branch information
WindSoilder committed Jan 26, 2024
1 parent e43d893 commit d646903
Show file tree
Hide file tree
Showing 28 changed files with 318 additions and 431 deletions.
1 change: 0 additions & 1 deletion crates/nu-cli/src/syntax_highlight.rs
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,6 @@ fn find_matching_block_end_in_expr(
Expr::Filepath(_, _) => None,
Expr::Directory(_, _) => None,
Expr::GlobPattern(_, _) => None,
Expr::LsGlobPattern(_, _) => None,
Expr::String(_) => None,
Expr::CellPath(_) => None,
Expr::ImportPattern(_) => None,
Expand Down
205 changes: 0 additions & 205 deletions crates/nu-cmd-base/src/arg_glob.rs

This file was deleted.

3 changes: 0 additions & 3 deletions crates/nu-cmd-base/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,4 @@
mod arg_glob;
pub mod formats;
pub mod hook;
pub mod input_handler;
pub mod util;
pub use arg_glob::arg_glob;
pub use arg_glob::arg_glob_leading_dot;
8 changes: 5 additions & 3 deletions crates/nu-command/src/filesystem/ls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ impl Command for Ls {
.input_output_types(vec![(Type::Nothing, Type::Table(vec![]))])
// LsGlobPattern is similar to string, it won't auto-expand
// and we use it to track if the user input is quoted.
.optional("pattern", SyntaxShape::LsGlobPattern, "The glob pattern to use.")
.optional("pattern", SyntaxShape::GlobPattern, "The glob pattern to use.")
.switch("all", "Show hidden files", Some('a'))
.switch(
"long",
Expand Down Expand Up @@ -165,7 +165,8 @@ impl Command for Ls {
};

let hidden_dir_specified = is_hidden_dir(&path);
// when it's quoted, we need to escape our glob pattern
// when it's quoted, we need to escape our glob pattern(but without the last extra
// start which may be added under given directory)
// so we can do ls for a file or directory like `a[123]b`
let path = if quoted {
let p = path.display().to_string();
Expand All @@ -185,7 +186,8 @@ impl Command for Ls {
};

let glob_path = Spanned {
item: path.clone(),
// It needs to be un-quoted, the relative logic is handled previously
item: NuPath::UnQuoted(path.clone()),
span: p_tag,
};

Expand Down
16 changes: 8 additions & 8 deletions crates/nu-command/src/filesystem/mv.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
use std::path::{Path, PathBuf};

use super::util::try_interaction;
use nu_cmd_base::arg_glob;
use nu_engine::env::current_dir;
use nu_engine::CallExt;
use nu_protocol::ast::Call;
use nu_protocol::engine::{Command, EngineState, Stack};
use nu_protocol::{
Category, Example, IntoInterruptiblePipelineData, PipelineData, ShellError, Signature, Span,
Spanned, SyntaxShape, Type, Value,
Category, Example, IntoInterruptiblePipelineData, NuPath, PipelineData, ShellError, Signature,
Span, Spanned, SyntaxShape, Type, Value,
};

#[derive(Clone)]
Expand Down Expand Up @@ -63,7 +62,8 @@ impl Command for Mv {
_input: PipelineData,
) -> Result<PipelineData, ShellError> {
// TODO: handle invalid directory or insufficient permissions when moving
let spanned_source: Spanned<String> = call.req(engine_state, stack, 0)?;
let mut spanned_source: Spanned<NuPath> = call.req(engine_state, stack, 0)?;
spanned_source.item = spanned_source.item.strip_ansi_string_unlikely();
let spanned_destination: Spanned<String> = call.req(engine_state, stack, 1)?;
let verbose = call.has_flag(engine_state, stack, "verbose")?;
let interactive = call.has_flag(engine_state, stack, "interactive")?;
Expand All @@ -73,11 +73,11 @@ impl Command for Mv {
let ctrlc = engine_state.ctrlc.clone();

let path = current_dir(engine_state, stack)?;
let source = path.join(spanned_source.item.as_str());
let destination = path.join(spanned_destination.item.as_str());

let mut sources =
arg_glob(&spanned_source, &path).map_or_else(|_| Vec::new(), Iterator::collect);
let mut sources = nu_engine::glob_from(&spanned_source, &path, call.head, None)
.map(|p| p.1)
.map_or_else(|_| Vec::new(), Iterator::collect);

if sources.is_empty() {
return Err(ShellError::FileNotFound {
Expand All @@ -94,7 +94,7 @@ impl Command for Mv {
//
// Second, the destination doesn't exist, so we can only rename a single source. Otherwise
// it's an error.

let source = path.join(spanned_source.item.as_ref());
if destination.exists() && !force && !destination.is_dir() && !source.is_dir() {
return Err(ShellError::GenericError {
error: "Destination file already exists".into(),
Expand Down
29 changes: 15 additions & 14 deletions crates/nu-command/src/filesystem/open.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ use nu_protocol::ast::Call;
use nu_protocol::engine::{Command, EngineState, Stack};
use nu_protocol::util::BufferedReader;
use nu_protocol::{
Category, DataSource, Example, IntoInterruptiblePipelineData, PipelineData, PipelineMetadata,
RawStream, ShellError, Signature, Spanned, SyntaxShape, Type, Value,
Category, DataSource, Example, IntoInterruptiblePipelineData, NuPath, PipelineData,
PipelineMetadata, RawStream, ShellError, Signature, Spanned, SyntaxShape, Type, Value,
};
use std::io::BufReader;

Expand Down Expand Up @@ -42,10 +42,10 @@ impl Command for Open {
fn signature(&self) -> nu_protocol::Signature {
Signature::build("open")
.input_output_types(vec![(Type::Nothing, Type::Any), (Type::String, Type::Any)])
.optional("filename", SyntaxShape::Filepath, "The filename to use.")
.optional("filename", SyntaxShape::GlobPattern, "The filename to use.")
.rest(
"filenames",
SyntaxShape::Filepath,
SyntaxShape::GlobPattern,
"Optional additional files to open.",
)
.switch("raw", "open file as raw binary", Some('r'))
Expand All @@ -63,8 +63,8 @@ impl Command for Open {
let call_span = call.head;
let ctrlc = engine_state.ctrlc.clone();
let cwd = current_dir(engine_state, stack)?;
let req_path = call.opt::<Spanned<String>>(engine_state, stack, 0)?;
let mut path_params = call.rest::<Spanned<String>>(engine_state, stack, 1)?;
let req_path = call.opt::<Spanned<NuPath>>(engine_state, stack, 0)?;
let mut path_params = call.rest::<Spanned<NuPath>>(engine_state, stack, 1)?;

// FIXME: JT: what is this doing here?

Expand All @@ -87,19 +87,20 @@ impl Command for Open {
}
};

path_params.insert(0, filename);
path_params.insert(
0,
Spanned {
item: NuPath::UnQuoted(filename.item),
span: filename.span,
},
);
}

let mut output = vec![];

for path in path_params.into_iter() {
for mut path in path_params.into_iter() {
//FIXME: `open` should not have to do this
let path = {
Spanned {
item: nu_utils::strip_ansi_string_unlikely(path.item),
span: path.span,
}
};
path.item = path.item.strip_ansi_string_unlikely();

let arg_span = path.span;
// let path_no_whitespace = &path.item.trim_end_matches(|x| matches!(x, '\x09'..='\x0d'));
Expand Down

0 comments on commit d646903

Please sign in to comment.