-
Notifications
You must be signed in to change notification settings - Fork 599
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added charset support, centralized UTF-8 usage #305
Conversation
* @return remote character set or {@code null} for default | ||
*/ | ||
public Charset getRemoteCharset() | ||
{ |
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.
Please use the same formatting as the rest of the code, ie. open-curly on end-of-line
@@ -93,7 +98,7 @@ public Command exec(String command) | |||
throws ConnectionException, TransportException { | |||
checkReuse(); | |||
log.debug("Will request to exec `{}`", command); | |||
sendChannelRequest("exec", true, new Buffer.PlainBuffer().putString(command)) | |||
sendChannelRequest("exec", true, new Buffer.PlainBuffer().putString(command.getBytes(getRemoteCharset()))) |
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.
The main question I have when reading this, should every string be written using the remote charset? If so, it is nicer to make it a property of the Buffer, rather than converting it everywhere we call putString
.
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.
Or make a putString(String s, Charset cs)
method, which makes the rest of the code at least cleaner and more concise.
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.
The remote side seems to decode the command using its own charset instead of utf-8. Commands containing filenames with special characters won't work properly using utf-8 on non-utf-8 remote machines.
I'll add the putString(String s, Charset cs)
method. There will still be a lot of getRemoteCharset()
calls in SFTPEngine though which could also be replaced by a property.
@@ -128,6 +129,9 @@ | |||
|
|||
private final List<LocalPortForwarder> forwarders = new ArrayList<LocalPortForwarder>(); | |||
|
|||
/** character set of the remote machine */ | |||
protected Charset remoteCharset; |
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.
Assign the default IOUtils.UTF_8
here.
* remote character set or {@code null} for default | ||
*/ | ||
public void setRemoteCharset(Charset remoteCharset) { | ||
this.remoteCharset = remoteCharset; |
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.
Ensure that you cannot set null
@Override | ||
public Session startSession() | ||
throws ConnectionException, TransportException { | ||
checkConnected(); | ||
checkAuthenticated(); | ||
final SessionChannel sess = new SessionChannel(conn); | ||
final SessionChannel sess = (remoteCharset != null)? new SessionChannel(conn, remoteCharset) : new SessionChannel(conn); |
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.
No need to do the if in the case that we default to UTF_8 here already.
THanks, merged :) |
Here's the fix as promised in Issue #277. Feel free to make changes.