- Notifications
You must be signed in to change notification settings - Fork2.5k
ssh: Omit port option from ssh command unless specified in remote url#6845
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Yeah, you're right. Looking at git, they specify I added#6851 so that we could discern that as well. Does that make sense in this regard and can you check whether the port was specifically provided using that? |
That is what I wanted. I'll rebase this PR on#6851 and using that to check whether we should add |
35dccc3 toce1d2c0CompareThere 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.
Sorry; I didn't notice that you'd pushed an update. One thing I notice is that I think that we can avoid an allocation of theport_str
src/libgit2/transports/ssh_exec.c Outdated
| @@ -131,6 +131,7 @@ static int get_ssh_cmdline( | |||
| git_str ssh_cmd = GIT_STR_INIT; | |||
| const char *default_ssh_cmd = "ssh"; | |||
| int error; | |||
| git_str port_str = GIT_STR_INIT; | |||
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.
| git_str port_str = GIT_STR_INIT; |
src/libgit2/transports/ssh_exec.c Outdated
| url->username ? url->username : "", | ||
| url->username ? "@" : "", | ||
| url->host, | ||
| command, | ||
| url->path); | ||
| done: | ||
| git_str_dispose(&port_str); |
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.
| git_str_dispose(&port_str); |
src/libgit2/transports/ssh_exec.c Outdated
| if (url->port_specified && (error = git_str_printf(&port_str, "-p %s", url->port)) < 0) | ||
| goto done; | ||
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.
| if (url->port_specified&& (error=git_str_printf(&port_str,"-p %s",url->port))<0) | |
| gotodone; |
src/libgit2/transports/ssh_exec.c Outdated
| ssh_cmd.size > 0 ? ssh_cmd.ptr : default_ssh_cmd, | ||
| url->port, | ||
| port_str.ptr, |
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.
| port_str.ptr, | |
| url->port_specified ?"-p " :"", | |
| url->port_specified ?url->port :"", |
Omit `-p 22` option from ssh command by default.Adding `-p 22` option when a port is not in a remote url causes that`Port` option in a ssh config is ignored.
ce1d2c0 tod2389b2Compare
Right. I've removed the useless allocation. |
cb0bf67 intolibgit2:mainUh oh!
There was an error while loading.Please reload this page.
Thanks for the fix! Great improvement. |
Uh oh!
There was an error while loading.Please reload this page.
Currently, when using
GIT_SSH_EXECfeature,libgit2doen't respect aPortconfig in ssh config because it always insert-p 22option to a ssh command if there is no port value in a git remote url (e.g.git@my-custom-domain.org:user/path). I've experienced this for my company's GHE.One caveat of this implementation is that if you specify port22in a remote url despite a different port in ssh config, it will be ignored.For example, in~/.ssh/config:and if you try to fetch or push with a remote urlssh://git@my-custom-domain.org:22/user/path, the port1234will be used.It would be unlikely someone tries to do this though, I guess.EDITED:
Now
-p <port>option is added only when the port is specified in a remote url.