-
Notifications
You must be signed in to change notification settings - Fork 267
Use ArgumentList for commands instead of manual escaping #473
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
Conversation
BTW, I don't think using 'ArgumentList' is better than 'Arguments' since we have manually escaped the path strings and when creating a specified process, the parameter list is always concated into a single string.
Manual escaping is bug-prone and may fail if path contains " with the current implementation.
But the main reason in ArgumentList is to provide a more robust and secure way to provide arguments. It is simply easier to work with array than string.
when creating a specified process, the parameter list is always concated into a single string
Maybe on Windows. Before this dotnet will parse command to argv anyway, later passed to execve inside native interop
In the end, the main goal of this PR is to make commands more readable and easier to maintain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ExtraArgs is binded to a TextBox that allows user to provide extra arguments while clone repositories
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See:
image
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be --all instead of -all
I can address the issues, but how do you feel about the change in general? No worries if you don't need this
I can address the issues, but how do you feel about the change in general? No worries if you don't need this
Both ways are fine for me. I don't have any preference.
This will refactor command arguments from a single string to an argument array
Argument arrays are easier to work with, and you don't have to worry about escaping arguments. They are also easier to work with because you can split them into multiple lines
I've tested all the features I use, but an extra pair of eyes is really needed 😁