Skip to content
This repository was archived by the owner on Sep 11, 2020. It is now read-only.

Fix ssh workflow #96

Merged
merged 7 commits into from
Oct 27, 2016
Merged

Fix ssh workflow #96

merged 7 commits into from
Oct 27, 2016

Conversation

alcortesm
Copy link
Contributor

Fix #80

We were sending an additional \n to the server when sending the
upload request, see clients/ssh/git_upload_pack.go:174:

fmt.Fprintln(si, r.String())

The reason for this was to flush the input stream, otherwise, the
message was not send to the server.

Also, we were (and still are) not checking remote execution errors, so
we were unaware of this error, reading the first portion of the packfile
as if nothing were wrong.

On the few ocasions when the server was quick enough to fail before
sending the full packfile, one of our tests (the one that checks the
received packfile size) failed.

We were also escaping the repository name in the remote command
execution string incorrectly.

Now we are:

  • using ssh.Run to run the remote command, instead of start and wait,
    which is the same but simpler.
  • using io.Copy instead of fmt.Fprintln, so we avoid adding and extra EOL and also we don't use a line buffered stream and therefore we no longer have to flush it.
  • we are closing the input stream as soon as possible, so the remote
    command can exit also as soon as possible.
  • the remote command escape character (') is used correctly

Pending tasks:

  • TODO: use advrefs, when Use advrefs in gituploadpackinfo #92 is accepted
  • TODO support multi_ack mode
  • TODO support multi_ack_detailed mode
  • TODO support acks for common objects
  • TODO build a proper state machine for all these processing options
  • TODO (VERY IMPORTANT) don't ignore the error from the remote execution of the command.

We were sending an additional `\n` to the server when sending the
upload request, see clients/ssh/git_upload_pack.go:174:

    fmt.Fprintln(si, r.String())

The reason for this was to flush the input stream, otherwise, the
message was not send to the server.

Also, we were (and still are) not checking remote execution errors, so
we were unaware of this error, reading the first portion of the packfile
as if nothing were wrong.

On the few ocasions when the server was quick enough to fail before
sending the full packfile, one of our tests (the one that checks the
received packfile size) failed.

We were also escaping the repository name in the remote command
execution string incorrectly.

Now we are:

- using ssh.Run to run the remote command, instead of start and wait,
  which is the same but simpler.

- using io.Copy instead of fmt.Fprintln, so we avoid adding and extra EOL and also we don't use a line buffered stream.
  and we no longer have to flush it.

- we are closing the input stream as soon as possible, so the remote
  command can exit also as soon as possible.

- the remote command escape character (') is used correctly
@alcortesm
Copy link
Contributor Author

alcortesm commented Oct 27, 2016

Take a careful look at commit 154dbe4 that removes the panic and propagate the error to the close of the stream.

I'm pretty sure we will not leak the Gorutine, nor the new channel, no matter in which order the session and remote command ends.

Sadly, this cannot be easily tested yet, until we include our own SSH test server, so we can use our own git-upload-pack implementation that fails when we want it to.

You can temporally test it by modifying Fetch to send bad upload-request though: just adding a si.Write([]byte("\n") before closing si will do the trick. This will reproduce the old SSH bug, with the difference that now you will get an error when closing the stream :).

I'm not sure how to make the close error semantics, though, maybe adding it to the Fetch comment, let me know your opinion on this matter and I will modify the PR.

@mcuadros mcuadros merged commit e25b29c into src-d:master Oct 27, 2016
@alcortesm alcortesm deleted the fix-ssh-workflow branch November 10, 2016 17:33
mcuadros pushed a commit that referenced this pull request Jan 31, 2017
* Fix #80

We were sending an additional `\n` to the server when sending the
upload request, see clients/ssh/git_upload_pack.go:174:

    fmt.Fprintln(si, r.String())

The reason for this was to flush the input stream, otherwise, the
message was not send to the server.

Also, we were (and still are) not checking remote execution errors, so
we were unaware of this error, reading the first portion of the packfile
as if nothing were wrong.

On the few ocasions when the server was quick enough to fail before
sending the full packfile, one of our tests (the one that checks the
received packfile size) failed.

We were also escaping the repository name in the remote command
execution string incorrectly.

Now we are:

- using ssh.Run to run the remote command, instead of start and wait,
  which is the same but simpler.

- using io.Copy instead of fmt.Fprintln, so we avoid adding and extra EOL and also we don't use a line buffered stream.
  and we no longer have to flush it.

- we are closing the input stream as soon as possible, so the remote
  command can exit also as soon as possible.

- the remote command escape character (') is used correctly

* WIP

* ssh: return remote command exit value when closing the packfile stream
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants