`gix-url` parses SSH aliases as file paths

[niklaswimmer]

Duplicates

• I have searched the existing issues

Current behavior 😯

The parsing of URLs in gix-url does not respect the user's SSH config file. Instead, it classifies abbreviated SSH URLs as file paths.

For example, in my SSH config I have the following entry defined to abbreviate git@github.com to just github (and to choose a specific key file for this host).

Host github
    User git
    Hostname github.com
    IdentityFile ~/.ssh/git/github_rsa

Cloning with Git then looks like git clone github:byron/gitoxide. If I try gix clone github:byron/gitoxide I get the following error:

Error: Could not verify that "/tmp/github:byron/gitoxide" url is a valid git directory before attempting to use it

Caused by:
    0: Could not retrieve metadata of "/tmp/github:byron/gitoxide/.git"
    1: No such file or directory (os error 2)

The problem seems to stem from the URL parsing logic which marks any URL without an @ or . as a file path:

https://github.com/Byron/gitoxide/blob/2087032b5956dcd82bce6ac57e530e8724b57f17/gix-url/src/parse.rs#L39-L45

I first noticed this issue with ein tool organize, which skips all file URLs silently because they do not have a host specified: https://github.com/Byron/gitoxide/blob/2087032b5956dcd82bce6ac57e530e8724b57f17/gitoxide-core/src/organize.rs#L162-L165

Expected behavior 🤔

I would expect gitoxide to parse such abbreviated URLs correctly. I am not sure exactly how though.

I looked at the Git source and to me it seems like they simply never check for the @ character (see this function here which is then used here)?

However, looking at gitoxide's tests, it seems to me like the current behavior is the expected one: https://github.com/Byron/gitoxide/blob/2087032b5956dcd82bce6ac57e530e8724b57f17/gix-url/tests/parse/ssh.rs#L68

Given that, the only solution I can think of is to look at the SSH config to determine if we are dealing with an SSH alias?

If we can find an acceptable solution I would be happy to contribute the implementation myself :)

Steps to reproduce 🕹

No response

[Byron]

Thanks for taking the time to create such a thorough and well-researched issue, it's very helpful :)!

There has been lots of back and forth with supporting git urls and their various forms. gix-url was among the first crates I wrote and back then, I did it roughly by looking at the docs (which don't usually cover everything) rather than by following the git implementation to the letter.

Further, I checked the commit message of the test case above that wants the wrong behaviour, and it didn't make clear why that's supposed to be required at all - my feeling is it's just to put a pin into the current behaviour and call it a day.

With that said, I think that should change, and following what git does is the way to go as this is part of the mission of gitoxide, also if this means expectations in tests have to change along with it.

Your help would be greatly appreciated, and I think bringing this check to gix-url along with other logic could be a viable step forward. Feel free to rewrite the logic entirely if everything is backed by tests to which git agrees. Thanks so much!

[niklaswimmer]

my feeling is it's just to put a pin into the current behaviour and call it a day.

For what its worth, I got the same impression.

I will look into it!

[niklaswimmer]

@Byron would it be acceptable/possible that we make all fields of the Url struct private? Once that is done it would be possible to change the internal representation without introducing further breaking changes to the API. Of course we would provide getter methods instead.

I have some ideas for how to best represent an url internally, but I figured I better check in with you on what is possible before doing much thinking.

[Byron]

Yes, absolutely, please feel free to make all the breaking changes you see fit to make this work and get the quality we need.
gix-url was one of the first crates and it shows. Time for this to change :).

[niklaswimmer]

Alright, my current goal is to start by porting over all test-cases from git and libgit2. That should give us a good idea of where our parsing lacks. I would then go ahead and disable all the failing tests for now and track them in something like a "gix-url to 1.0" issue.

As a next step I think it would make sense to rewrite the parsing logic from the ground up. It currently gives me some grown system vibes that added new cases here and there over time. we can definitely do better than that, given that we basically already know all possible cases beforehand by examining git/libgit2/our tests. This is also the phase where I would have to break the public API (although I think I could break it but keep the current parsing and then change the parsing without breaking it again, which is probably better for releases).

Lastly we would work towards making all test pass, maybe polish up the public API and that should be enough to essentially "finish" gix-url in my opinion.

Any thoughts?

[Byron]

Alright, my current goal is to start by porting over all test-cases from git and libgit2. That should give us a good idea of where our parsing lacks. I would then go ahead and disable all the failing tests for now and track them in something like a "gix-url to 1.0" issue.

That's a great idea, more tests is better! It's OK to explicitly 'remember' to fix the disabled tests in the tracking issue, even though I think they won't be forgotten, particularly because I wouldn't expect the disabled tests to be around for too long.

As a next step I think it would make sense to rewrite the parsing logic from the ground up. It currently gives me some grown system vibes that added new cases here and there over time. we can definitely do better than that, given that we basically already know all possible cases beforehand by examining git/libgit2/our tests. This is also the phase where I would have to break the public API (although I think I could break it but keep the current parsing and then change the parsing without breaking it again, which is probably better for releases).

The current API provides direct access to fields which should be OK given that we know everything the URL can be or contain. If you find or see other reasons to change that though, I am all up for it. That means, I don't want accessor methods just so they are there, and will always prefer direct member access if it's safe to do, i.e. all possible states are valid. Thus far, this seemed the case.

Lastly we would work towards making all test pass, maybe polish up the public API and that should be enough to essentially "finish" gix-url in my opinion.

Yes, that should be the 1.0 release :).

Any thoughts?

I can't wait to see the first PR (or the PR if it's all done in go nonetheless). Thanks so much for tackling this!

[niklaswimmer]

Fixed via #990