sshaw / git-link

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

Do explicit url--allowed-chars for second url-hexify-string argument #94

Closed bitti closed 2 years ago

bitti commented 2 years ago

Passing this argument as list is only supported since Emacs 27.0.90 (when bug#26469 was implemented), so we need to do an explicit conversion to a vector to keep to support older versions. The helper function url--allowed-chars is available since url-hexify-string allows the second argument, which is since Emacs 24.2.90. That should be early enough for all versions in common use today.

I tested this locally and it still works fine for my Emacs 28.0.50.

sshaw commented 2 years ago

Hi, thanks so much for coming through with a quick fix for this!

/ isn't being escaped:

ELISP> emacs-version
"27.2"
ELISP> (url-hexify-string "foo ooo /bar" (url--allowed-chars (cons ?/ url-unreserved-chars)))
"foo%20ooo%20/bar"

Replacing ?/ with "/" works but did not try on 26 —yet!

I didn't quite understand the user case for /. Can you elaborate?

url--allowed-chars implies it's a "private" function via --, is this your understanding? I mean we can use it but just asking because I program in a constant state of paranoia and this is an emacs convention.

bitti commented 2 years ago

/ isn't being escaped

That's the whole point of adding it to the allowed chars, it shouldn't be escaped.

I didn't quite understand the user case for /. Can you elaborate?

As mentioned, for github it's cosmetic since it also accepts the escaped version as path separators, for other providers I'm not so sure and there may be a danger that it can be interpreted as part of a filename, so not escaping it is the proper way I think. (And no, never use / in a filename, although it's technically possible).

bitti commented 2 years ago

url--allowed-chars implies it's a "private" function via --, is this your understanding?

Yes, but it's available since the second arg for url-hexify-string got implemented about 10 years ago, and I don't think that will change in the foreseeable future. Copying it's code on the other hand would make it depended on even more implementation details.

bitti commented 2 years ago

never use / in a filename, although it's technically possible

Ok, I'm wrong on that, seems it's basically the only character which is not possible in a filename (besides\0 of course). In any case escaped and unescaped slashes are interpreted differently by webservers in general, therefore I think we should keep them unescaped.

sshaw commented 2 years ago

/ isn't being escaped

That's the whole point of adding it to the allowed chars, it shouldn't be escaped.

Ha, yes of course 🙃

Thanks again for all your help!