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.

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

Optionally enable backtick command substitution for pickRemoteProcess #5053

Merged
merged 12 commits into from
Mar 10, 2020

Conversation

Helloimbob
Copy link
Contributor

This adds an option to use the obsolete backtick (`) command substitution in the debugger's pickRemoteProcess shell command for use with remote shells (such as csh) which don't support $() substitution.

This fixes #4015

@msftclas
Copy link

msftclas commented Mar 4, 2020

CLA assistant check
All CLA requirements met.

Update useBacktickCommandSubstitution setting to window scope
Copy link
Collaborator

@sean-mcmanus sean-mcmanus left a comment

Choose a reason for hiding this comment

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

Seems fine to me, but someone from the debugger team needs to sign off too.

if (settings.useBacktickCommandSubstitution) {
substitutionBegin = `\``;
substitutionEnd = `\``;
escapedQuote = `\"`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does backtick substitution mean that we don't need to escape the \ symbol?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally with the escaped \ the command failed with an unmatched " error. I've just looked into this again and it seems to actually be a quirk in csh where stuff in single quotes doesn't get expanded: Stack Overflow answer

@@ -108,6 +109,16 @@ export class RemoteAttachPicker {
private getRemoteProcessCommand(): string {
let innerQuote: string = `'`;
let outerQuote: string = `"`;
let substitutionBegin: string = `$(`;
Copy link
Contributor

Choose a reason for hiding this comment

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

rename as parameterBegin and parameterEnd

@sean-mcmanus
Copy link
Collaborator

We should probably delay checking this in until 0.27.0-insiders3 or 0.27.0.

Renaming substitutionBegin and substiutionEnd to parameterBegin and parameterEnd
@sean-mcmanus
Copy link
Collaborator

sean-mcmanus commented Mar 5, 2020

@bobbrow @WardenGnaw Should we name the setting something like C_Cpp.debugging.useBacktickCommandSubstitution or C_Cpp.debugger.useBacktickCommandSubstitution ? It seems like that would help avoid confusing it with the other settings that are mostly language service related.

@sean-mcmanus
Copy link
Collaborator

@Helloimbob We just checked in a big "enable strictNullChecks" change, which is probably causing the merge conflict.

@sean-mcmanus
Copy link
Collaborator

@WardenGnaw Can this be checked in for 0.27.0-insiders3? Should we rename the setting?

@WardenGnaw
Copy link
Member

I agree that it should be renamed to C_Cpp.debugger.useBacktickCommandSubstitution.

@sean-mcmanus sean-mcmanus merged commit 831f29c into microsoft:master Mar 10, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Oct 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

{command:pickRemoteProcess} doesn't work if remote user's default shell is CShell
5 participants