Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix bash completion in directory with spaces #13126

Merged
merged 1 commit into from
Dec 7, 2023
Merged

fix bash completion in directory with spaces #13126

merged 1 commit into from
Dec 7, 2023

Conversation

lukas-code
Copy link
Member

What does this PR try to resolve?

This PR fixes bash completion for cargo run --bin <TAB> when the path to the project contains a space.

How should we test and review this PR?

  1. create a project in a directory with a space, for example mkdir '/tmp/has space' && cd '/tmp/has space' && cargo new foo && cd foo.
  2. add a binary to Cargo.toml, e.g. 
    [[bin]]
name = "bar"
  3. on bash, type cargo run --bin b and press tab
  4. observe error message bash: $manifest: ambiguous redirect, command is not completed
  5. apply the fix to $(rustc --print sysroot)/etc/bash_completion.d/cargo
  6. repeat step 3 and observe that the command is completed successfully

@rustbot
Copy link
Collaborator

rustbot commented Dec 6, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @epage (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added A-completions Area: shell completions S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 6, 2023
@epage
Copy link
Contributor

epage commented Dec 6, 2023

@bors r+

I guess? I dislike working with completions...

@bors
Copy link
Contributor

bors commented Dec 6, 2023

📌 Commit 5853e99 has been approved by epage

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 6, 2023
@bors
Copy link
Contributor

bors commented Dec 6, 2023

⌛ Testing commit 5853e99 with merge 0989757...

bors added a commit that referenced this pull request Dec 6, 2023
@bors
Auto merge of #13126 - lukas-code:bashcomp, r=epage
0989757 
fix bash completion in directory with spaces

### What does this PR try to resolve?

This PR fixes bash completion for `cargo run --bin <TAB>` when the path to the project contains a space.

### How should we test and review this PR?

1. create a project in a directory with a space, for example `mkdir '/tmp/has space' && cd '/tmp/has space' && cargo new foo && cd foo`.
2. add a binary to Cargo.toml, e.g.
   ```
   [[bin]]
   name = "bar"
   ```
3. on bash, type `cargo run --bin b` and press tab
4. observe error message `bash: $manifest: ambiguous redirect`, command is not completed
5. apply the fix to `$(rustc --print sysroot)/etc/bash_completion.d/cargo`
6. repeat step 3 and observe that the command is completed successfully
@bors
Copy link
Contributor

bors commented Dec 6, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 6, 2023
@weihanglo
Copy link
Member

Looks like we need to ignore those tests… again.

Instead of error out, now it completes nothing on my machine.

  • macbook M1
  • Bash 5.2.15
  • bash-completion 2.11

I don't know what could go run.

@weihanglo weihanglo mentioned this pull request Dec 7, 2023
Merged
bors added a commit that referenced this pull request Dec 7, 2023
@bors
Auto merge of #13129 - weihanglo:ignore, r=epage
64266c8 
test: re-ignore git auth tests for gitoxide

Happened again in <#13126 (comment)>
even after #13117. Let's ignore them again and hope someday we can fix it :)
@lukas-code
Copy link
Member Author

Instead of error out, now it completes nothing on my machine.

I just tested it with bash 5.2.15 + bash-completion 2.11 in a docker image and it seems to work for me.

Did you add a [[bin]] target to Cargo.toml? The script currently only works for manually defined [[bin]]/[[test]]/[[bench]] targets and only auto-discovered targets for examples, and this PR does not change that.

weihanglo
weihanglo approved these changes Dec 7, 2023
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the step to review. I don't know how I missed that part.

Now it seems working again :)

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Dec 7, 2023

💡 This pull request was already approved, no need to approve it again.

  • This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.

@bors
Copy link
Contributor

bors commented Dec 7, 2023

📌 Commit 5853e99 has been approved by weihanglo

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 7, 2023
@bors
Copy link
Contributor

bors commented Dec 7, 2023

⌛ Testing commit 5853e99 with merge 52b1320...

@bors
Copy link
Contributor

bors commented Dec 7, 2023

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 52b1320 to master...

@bors bors merged commit 52b1320 into rust-lang:master Dec 7, 2023
20 checks passed
@lukas-code lukas-code deleted the bashcomp branch December 7, 2023 13:55
@weihanglo weihanglo mentioned this pull request Dec 9, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 9, 2023
@bors
Auto merge of rust-lang#118765 - weihanglo:update-cargo, r=weihanglo
c67cd8f 
Update cargo

12 commits in 9787229614b27854cf73d57ffae430d7c1e6caa4..6feabf94773286928b7d73d0d313c0c5562b66af
2023-12-06 02:29:23 +0000 to 2023-12-08 22:38:37 +0000
- fix: explicitly remap current dir by using `.` (rust-lang/cargo#13114)
- Don't rely on mtime to test changes (rust-lang/cargo#13143)
- refactor: Pull PackageIdSpec into schema (rust-lang/cargo#13128)
- fix: Print rustc messages colored on wincon (rust-lang/cargo#13140)
- Add a windows manifest file (rust-lang/cargo#13131)
- Avoid writing CACHEDIR.TAG if it already exists (rust-lang/cargo#13132)
- re-enable flaky tests thanks to update to `gix-config`. (rust-lang/cargo#11821) (rust-lang/cargo#13130)
- fix bash completion in directory with spaces (rust-lang/cargo#13126)
- test: re-ignore git auth tests for gitoxide (rust-lang/cargo#13129)
- fix(toml): Disallow inheriting of dependency public status (rust-lang/cargo#13125)
- re-enable previously disabled tests with Windows-specific fix (rust-lang/cargo#13117)
- refactor: Clarify PackageId constructor names (rust-lang/cargo#13123)

r? ghost
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 9, 2023
@bors
Auto merge of rust-lang#118765 - weihanglo:update-cargo, r=weihanglo
a01b2d4 
Update cargo

13 commits in 9787229614b27854cf73d57ffae430d7c1e6caa4..66ad359b408ccdf867cceb30cc2dff1ed4afd82d
2023-12-06 02:29:23 +0000 to 2023-12-09 12:30:01 +0000
- chore: downgrade to openssl v1.1.1 (rust-lang/cargo#13144)
- fix: explicitly remap current dir by using `.` (rust-lang/cargo#13114)
- Don't rely on mtime to test changes (rust-lang/cargo#13143)
- refactor: Pull PackageIdSpec into schema (rust-lang/cargo#13128)
- fix: Print rustc messages colored on wincon (rust-lang/cargo#13140)
- Add a windows manifest file (rust-lang/cargo#13131)
- Avoid writing CACHEDIR.TAG if it already exists (rust-lang/cargo#13132)
- re-enable flaky tests thanks to update to `gix-config`. (rust-lang/cargo#11821) (rust-lang/cargo#13130)
- fix bash completion in directory with spaces (rust-lang/cargo#13126)
- test: re-ignore git auth tests for gitoxide (rust-lang/cargo#13129)
- fix(toml): Disallow inheriting of dependency public status (rust-lang/cargo#13125)
- re-enable previously disabled tests with Windows-specific fix (rust-lang/cargo#13117)
- refactor: Clarify PackageId constructor names (rust-lang/cargo#13123)
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 12, 2023
@bors
Auto merge of rust-lang#118765 - weihanglo:update-cargo, r=weihanglo
3340d49 
Update cargo

20 commits in 9787229614b27854cf73d57ffae430d7c1e6caa4..1aa9df1a5be205cce621f0bc0ea6062a5e22a98c
2023-12-06 02:29:23 +0000 to 2023-12-12 14:52:31 +0000
- crates-io: Add support for other 2xx HTTP status codes (rust-lang/cargo#13158)
- Remove the deleted feature test_2018_feature from the test (rust-lang/cargo#13156)
- refactor(schema): Remove reliance on cargo types (rust-lang/cargo#13154)
- fix(toml)!: Disallow `[lints]` in virtual workspaces (rust-lang/cargo#13155)
- Limit exported-private-dependencies lints to libraries (rust-lang/cargo#13135)
- chore: update to gix-index@0.27.1 (rust-lang/cargo#13148)
- Update curl-sys to bring in curl 8.5.0 (rust-lang/cargo#13147)
- chore: downgrade to openssl v1.1.1 (rust-lang/cargo#13144)
- fix: explicitly remap current dir by using `.` (rust-lang/cargo#13114)
- Don't rely on mtime to test changes (rust-lang/cargo#13143)
- refactor: Pull PackageIdSpec into schema (rust-lang/cargo#13128)
- fix: Print rustc messages colored on wincon (rust-lang/cargo#13140)
- Add a windows manifest file (rust-lang/cargo#13131)
- Avoid writing CACHEDIR.TAG if it already exists (rust-lang/cargo#13132)
- re-enable flaky tests thanks to update to `gix-config`. (rust-lang/cargo#11821) (rust-lang/cargo#13130)
- fix bash completion in directory with spaces (rust-lang/cargo#13126)
- test: re-ignore git auth tests for gitoxide (rust-lang/cargo#13129)
- fix(toml): Disallow inheriting of dependency public status (rust-lang/cargo#13125)
- re-enable previously disabled tests with Windows-specific fix (rust-lang/cargo#13117)
- refactor: Clarify PackageId constructor names (rust-lang/cargo#13123)
@ehuss ehuss added this to the 1.76.0 milestone Mar 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weihanglo weihanglo weihanglo approved these changes

Assignees

@epage epage

Labels
A-completions Area: shell completions S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.76.0
Development

Successfully merging this pull request may close these issues.
6 participants
@lukas-code @rustbot @epage @bors @weihanglo @ehuss