sshaw / git-link

Emacs package to get the GitHub/Bitbucket/GitLab/... URL for a buffer location
394 stars 73 forks source link

match organization/username capitalization to fix Github permalink preview #57

Closed peterbecich closed 3 years ago

peterbecich commented 6 years ago

For the permalink preview to appear I think the capitalization must be matched exactly.

https://github.com/sshaw/git-link/blob/efd14ab5f17f5942d25e165210447f3983f3250e/git-link-test.el#L6

https://github.com/Sshaw/git-link/blob/efd14ab5f17f5942d25e165210447f3983f3250e/git-link-test.el#L6

It seems git-link produces lower-case permalinks in all cases. I don't know how involved this fix is for such a tiny issue, nor if the properly capitalized string is even available.

I get a ton of value out of git-link -- thank you for the terrific package!

sshaw commented 6 years ago

Hi,

Do your git remotes' URLs use the proper casing?

I use this with several organizations and repos that aren't lowercase and have never had a problem.

peterbecich commented 6 years ago

Thanks, it hadn't occurred to me to check that. The remotes' URLs use correct casing, though.

git-link-use-commit has no effect on this tiny problem.

I have only found this problem in a private repository and have not tried a public repository with capitalized organization.

sshaw commented 6 years ago

Hi, I cannot reproduce this, can you provide an example? And are you generating the link locally or via tramp?

sshaw commented 6 years ago

Closing. Feel free to reopen with more info or with an example that allows one to reproduce the issue.

magne-hov commented 3 years ago

I think I am experiencing the same issue as @peterbecich .

When using an SSH-style URL as remote, such as ssh://git@github.com:Some-Capitalized-Organization/And-A-Repo.git, the case of the first component of the URL path is lost:

Tracing url-generic-parse-url while doing a M-x git-link shows:

1 -> (url-generic-parse-url "ssh://git@github.com:Some-Capitalized-Organization/And-A-Repo.git")
1 <- url-generic-parse-url: #s(url "ssh" "git" nil "github.com:some-capitalized-organization" nil "/And-A-Repo.git" nil nil t ...)

To me, it looks like url-generic-parse-url is not able to correctly interpret SSH-style URLs where the port number is omitted and a relative path is given. The first component of the path is instead interpreted as the port number and downcased together with the rest of the hostname (which indeed is case-insensitive).

This is a bit of a shame, as this is the style of clone URL that GitHub will suggest for you.

I can work around this issue by making the remote URL have an absolute path, and the resulting links produce permalink previews in GH posts:

-ssh://git@github.com:Some-Capitalized-Organization/And-A-Repo.git
+ssh://git@github.com:/Some-Capitalized-Organization/And-A-Repo.git
1 -> (url-generic-parse-url "ssh://git@github.com:/Some-Capitalized-Organization/And-A-Repo.git")
1 <- url-generic-parse-url: #s(url "ssh" "git" nil "github.com" nil "/Some-Capitalized-Organization/And-A-Repo.git" nil nil t ...)
sshaw commented 3 years ago

Ah I see @magne-hov. The disconnect is my case the URLs were being downcased but the page was not 404ing. In your case they are 404ing. Is your example for a private repo too?

magne-hov commented 3 years ago

Skye Shaw notifications@github.com writes:

ELISP> (git-link--parse-remote "git@github.com:/Some-Capitalized-Organization/And-A-Repo.git")
("github.com" "Some-Capitalized-Organization/And-A-Repo")
ELISP> (git-link--parse-remote "git@github.com:sshaw/Time-Timecode.git")
("github.com" "sshaw/Time-Timecode")
ELISP> (git-link--parse-remote "git@github.com:ScreenStaring/shopify_id_export")
("github.com" "screenstaring/shopify_id_export")

Also I can link no problem to the above repos using the following remotes:

~/code/perl/Time-Timecode >git remote -v
origin    git@github.com:sshaw/Time-Timecode.git (fetch)
origin    git@github.com:sshaw/Time-Timecode.git (push)
~/.go/src/github.com/screenstaring/shopify_id_export >git remote -v
origin    git@github.com:ScreenStaring/shopify_id_export.git (fetch)
origin    git@github.com:ScreenStaring/shopify_id_export.git (push)

Does linking to the above repos work for you?

For the two above repos I've cloned them using the URLs above.

  1. For git@github.com:sshaw/Time-Timecode.git I get f.ex this link: https://github.com/sshaw/Time-Timecode/blob/2939ab1807d445f5f56b58b3bdd326c30ea706d8/Makefile.PL#L6

  2. And for git@github.com:ScreenStaring/shopify_id_export.git I get f.ex this link: https://github.com/screenstaring/shopify_id_export/blob/2c8ed49e4f6e1706d0a176276271f27b398aac56/README.md#L14

The URL for repository 1. looks good.

The URL for repository 2. is bad. The capitalization of ScreenStaring has been lost. This is the same as you seem to have got with git-link--parse-remote:

("github.com" "screenstaring/shopify_id_export")

Opening this URL in a browser works fine; a HTTP request resolves to the same page. However, using this URL in a GitHub comment in the same repository will not produce a preview rendering of the line of code.

In your case they are 404ing. Is your example for a private repo too?

All of the links I get work fine for HTTP, there's no 404-ing.

Edit: I moved this final section to a new non-email comment to get previews working in it.

magne-hov commented 3 years ago

Apologies, I didn't realise that emails didn't support Markdown, which was needed for the preview. I've copied that section here so that the preview shows when it should be expected.


Github's preview links only seem to produce previews when posted in the repository that they link to. That means that I can't make any links with git-link that point to this repository but which also fail to produce the previews.

I hope the following example can illustrate the problem. Below are two pair of links. The first of each pair is verbatim so we can see the actual target of the link, and the second of each pair is without any formatting on its own line.

The only change between the pairs is that sshaw -> Sshaw. When the capitalisation is wrong, a preview is not produced.

https://github.com/sshaw/git-link/blob/93ea243277f2f2092b77928f0e1155e3e3a8da6a/git-link-test.el#L6:

https://github.com/sshaw/git-link/blob/93ea243277f2f2092b77928f0e1155e3e3a8da6a/git-link-test.el#L6

https://github.com/Sshaw/git-link/blob/93ea243277f2f2092b77928f0e1155e3e3a8da6a/git-link-test.el#L6:

https://github.com/Sshaw/git-link/blob/93ea243277f2f2092b77928f0e1155e3e3a8da6a/git-link-test.el#L6

sshaw commented 3 years ago

Github's preview links

oooooh..... you know looking at Peter's original issue he mentioned this clearly and somehow I missed it and here we are 🤦

Nevertheless I'm toying around with 2 fixes locally: 1) patches in 22/ if there's no port and proceeds with parsing 2) ~other~ extracts the projects name after parsing if there's a : in the hostname.

Both kinda ugly and makes me think that maybe url-generic-parse-url should be dropped for a regex. Thoughts?

Either way should have a fix pushed in ~24 hours. Thanks for the help!

⌛

magne-hov commented 3 years ago

Both kinda ugly and makes me think that maybe url-generic-parse-url should be dropped for a regex.

Considering the range of formats accepted by git, it might be cleaner to just go for a regex instead of trying to coax every format into a valid URL.

Thanks for the help!

Thank you for the package! I use it almost every day.

sshaw commented 3 years ago

Considering the range of formats accepted by git, it might be cleaner to just go for a regex instead

Used to do that and ran into some issues 8a43dc026e847354c845509cc8d62583e5113ff2

Either way should have a fix pushed in ~24 hours. Thanks for the help!

Yeah going to have to push this back to the weekend unfortunately

magne-hov commented 3 years ago

While looking around for inspiration I found this Rust library which seems to do a good job of parsing GitHub URLs https://github.com/tjtelan/git-url-parse-rs.

sshaw commented 3 years ago

I found this Rust library which seems to do a good job of parsing GitHub URLs

This is the first Rust code I've looked at for more than 3 minutes 🎉

I considered creating an emacs library for this but things can get tricky. I mean we're doing some things like this already but I ain't got time to deal with all this for a general case lib.

I ended up piggybacking onto the existing code to fix scp style URLs. Have a look and let me know if it fixes things for you: https://github.com/sshaw/git-link/commit/3dfe155a8fb2cec386ffb804bcfdeb1d7cd08a03

@peterbecich @magne-hov

magne-hov commented 3 years ago

I ended up piggybacking onto the existing code to fix scp style URLs. Have a look and let me know if it fixes things for you: 3dfe155

The changes on your casefix branch fixes things on my end. It feels like an acceptable solution considering the irregularity of the scp style.

Thank you for looking at this so quickly!

sshaw commented 3 years ago

Released in v0.8.1