uri: support ports and split queries/fragments from path#825
Merged
Conversation
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
mjcheetham
reviewed
Sep 26, 2022
ldennington
force-pushed
the
support-ports
branch
4 times, most recently
from
March 1, 2023 00:41
1878d15 to
d46f426
Compare
ldennington
commented
Mar 1, 2023
ldennington
force-pushed
the
support-ports
branch
5 times, most recently
from
March 2, 2023 02:02
8f3e887 to
0d140b1
Compare
ldennington
marked this pull request as ready for review
March 2, 2023 02:10
ldennington
force-pushed
the
support-ports
branch
from
March 2, 2023 02:11
0d140b1 to
c936a8c
Compare
ldennington
changed the title
mjcheetham
approved these changes
Mar 2, 2023
mjcheetham
left a comment
mjcheetham
left a comment
Contributor
There was a problem hiding this comment.
Looks good! Just one 'nit' question.
GCM currently ignores ports in URIs. This means attempting to authenticate to a URI with a port can lead to some unexpected behavior (e.g. shortcutting the provider auto-detect process won't work, even if the provider is set in config). This change adds support for ports by updating GetGitConfigurationScopes() to recognize them. This change also updates GetRemoteUri() to recognize paths with queries and fragments, as that issue was uncovered during the implementation of the GetGitConfigurationScopes() fix. Without it, input paths containing queries and/or fragments get saved as part of Uri.Path, which converts the query '?' and the fragment '#' to url encoding.
ldennington
force-pushed
the
support-ports
branch
from
March 2, 2023 16:10
c936a8c to
62abc30
Compare
mjcheetham
added a commit
that referenced
this pull request
May 2, 2023
**Changes:** - Support ports in URL-scoped config (#825) - Support URL-scoped enterprise default settings (#1149) - Add support for client TLS certificates (#1152) - Add TRACE2 support(#1131, #1151, #1156, #1162) - Better browser detection inside of WSL (#1148) - Handle expired OAuth refresh token for generic auth (#1196) - Target *-latest runner images in CI workflow (#1178) - Various bug fixes: - Ensure we create a WindowsProcessManager on Windows (#1146) - Ensure we start child processes created with ProcessManager (#1177) - Fix app path name of Windows dropping file extension (#1181) - Ensure we init IEnvironment before SessionManager (#1167) - git: consistently read from stdout before exit wait (#1136) - trace2: guard against null pipe client in dispose (#1135) - Make Avalonia UI the default Windows and move to in-process (#1207) - Add Git configuration options for trace & debug (#1228) - Transition from Nerdbank.GitVersioning to a version file (#1231) - Add support for using the current Windows user for WAM on DevBox (#1197) - Various documentation updates: - org-rename: update references to GitCredentialManager (#1141) - issue templates: remove core suffix (#1180) - readme: add link to project roadmap (#1204) - docs: add bitbucket app password requirements (#1213) - .net tool: clarify install instructions (#1126) - docs: call out different GCM install paths in WSL docs (#1168) - docs: add trace2 to config/env documentation (#1230)
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
3 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.
GCM currently ignores ports in URIs. This means attempting to authenticate
to a URI with a port can lead to some unexpected behavior (e.g.
shortcutting the provider auto-detect process won't work, even if the
provider is set in config). This change adds support for ports by updating
GetGitConfigurationScopes() to recognize them.
This change also updates GetRemoteUri() to recognize paths with queries
and fragments, as that issue was uncovered during the implementation of
the GetGitConfigurationScopes() fix. Without it, input paths containing
queries and/or fragments get saved as part of Uri.Path, which converts the
query '?' and the fragment '#' to url encoding.
I validated these changes with unit tests for applicable scenarios and by
running a locally-compiled version of GCM with tracing enabled and the
following config set:
credential.http://localhost:7990/bitbucket.provider bitbucketTrace logs showed that the override was recognized and auto-detection
was skipped.
Fixes #608