search

git-up / GitUp

The Git interface you've been missing all your life has finally arrived.
http://gitup.co
GNU General Public License v3.0
11.44k stars 1.23k forks source link

Check if `SSH_AUTH_SOCK` env var is set before trying to use the SSH agent. #987

Open lapfelix opened 2 months ago

lapfelix commented 2 months ago

Fixes the cloning test through ssh (GCSingleCommitRepositoryTests's testClone) on computers that don't have a correctly setup ssh agent, like mine 🥴 (as the keys in ~/.ssh will be used instead, and that's going to work as long as they're not protected by a passphrase).

If SSH_AUTH_SOCK isn't there, it will fail later on anyway when libgit2 uses libssh2.

Should also make ssh agent workarounds like this one unnecessary: https://github.com/git-up/GitUp/issues/161#issuecomment-643863909

I AGREE TO THE GITUP CONTRIBUTOR LICENSE AGREEMENT

lucasderraugh commented 2 months ago

@lapfelix Being relatively unfamiliar with this portion of the codebase, does libssh2 not support accessing passphrased sah keys?

lapfelix commented 2 months ago

hmm it would definitely be possible to support passphrased ssh keys, and GitUp could save the passphrase in the keychain.

(In theory though the ssh agent should "just work" but in practice, I'm not sure why it's not super reliable and why it sometimes needs to be kickstarted after a reboot)

lucasderraugh commented 2 months ago

I guess I'm trying to figure out what this particular PR is trying to improve. Wouldn't this environment variable be set as long as ssh-agent was running? Is this just meant to short circuit the case where it isn't?

lapfelix commented 2 months ago

Yes basically I didn't find any straightforward way to retry calling the _CredentialsCallback, but if the ssh-agent isn't running, the environment variable won't be set, so we don't even need to try the ssh-agent, we know it'll fail.

This PR helps cases where the ssh agent isn't running, but maybe there's a more "correct" solution to the problem. Like I wonder when line 455 ever executed, for instance

lucasderraugh commented 1 week ago

For some reason I seem to be failing in testClone with or without this change on the SSH clone GCSingleCommitRepositoryTests:173 and I'm trying to figure out why but I assume it's related to what you were trying to fix here. I know these tests were all passing previously, not sure if macOS version changed something here but if you know how to get them passing, I'd be happy to hear it.

Really should setup Xcode Cloud after this to have some easy CI wins.

Cykelero commented 1 week ago

It seems that when running tests, the SSH_AUTH_SOCK env variable is not defined; whereas when running the app target, the variable does have a value (something like /private/tmp/com.apple.launchd.A4ChBGpp47/Listeners). I think that's what preventing libgit2 from cloning the repo.

But, why does that difference exist? I'm not familiar with how Xcode runs things enough, and looking at the schemes, I didn't find anything obvious that'd explain the difference.

lucasderraugh commented 1 week ago

Ya it's strange as I know this passed previously without change to this. And yes cloning this repo via SSH in app does work.