lgoldstein commented on code in PR #430:
URL: https://github.com/apache/mina-sshd/pull/430#discussion_r1381259350
##########
sshd-git/src/main/java/org/apache/sshd/git/pack/GitPackCommand.java:
##########
@@ -77,7 +79,11 @@ public void run() {
Repository db = key.open(true /* must exist */);
String subCommand = args[0];
if (RemoteConfig.DEFAULT_UPLOAD_PACK.equals(subCommand)) {
- new UploadPack(db).upload(getInputStream(), getOutputStream(),
getErrorStream());
+ UploadPack uploadPack = new UploadPack(db);
+ String protocol = this.getEnvironment().getEnv()
+
.getOrDefault(GitProtocolConstants.PROTOCOL_ENVIRONMENT_VARIABLE, "version=0");
Review Comment:
`version=0` should be a **literal constant** as well
```java
public final class GitProtocolConstants {
public static final String PROTOCOL_ENVIRONMENT_VARIABLE = "whatever";
public static final String DEFAULT_PROTOCOL_VALUE = "version=0";
}
```
and then the code here should be
```java
this.getEnvironment().getEnv()
.getOrDefault(GitProtocolConstants.PROTOCOL_ENVIRONMENT_VARIABLE,
GitProtocolConstants.DEFAULT_PROTOCOL_VALUE);
```
##########
sshd-git/src/main/java/org/apache/sshd/git/transport/GitSshdSession.java:
##########
@@ -128,7 +133,7 @@ public Process exec(String commandName, int timeout) throws
IOException {
log.trace("exec({}) session={}, timeout={} sec.", commandName,
session, timeout);
}
- ChannelExec channel = session.createExecChannel(commandName);
+ ChannelExec channel = session.createExecChannel(commandName, null,
version2Env);
Review Comment:
This should not be hardcoded - please use the [Properties and inheritance
model](https://github.com/lgoldstein/mina-sshd/blob/master/docs/internals.md#properties-and-inheritance-model)
to create the environment map using whatever defaults you think are
appropriate. This lets the users configure something else if they think it is
necessary.
##########
sshd-git/src/main/java/org/apache/sshd/git/transport/GitSshdSession.java:
##########
@@ -30,13 +32,16 @@
import org.eclipse.jgit.transport.CredentialsProvider;
import org.eclipse.jgit.transport.RemoteSession;
import org.eclipse.jgit.transport.URIish;
+import static org.eclipse.jgit.transport.GitProtocolConstants.*;
import org.eclipse.jgit.util.FS;
/**
* @author <a href="mailto:dev@mina.apache.org";>Apache MINA SSHD Project</a>
*/
public class GitSshdSession extends AbstractLoggingBean implements
RemoteSession {
+ private static final Map version2Env =
Collections.singletonMap(PROTOCOL_ENVIRONMENT_VARIABLE, VERSION_2_REQUEST);
Review Comment:
Please use generics - `Map<SomeKeyType, SomeValueType>`. Anyway, this is not
necessary - see my comment regarding using properties model
##########
sshd-git/src/main/java/org/apache/sshd/git/transport/GitSshdSession.java:
##########
@@ -30,13 +32,16 @@
import org.eclipse.jgit.transport.CredentialsProvider;
import org.eclipse.jgit.transport.RemoteSession;
import org.eclipse.jgit.transport.URIish;
+import static org.eclipse.jgit.transport.GitProtocolConstants.*;
Review Comment:
We never use `import static` and definitely never use star imports. I am
pretty sure this code did not compile using our build framework as we have
codestyle plugins that are supposed to prevent this. Please fix this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org
For additional commands, e-mail: dev-h...@mina.apache.org
