A couple fixes for the credential helper selector #319
Conversation
This file contains 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
In b64ba6a (credential-helper-selector: use the correct format specifier for Unicode, 2020-08-11), we changed all those `%s` to `%S` to make sure that wide-character string parameters were handled correctly. While this does appear to work correctly, it is a bit fragile because `%S` is fragile: there are two standards defining its behavior, and they disagree. The Microsoft convention is that `%S` in a wide-character format string means that the argument is a multi-byte string: https://docs.microsoft.com/en-us/cpp/c-runtime-library/format-specification-syntax-printf-and-wprintf-functions This is the opposite interpretation of POSIX, which defines `%S` to indicate a wide-character string argument, whether specified in a multi-byte format string or a wide-character format string: https://pubs.opengroup.org/onlinepubs/9699919799/functions/fwprintf.html However, there is one safe recourse: if you want to print wide-character strings via `printf()` or `wprintf()`, just use the `%ls` format specifier. That works with POSIX semantics as well as with Microsoft semantics. So let's do exactly that. Signed-off-by: Johannes Schindelin <[email protected]>
In general, we cannot guarantee that the path of the chosen credential helper has no special characters in it. Therefore, we have to quote it. This can be verified by putting a credential manager into a directory whose path contains a space (such as `C:\Program Files\...`), adding that directory to the `PATH`, and then calling the credential-helper-selector e.g. with the argument `version` (which at least Git Credential Manager Core understands). While the executable will still be spawned correctly, it will then fail to parse its command line correctly. The symptom looks like this: Unrecognized command 'Files'. usage: git-credential-manager-core <command> Available commands: erase get store configure [--system] unconfigure [--system] --version, version --help, -h, -? Let's quote the absolute path correctly when copying it into the command line. Signed-off-by: Johannes Schindelin <[email protected]>
dscho
added a commit
that referenced
this pull request
Dec 11, 2020
The credential-helper selector (which is the default credential helper in the Portable version of Git for Windows) [now handles paths with spaces correctly](#319). Signed-off-by: Johannes Schindelin <[email protected]>
This comment has been minimized.
This comment has been minimized.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
In a conversation with @mjcheetham I realized that neither
%snor%Sare appropriate format specifiers for wide-character string arguments inprintf()/wprintf()calls. Instead,%lsis the best option.After fixing this, I noticed in my tests that the helper selector constructed a command line with improperly quoted paths. That's the other fix included in this PR.