Re: [PR] GH-429: Support GIT protocol-v2 [mina-sshd]
tomaswolf closed pull request #430: GH-429: Support GIT protocol-v2 URL: https://github.com/apache/mina-sshd/pull/430 -- 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
Re: [PR] GH-429: Support GIT protocol-v2 [mina-sshd]
lgoldstein commented on PR #430: URL: https://github.com/apache/mina-sshd/pull/430#issuecomment-1850539152 Merged - see [43288b58c7](https://github.com/apache/mina-sshd/commit/f5c63a87d46ca5820e17af00b4e65a43288b58c7) with acknowledgement and thanks for your contribution - you can close the PR now. -- 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
Re: [PR] GH-429: Support GIT protocol-v2 [mina-sshd]
lgoldstein commented on code in PR #430:
URL: https://github.com/apache/mina-sshd/pull/430#discussion_r1418394044
##
sshd-git/src/main/java/org/apache/sshd/git/transport/GitSshdSession.java:
##
@@ -133,7 +132,16 @@ public Process exec(String commandName, int timeout)
throws IOException {
log.trace("exec({}) session={}, timeout={} sec.", commandName,
session, timeout);
}
-ChannelExec channel = session.createExecChannel(commandName, null,
version2Env);
+ChannelExec channel;
+Optional protocolVer =
GitModuleProperties.GIT_PROTOCOL_VERSION.get(session);
+if (protocolVer.isPresent()) {
Review Comment:
Recommend using the following code:
```java
Optional protocolVer =
GitModuleProperties.GIT_PROTOCOL_VERSION.get(session);
String protocol = protocolVer.orElse(null);
if (GenericUtils.isNotBlank(protocol)) {
...
}
```
```
##
sshd-git/src/main/java/org/apache/sshd/git/pack/GitPackCommand.java:
##
@@ -80,9 +80,10 @@ public void run() {
String subCommand = args[0];
if (RemoteConfig.DEFAULT_UPLOAD_PACK.equals(subCommand)) {
UploadPack uploadPack = new UploadPack(db);
-String protocol = this.getEnvironment().getEnv()
-
.getOrDefault(GitProtocolConstants.PROTOCOL_ENVIRONMENT_VARIABLE, "version=0");
-uploadPack.setExtraParameters(Collections.singleton(protocol));
+String protocol =
this.getEnvironment().getEnv().get(GitProtocolConstants.PROTOCOL_ENVIRONMENT_VARIABLE);
+if (protocol != null) {
Review Comment:
Recommend using
```java
if (GenericUtils.isNotBlank(protocol)) {
...
}
```
```
--
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
Re: [PR] GH-429: Support GIT protocol-v2 [mina-sshd]
lgoldstein commented on PR #430: URL: https://github.com/apache/mina-sshd/pull/430#issuecomment-1844520166 Almost there - the only (minor) issue is assuming that the protocol property is either *null* or valid - as a matter of "safety" unless empty/blank strings have special semantics we treat them as "equivalent" to *null*. See my 2 comments - once you fix this I would be happy to merge this in. Good job on the whole... -- 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
Re: [PR] GH-429: Support GIT protocol-v2 [mina-sshd]
pflaska commented on code in PR #430:
URL: https://github.com/apache/mina-sshd/pull/430#discussion_r1416286714
##
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:
The `version=0` or `version=1` is sometimes used to specify an original
version. The previous version is not specified by any env. variable when the
original protocol is used. I follow the same pattern in the code now and we no
longer use `version=0`.
--
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
Re: [PR] GH-429: Support GIT protocol-v2 [mina-sshd]
pflaska commented on code in PR #430: URL: https://github.com/apache/mina-sshd/pull/430#discussion_r1416276242 ## 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: Ok, now the imports section contains only the `GitProtocolConstants` class. -- 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
Re: [PR] GH-429: Support GIT protocol-v2 [mina-sshd]
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);
```
--
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
Re: [PR] GH-429: Support GIT protocol-v2 [mina-sshd]
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 mailto:dev@mina.apache.org;>Apache MINA SSHD Project
*/
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`. 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
