fix: Include port in GitRemote origin #4393
Conversation
| @@ -170,6 +178,7 @@ private String repositoryPath(@Nullable String origin) { | |||
| throw new IllegalArgumentException("Unable to find origin '" + origin + "' in '" + hostAndPath + "'"); | |||
| } | |||
| return hostAndPath.substring(origin.length()) | |||
| .replaceFirst("^:\\d+", "") | |||
There was a problem hiding this comment.
It strips out the port number when creating the repository path. The port number is present because we first built hostAndPath then take a substring of it.
There was a problem hiding this comment.
OK, you're right. This should not be required through I have to update GitRemote.Parser#parse to cache host + port as the origin
There was a problem hiding this comment.
I updated registerRemote to cache origins with their port number if present. I also added gitlab.com:22 to the list of known origins. I'm not sure if gitlab is special and if we should include origins with :22 and :443 for github and bitbucket as well. I figure this would only be relevant for on-prem installs?
There was a problem hiding this comment.
Personally, I find these all as the same SCM origin given the current logic:
https://github.com
https://github.com:443
ssh://[email protected]
ssh://[email protected]:22
With both of the port style URLs above, they are just declaring explicitly the default port. Declaring the port is optional, however, when there is a well established default value for the proticol. So to me they all refer to the same origin as GitRemote isn't drawing any distinction between protocols/ports at the moment.
Now if you wanted protocol to be a part of the GitRemote spec, then I'd say the above maps into two -- HTTPS and SSH remotes -- but the presence of absence of the default port doesn't create a new origin.
There was a problem hiding this comment.
hmm I think you are right and we should ignore explicit default port values.
Adding the protocol into the mix would make it "more correct/complete" but the problem it solves is such a niche (2 servers on different protocols on their default ports) that it's probably not worth it.
What's changed?
Update
GitRemoteto include port in origin.What's your motivation?
Host port is required to avoid ambiguity between origins.
Anything in particular you'd like reviewers to focus on?
No
Anyone you would like to review specifically?
@pstreef
Have you considered any alternatives or workarounds?
No
Any additional context
No
Checklist