Improve command line argument quoting logic#335
Open
nahk-ivanov wants to merge 2 commits intotunnelvisionlabs:masterfrom
nahk-ivanov:fix-cmd-args-quoting
Open
Improve command line argument quoting logic#335nahk-ivanov wants to merge 2 commits intotunnelvisionlabs:masterfrom nahk-ivanov:fix-cmd-args-quoting
nahk-ivanov wants to merge 2 commits intotunnelvisionlabs:masterfrom
nahk-ivanov:fix-cmd-args-quoting
Conversation
…age and no files generated. After troubleshooting I root-caused the issue to .NET command line parsing rules, where special characters like double-quotes and backslashes have to be escaped under some circumstances. There already was a code to handle some of these cases, but it didn't cover the following: 1) When there are multiple backslashes preceeding the double-quote, we need to escape all of them, not only the last one. 2) When there is a backslash at the end of the argument, we need to escape it, otherwise it will escape the double-quote appended by the code itself, causing attribute value to run out when parsed by .NET infrastructure. This change is improving the logic to account for these two extra cases. I also added unit tests to demonstrate the behavior. In order to do so, I had to move the JoinArguments methd to a public helper class, so that it can be accessed from unit tests, as I'm not a big fan of InternalsVisibleTo.
nahk-ivanov
commented
Jul 16, 2019
| } | ||
|
|
||
| // escape backslashes appearing before a double quote or end of line by doubling them | ||
| string arg = Regex.Replace(argument, @"(\\+)(""|$)", "$1$1$2"); |
Author
There was a problem hiding this comment.
If you don't want tests/new helper class, then all it takes to fix it is to replace this line in the original method.
Author
Member
|
I don't mind adding a new class and test project, but we definitely want IVT to the test project and change the helper class to |
…y using InternalsVisibleTo attribute
Author
|
Done. Luckily there was already a signing key for test assemblies, so I didn't have to add a new one. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We hit an issue where C# generator tool would exit with no error message and no files generated.
After troubleshooting I root-caused the issue to .NET command line parsing rules, where special characters like double-quotes and backslashes have to be escaped under some circumstances.
There already was a code to handle some of these cases, but it didn't cover the following:
This change is improving the logic to account for these two extra cases. I also added unit tests to demonstrate the behavior. In order to do so, I had to move the JoinArguments method to a public helper class, so that it can be accessed from unit tests, as I'm not a big fan of InternalsVisibleTo.