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

fix(core): Use Invoke-Command instead of Invoke-Expression #4941

Merged
merged 4 commits into from
May 26, 2022

Conversation

niheaven
Copy link
Member

@niheaven niheaven commented May 20, 2022

Description

Get rid of Invoke-Expression that may cause problems and may leak some code.

Ref: https://github.com/MicrosoftDocs/PowerShell-Docs/wiki/The-case-for-and-against-Invoke-Expression

Motivation and Context

How Has This Been Tested?

  • scoop -v
  • scoop update
  • scoop install gnupg
  • .\bin\checkver i* in 'extras' bucket
  • .\test.ps1

Checklist:

  • I have read the Contributing Guide.
  • I have ensured that I am targeting the develop branch.
  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.
  • I have added an entry in the CHANGELOG.

@rashil2000
Copy link
Member

There are multiple instances of such commands:

$currentBranch = Invoke-Command { git -C "$currentdir" branch }

Why not just replace them with:

$currentBranch = git -C "$currentdir" branch

@niheaven
Copy link
Member Author

Invoke-Command run the script block in a child scope, so it should be safer and could improve readability.

@rashil2000
Copy link
Member

Scope doesn't matter for git (and other similar external commands) right? On the other hand, I feel it hampers readability because it makes it seem like git commands need special handling.

For strings containing PowerShell code, it makes sense.

@niheaven
Copy link
Member Author

Great, let's try and see if there are unexpected bugs.

@niheaven
Copy link
Member Author

Removed Invoke-Command before git, and tested okay here.

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