Skip to content

Mitigate the poor interaction between ndots expansion and non-path strings#13218

Merged
devyn merged 3 commits intonushell:mainfrom
devyn:fix-external-arg-slashes
Jun 24, 2024
Merged

Mitigate the poor interaction between ndots expansion and non-path strings#13218
devyn merged 3 commits intonushell:mainfrom
devyn:fix-external-arg-slashes

Conversation

@devyn
Copy link
Copy Markdown
Contributor

@devyn devyn commented Jun 24, 2024

Description

@hustcer reported that slashes were disappearing from external args since #13089:

$> ossutil ls oss://abc/b/c
Error: invalid cloud url: "oss:/abc/b/c", please make sure the url starts with: "oss://"

$> ossutil ls 'oss://abc/b/c'
Error: oss: service returned error: StatusCode=403, ErrorCode=UserDisable, ErrorMessage="UserDisable", RequestId=66791EDEFE87B73537120838, Ec=0003-00000801, Bucket=abc, Object=

I narrowed this down to the ndots handling, since that does path parsing and path reconstruction in every case. I decided to change that so that it only activates if the string contains at least ..., since that would be the minimum trigger for ndots, and also to not activate it if the string contains ://, since it's probably undesirable for a URL.

Kind of a hack, but I'm not really sure how else we decide whether someone wants ndots or not.

User-Facing Changes

  • bare strings not containing ndots are not modified
  • bare strings containing :// are not modified

Tests + Formatting

Added tests to prevent regression.

@devyn
Copy link
Copy Markdown
Contributor Author

devyn commented Jun 24, 2024

One other benefit realized: the ndots expansion was messing with arg handling on Windows, always replacing forward slashes with backslashes. This should help prevent that. We may need to do something other than use path components for ndots to avoid that level of normalization...

@devyn devyn merged commit 43e349a into nushell:main Jun 24, 2024
@devyn devyn deleted the fix-external-arg-slashes branch June 25, 2024 00:01
@hustcer hustcer added this to the v0.95.0 milestone Jun 25, 2024
ysthakur pushed a commit that referenced this pull request Jan 5, 2025
<!--
if this PR closes one or more issues, you can automatically link the PR
with
them by using one of the [*linking
keywords*](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword),
e.g.
- this PR should close #xxxx
- fixes #xxxx

you can also mention related issues, PRs or discussions!
-->

# Description
Prevents ndots from being expanded if they are prefixed with `./`, as
the agreed resolution to #13303. Only applies to externals, mirroring
the fix from #13218.

I did
[attempt](https://github.com/132ikl/nushell/tree/internal-ndots-attempt)
to apply the fix for internal commands as well, but it seems like the
path is expanded too aggressively and I haven't investigated it further
yet. `./...` gets normalized into `<pwd>/./...`, which gets normalized
into `<pwd>/...` before being handed to `expand_ndots`, and at that
point it just looks like a normal n-dots so we can't tell we shouldn't
expand.

(Fixes #13303)

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

* N-dots are no longer expanded to external command calls when prefixed
with `./`.

# Tests + Formatting
<!--
Don't forget to add tests that cover your changes.

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 -- -D warnings -D clippy::unwrap_used` to
check that you're using the standard code style
- `cargo test --workspace` to check that all tests pass (on Windows make
sure to [enable developer
mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging))
- `cargo run -- -c "use toolkit.nu; toolkit test stdlib"` to run the
tests for the standard library

> **Note**
> from `nushell` you can also use the `toolkit` as follows
> ```bash
> use toolkit.nu # or use an `env_change` hook to activate it
automatically
> toolkit check pr
> ```
-->



Added tests to prevent regression.

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# 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.
-->

N/A
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.

2 participants