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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

External fetch/push #394

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

External fetch/push #394

wants to merge 4 commits into from

Conversation

tom95
Copy link
Collaborator

@tom95 tom95 commented Oct 15, 2022

This adds a preference that makes the git browser invoke the git CLI for fetch and push. The rationale here is to support authentication via SSH. If you think that SSH support landing in Squot/Squit anytime soon is highly unlikely, I would even expose this option as part of the GitBrowser settings. Otherwise, I would consider this a temporary quirk.

Note that clone via the GitBrowser was not adapted.

A soft-dependency on OSProcess is added via Smalltalk classNamed:.

Copy link
Contributor

@LinqLover LinqLover left a comment

Choose a reason for hiding this comment

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

Sounds interesting! Two thoughts from my side:

  • Apart from SSH authentication, this might also be useful when you have problems pushing, e.g. when pushing refs with some huge new chunks. Should this be mentioned in the preference description?

  • To what extent is the handling of edge cases currently aligned to the existing behavior, e.g., no upstream branch, no right to push, or the remote server being offline?

@tom95
Copy link
Collaborator Author

tom95 commented Oct 15, 2022

Apart from SSH authentication, this might also be useful when you have problems pushing, e.g. when pushing refs with some huge new chunks. Should this be mentioned in the preference description?

I think, since going to the preferences to temporarily adapt behavior when an error occurs is a bit of an odd recommendation (even though I understand the rationale), I would tend not to include it in the preference text.

To what extent is the handling of edge cases currently aligned to the existing behavior, e.g., no upstream branch, no right to push, or the remote server being offline?

If fetching/pushing fails for any reason, the error text is displayed. The modified code is one level "below" the code that determines the upstream branch, so the default Squot behavior already does the trick.

However, I just realized things fail catastrophically if you have a password set on your SSH key. I'm a bit worried that we may not be able to fix that entirely for Windows (no stdin support) but let's see...

@@ -1,7 +1,7 @@
git porcelain - external
externalGitDo: aCommandLineSuffix showText: aBoolean
| res |
res := self externalCommand: ('git -C {1} {2}' format: {repository workingDir. aCommandLineSuffix}).
res := self externalCommand: ('git -C {1} {2}' format: {repository workingDir pathName copyReplaceAll: ' ' with: '\ '. aCommandLineSuffix}).
Copy link
Contributor

Choose a reason for hiding this comment

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

What about quotes? Afaik linux filenames are allowed to contain double quotes (") as well :P

@LinqLover
Copy link
Contributor

If fetching/pushing fails for any reason, the error text is displayed. The modified code is one level "below" the code that determines the upstream branch, so the default Squot behavior already does the trick.

If possible, it would be great if we could still "force push" or "create a new upstream branch" by pressing the right button in the error dialog ... but we can also leave this out for now if it is more complicated.

However, I just realized things fail catastrophically if you have a password set on your SSH key. I'm a bit worried that we may not be able to fix that entirely for Windows (no stdin support) but let's see...

Just random googling, would this work for SSH passwords? Personally, I never use a password for my SSH keys :D

@tom95
Copy link
Collaborator Author

tom95 commented Oct 15, 2022

If possible, it would be great if we could still "force push" or "create a new upstream branch" by pressing the right button in the error dialog ... but we can also leave this out for now if it is more complicated.

Short of parsing the output of the git CLI and hoping that it remained and will remain stable throughout its history or switching to libgit2 there is no way I can think of to provide more nuanced actions for this approach.

Just random googling, would this work for SSH passwords? Personally, I never use a password for my SSH keys :D

Good idea! Sadly, it only applies to passwords when used with HTTP auth. In the meantime, I found out that current Linux'es will most likely invoke ssh-agent, which spawns a graphical dialog and does not need stdin, to unlock the key. So no problems here but it's still rather fragile in the end.

An idea that would probably work well would be to spawn a real terminal. Two issues, however:

  • there is no real standardized way to get a terminal (best approximation is apparently cmd.exe/x-terminal-emulator/osascript -e 'tell app "Terminal" to do script ""')
  • the terminal just finishes once the command exited, so if an error occurred we have no way to communicate that to the user, besides learning from the exit code (which we can obtain) that something went wrong

I assume you could get surprisingly far, if not all the way, with this PR as-is, as long as we can demand of users to configure their system such that fetch/pull work non-interactively. For a non-default option and with a clear disclaimer this may be acceptable, but not sure. (I'm already enjoying the functionality in any case :D)

@LinqLover
Copy link
Contributor

I assume you could get surprisingly far, if not all the way, with this PR as-is, as long as we can demand of users to configure their system such that fetch/pull work non-interactively.

+1. The other questions are only fine-tuning and should not block this PR IMHO. :-)

there is no real standardized way to get a terminal (best approximation is apparently cmd.exe/x-terminal-emulator/osascript -e 'tell app "Terminal" to do script ""')

Well, you wrote that stdin is only unavailable for Windows, right? Then you would only need cmd.exe for opening a terminal and could use the stdin directly otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants