Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create only one SSH session per GitOperation #1012

Merged
merged 4 commits into from Aug 12, 2020

Conversation

@fmeum
Copy link
Member

@fmeum fmeum commented Aug 12, 2020

馃摙 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring
  • Dependency updates

馃摐 Description

We should only create a single SSH session per GitOperation to reduce overhead and prevent repeated password prompts without bandaid solution and I finally figured out how to do it.

Along the way, a subtle bug is fixed that can lead to Git operations continuing after an error has been encountered.

馃挕 Motivation and Context

Would greatly improve the usability of #995 with security tokens and simplifies the code considerably.

馃挌 How did you test it?

I synced and cancelled password prompts and everything worked as expected.

馃摑 Checklist

  • I formatted the code with the IDE's reformat action (Ctrl + Shift + L/Cmd + Shift + L)
  • I reviewed submitted code
  • I added a CHANGELOG entry if applicable

馃敭 Next steps

馃摳 Screenshots / GIFs

fmeum added 4 commits Aug 12, 2020
This reverts commit 889208b.
This reverts commit 4172c70.
@fmeum fmeum requested review from msfjarvis and Skrilltrax as code owners Aug 12, 2020
@fmeum fmeum changed the title Enhancement/multiple ssh commands Create only one SSH session per GitOperation Aug 12, 2020
@fmeum
Copy link
Member Author

@fmeum fmeum commented Aug 12, 2020

I'm still surprised this works as per hierynomus/sshj#584 (comment), so please try to break it.

@msfjarvis
Copy link
Member

@msfjarvis msfjarvis commented Aug 12, 2020

I'm still surprised this works as per hierynomus/sshj#584 (comment), so please try to break it.

Oh? Hold off the merge then I'll try some of my broken-ish servers.

@fmeum
Copy link
Member Author

@fmeum fmeum commented Aug 12, 2020

Oh? Hold off the merge then I'll try some of my broken-ish servers.

Thanks!

On a slightly different note: While testing, I discovered that we currently always create a sync commit, even if it's empty (see android-password-store/pass-test@cd23dab). This is not a big problem, but unnecessarily complicates the Git history. Is this something that would be easy to change?

@msfjarvis
Copy link
Member

@msfjarvis msfjarvis commented Aug 12, 2020

Oh? Hold off the merge then I'll try some of my broken-ish servers.

Thanks!

On a slightly different note: While testing, I discovered that we currently always create a sync commit, even if it's empty (see android-password-store/pass-test@cd23dab). This is not a big problem, but unnecessarily complicates the Git history. Is this something that would be easy to change?

Yeah I'll take a look, shouldn't be impossible.

@msfjarvis msfjarvis merged commit 4e8da9b into develop Aug 12, 2020
5 checks passed
5 checks passed
test-pr (23, freeRelease)
Details
test-pr (23, nonFreeRelease)
Details
test-pr (29, freeRelease)
Details
test-pr (29, nonFreeRelease)
Details
WIP Ready for review
Details
@msfjarvis msfjarvis deleted the enhancement/multiple_ssh_commands branch Aug 12, 2020
@msfjarvis msfjarvis added this to the 1.11.0 milestone Aug 12, 2020
@msfjarvis msfjarvis added the code label Aug 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can鈥檛 perform that action at this time.