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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(ssh): use /tmp to place control dir on darwin #5223

Merged
merged 1 commit into from
Jan 3, 2023
Merged

fix(ssh): use /tmp to place control dir on darwin #5223

merged 1 commit into from
Jan 3, 2023

Conversation

aymanbagabas
Copy link
Contributor

OpenSSH complains if the control path is too long (>=104) On Darwin, os.MkdirTemp is too long, use /tmp instead.

ControlPath too long ('/var/folders/v3/5pnzpycs0pzdpytmgfzc7tj80000gn/T/sock-96855798/sock-b677e145501e220cfb5583ba1638a77ffe779df9' >= 104 bytes)

@aymanbagabas aymanbagabas requested a review from a team as a code owner December 15, 2022 08:00
@aymanbagabas aymanbagabas force-pushed the darwin-control-path-too-long branch from 3edf358 to 5957c47 Compare December 15, 2022 08:06
bk2204
bk2204 reviewed Dec 15, 2022
View reviewed changes
Copy link
Member

@bk2204 bk2204 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thanks for the patch. I think there are still a few things we need to address here before going forward.

ssh/ssh.go Outdated Show resolved Hide resolved
ssh/ssh.go Outdated Show resolved Hide resolved
@aymanbagabas
fix(ssh): use /tmp to place control dir on darwin
83fdf8e 
OpenSSH complains if the control path is too long (>=104)
On Darwin, os.MkdirTemp is too long, use /tmp instead.

ControlPath too long ('/var/folders/v3/5pnzpycs0pzdpytmgfzc7tj80000gn/T/sock-96855798/sock-b677e145501e220cfb5583ba1638a77ffe779df9' >= 104 bytes)

Signed-off-by: Ayman Bagabas <[email protected]>
@aymanbagabas aymanbagabas force-pushed the darwin-control-path-too-long branch from 5957c47 to 83fdf8e Compare December 18, 2022 09:17
@chrisd8088
Copy link
Member

Just FYI, most of the Git LFS core team is away on holidays, so it might take a few weeks for this to be reviewed again.

chrisd8088
chrisd8088 approved these changes Dec 19, 2022
View reviewed changes
Copy link
Member

@chrisd8088 chrisd8088 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me now, as it seems tightly constrained to just the getControlDir() function. But I know brian was looking at it too, so I'll leave it for him to review as well after the holidays.

Thanks again for the patch!

@bk2204 bk2204 merged commit 2feca81 into git-lfs:main Jan 3, 2023
@chrisd8088 chrisd8088 mentioned this pull request Jul 19, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisd8088 chrisd8088 chrisd8088 approved these changes

@bk2204 bk2204 bk2204 approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@aymanbagabas @chrisd8088 @bk2204