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

Fix support for ssh clones of kart repos. #40

Merged
merged 1 commit into from Nov 26, 2021

Conversation

hamishcampbell
Copy link
Member

The plugin currently fails to recognize SSH paths to hosted repositories.

This change moves the responsibility for determining the users intended clone source into the clone widget. This also fixes handling of paths already prefixed with file:// and tightens up the checking of http/https paths.

This moves the responsibility for determining the users intended
clone source into the widget code. `kart@` and `http(s)://` paths
are passed to the API, other paths are assumed to be file system
paths.
@rcoup
Copy link
Member

rcoup commented Nov 23, 2021

Shouldn't Kart have that responsibility?

One issue... file protocol isn't the same as a file path: file:// uses the remote network protocol locally (with separate send & receive processes) so it's a lot slower than just accessing the path's Git dir which is how a file path works.

We're also missing ssh://, and other ssh usernames are possible 😀

@hamishcampbell
Copy link
Member Author

hamishcampbell commented Nov 23, 2021

This is a heuristic to interpret the input into the "URL of repository to clone" input. It seems appropriate for the widget to do it.:

Screen Shot 2021-11-24 at 11 40 41 AM

I would expect Kart to be more explicit.

One issue... file protocol isn't the same as a file path: file:// uses the remote network protocol locally (with separate send & receive processes) so it's a lot slower than just accessing the path's Git dir which is how a file path works.

Should it strip the file:// if it exists, instead of adding it when its missing? I'm unclear why this code exists at all then. Should it just accept whatever is provided instead of modifying it?

@volaya
Copy link
Contributor

volaya commented Nov 24, 2021

Should it strip the file:// if it exists, instead of adding it when its missing? I'm unclear why this code exists at all then. Should it just accept whatever is provided instead of modifying it?

No, the reason for adding it is that, if the file:// prefix is missing, Kart will not print our progress information, so we are forcing it in case there is no protocol explicitely defined. I guess that changing that behaviour in Kart (not sure why it behaves like that) is probably the best solution

@rcoup
Copy link
Member

rcoup commented Nov 24, 2021

the reason for adding it is that, if the file:// prefix is missing, Kart will not print our progress information

aha. This is a Git thing, not a Kart thing. So without the file:// Git clone/fetch/push just does OS file copies from the "remote" directory (or even instant copy-on-write on supported filesystems). Git could print progress in a similar/different way, but it doesn't at the moment.

→ I don't think % progress is worth the extra slowness - mostly people will use ssh/https remotes and see the progress.

We could get progress on the working copy population phase, currently there isn't any but that is in Kart's control.

@hamishcampbell
Copy link
Member Author

As it stands, SSH clones don't work at all, which is what I'm was seeking to fix. This doesn't change the existing file system handling though, perhaps that should be a separate issue?

@rcoup
Copy link
Member

rcoup commented Nov 24, 2021

We need to drop all the prefixing IMO and hand it off to Kart

@hamishcampbell
Copy link
Member Author

Will it break file based clones if the progress dialog doesn't work?

@volaya
Copy link
Contributor

volaya commented Nov 26, 2021

No, it won't break anything to remove the prefixing. I will merge the PR, so ssh clones work fine, and then remove the part that adds the fil:// prefix

@volaya volaya merged commit a808d2c into koordinates:main Nov 26, 2021
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.

None yet

3 participants