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

Jump to bottom

Create only one SSH session per GitOperation #1012

Merged
merged 4 commits into from Aug 12, 2020
Merged

Create only one SSH session per GitOperation #1012

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 fmeum changed the title Enhancement/multiple ssh commands Create only one SSH session per GitOperation Aug 12, 2020
@fmeum fmeum mentioned this pull request Aug 12, 2020
Open
@fmeum
Copy link
Member Author

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

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 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

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
@msfjarvis msfjarvis deleted the enhancement/multiple_ssh_commands branch August 12, 2020 14:41
@msfjarvis msfjarvis added this to the 1.11.0 milestone Aug 12, 2020
@msfjarvis msfjarvis added the code label Aug 12, 2020
@fmeum fmeum mentioned this pull request Aug 13, 2020
Closed
@fmeum fmeum mentioned this pull request Aug 27, 2020
Closed
@fmeum fmeum mentioned this pull request Sep 10, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msfjarvis msfjarvis msfjarvis approved these changes

@Skrilltrax Skrilltrax Awaiting requested review from Skrilltrax

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.11.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@fmeum @msfjarvis